Skip to content

Fix some Windows bugs in 4.x - #301

Open
ddeclerck wants to merge 1 commit into
OCamlPro:gitside-masterfrom
ddeclerck:windows_fixes
Open

Fix some Windows bugs in 4.x#301
ddeclerck wants to merge 1 commit into
OCamlPro:gitside-masterfrom
ddeclerck:windows_fixes

Conversation

@ddeclerck

@ddeclerck ddeclerck commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator

This PR fixes 20 failing tests in the Windows MSVC CI (9 tests under MSYS2).

@ddeclerck

Copy link
Copy Markdown
Collaborator Author

@GitMensch Note that at least two more tests could be easily fixed (opinion needed).

  • INDEXED undeclared keys

Difference in error message :

-libcob: prog.cob:82: error: unknown file error (status = 39) for file file2 ('./fileX') on OPEN
+libcob: prog.cob:82: error: unknown file error (status = 39) for file file2 ('./fileX' => .\fileX) on OPEN

This is because cob_get_filename_print considers ./fileX and .\fileX to be different.
I see two options here:

  • using a dedicated path comparison that ignores separator differences
  • normalizing paths to use unix-style spearators

Any preference ?

  • trace feature with indexed EXTFH

The error is as follows:

fatal error C1189: #error:  Macro definition of snprintf conflicts with Standard Library function declaration

This should already be taken care of by this definition in common.h:

#if !COB_USE_VC2015_OR_GREATER
/* VC2015+ provides standard function with plain
   name, old posix emulation with underscore */
#define snprintf		_snprintf
#endif

However, COB_USE_VC2015_OR_GREATER is defined in cobinternal.h, which is not included in common.h. Should we move these COB_USE_VC* to common.h ? Or since this is the only occurrence, just inline the definition of COB_USE_VC2015_OR_GREATER in the definition of snprintf ?

@ddeclerck
ddeclerck force-pushed the windows_fixes branch 3 times, most recently from 86438d9 to 248d0db Compare August 13, 2026 11:53
@codecov-commenter

codecov-commenter commented Aug 13, 2026

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 50.00000% with 8 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (gitside-master@f27c23a). Learn more about missing BASE report.

Files with missing lines Patch % Lines
libcob/fileio.c 53.84% 1 Missing and 5 partials ⚠️
libcob/cobcapi.c 0.00% 1 Missing and 1 partial ⚠️
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.
Additional details and impacted files
@@                Coverage Diff                @@
##             gitside-master     #301   +/-   ##
=================================================
  Coverage                  ?   62.94%           
=================================================
  Files                     ?       40           
  Lines                     ?    72837           
  Branches                  ?    20312           
=================================================
  Hits                      ?    45849           
  Misses                    ?    19799           
  Partials                  ?     7189           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ddeclerck
ddeclerck force-pushed the windows_fixes branch 3 times, most recently from bb5e488 to 798d9c6 Compare August 13, 2026 18:51

@GitMensch GitMensch left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't be the changes to build_windows be relevant for 3.x already (or did we just not merge them yet)?

Can you please commit https://github.com/OCamlPro/gnucobol/pull/300/changes upstream? I guess that will fix at least part of the MSVC issues here as well, no?

... why don't we have that much failures on MSYS2 also on 3.x? (the fileio parts are clear, these are new in 4.x and not that well tested on Windows)

Comment thread libcob/fileio.c Outdated
Comment thread libcob/fileio.c Outdated
Comment thread libcob/fileio.c
Comment thread tests/testsuite.src/run_misc.at
@ddeclerck

Copy link
Copy Markdown
Collaborator Author

Wouldn't be the changes to build_windows be relevant for 3.x already (or did we just not merge them yet)?

Yes, I'm already preparing a PR for that - just my focus was on 4.x for now (takes time to context-switch my brain between the two branches). It will also include the fixes that also apply to 3.x (protecting the call to fdcobsync and cob_free in cob_get_buff).

Can you please commit https://github.com/OCamlPro/gnucobol/pull/300/changes upstream? I guess that will fix at least part of the MSVC issues here as well, no?

Yes, will do that right now. And this has to be merged in 4.x too (I'll add it to the GC3/GC4 merge PR).

... why don't we have that much failures on MSYS2 also on 3.x? (the fileio parts are clear, these are new in 4.x and not that well tested on Windows)

Many failures were just because of new code in 4.x. Also, some new tests in 4.x "accidentally" revealed bugs that are present in 3.x but not triggered.

P.S : any suggestion regarding the two bugs mentionned in #301 (comment) ?

@ddeclerck
ddeclerck force-pushed the windows_fixes branch 2 times, most recently from dd921fd to d002ef1 Compare August 14, 2026 14:04
@ddeclerck ddeclerck mentioned this pull request Aug 14, 2026
@GitMensch

Copy link
Copy Markdown
Collaborator

This is because cob_get_filename_print considers ./fileX and .\fileX to be different. I see two options here:

* using a dedicated path comparison that ignores separator differences

that's reasonable for [_WIN32] - but then this should likely include /c -> [cC]: (I tend to not ignore file case, as FAT32 and NTFS case-sensitive exists on both Windows and Linux)...

This should already be taken care of by this definition in common.h.... in cobinternal.h

OK, that's a "new" bug in GC4 then; I understand Ron's idea with cobinternal.h, but ... am not 100% sure what to do with this and other headers splits. Please think about the general approach and then share your thoughts.

For that specific test where a C test file is compiled: do we need snprintf there in first case?
Isn't there a function in libcob that we could call instead (when then also ensures that the runtime from libcob is used)?

@GitMensch

Copy link
Copy Markdown
Collaborator

Also, some new tests in 4.x "accidentally" revealed bugs that are present in 3.x but not triggered.

I hope we get testcases for those together with the bugfix to 3.x :-)

@GitMensch GitMensch left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Comment thread libcob/fbdb.c Outdated
Comment thread libcob/cobcapi.c
Comment on lines 24 to 26
#ifndef _GNU_SOURCE
#define _GNU_SOURCE 1
#endif

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unrelated, just wondering - shouldn't this be in config.h as well?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just checked, the generated config.h already has the above _GNU_SOURCE definition (whatever the target - Linux, MSYS2 or MacOS). The static config.h for MSVC also has the same definition.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please drop it here then

@ddeclerck ddeclerck Sep 1, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, _GNU_SOURCE is defined in 3 libcob files: numeric.c, cobcapi.c and call.c. Should it be removed in all three ?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes (that's 4.x only, right?), and all of those should have a very early include of config.h

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same in 3.x

@ddeclerck
ddeclerck force-pushed the windows_fixes branch 4 times, most recently from b82f5b1 to bdba87c Compare September 1, 2026 10:08

@GitMensch GitMensch left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only request so far: Changelog... and possibly implementing changes in 3.x first

Comment thread tests/testsuite.src/run_file.at
Comment thread tests/testsuite.src/run_file.at
Comment on lines 26507 to 26508

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we don't have that test in twice (once with , once without -std=mf) then this test should be also included in 3.x (renamed, with adjusted test result) [otherwise only the "non mf" test can be copied over and/or this test be copied, but skipped with a note that dialect specific io status is not implemented yet.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note we have two MF-specific tests here : "MF io status (indexed)" and "Test MF Status (sequential)" (below). Both only compiled with -std=mf.

I'm fine with the idea of backporting relevant tests from 4.x to 3.x, but if we do so, wouldn't it be better to have a dedicated PR for this and do this globally - instead of sneaking in a few tests from time to time ?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PRs always welcomed :-)

Comment thread libcob/fileio.c
Comment thread libcob/ChangeLog
Comment thread .github/workflows/build_nightly.yml
Comment thread .github/workflows/windows-msys2.yml Outdated
@ddeclerck
ddeclerck force-pushed the windows_fixes branch 2 times, most recently from 6a91637 to 317e03c Compare September 1, 2026 10:54
Comment thread libcob/Makefile.am
lib_ci = libcobci.la
libcobci_la_SOURCES = libcobci.c fsqlxfd.c # includes fisam.c
libcobci_la_LIBADD = libcob.la $(CISAM_LIBS)
libcobci_la_LIBADD = libcob.la $(CISAM_LIBS) $(LIBCOB_LIBS)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess that works... and then we need to split those in configure.ac/Makefile.am; maybe LIBCOB_LIBS_COMMON (otherwise we'll have curses, libxml, gmp, ... here as well)?

... but in general we should have most of the others separate (as the plan is to have a split libcob-xml, libcob-json libcob-curses before the 4.x release candidate (following the io library split).

While Ron originally planned to only have that for allowing multiple libraries with the same entry point based on the same source file for "multi indexed backends" - this is something that will allow to spit libcob packaging and therefore heavily reduce the dependencies - no curses used, none needed (and also none dlopen'd - distinguished for each process).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants