Add/update Windows workflows

[ddeclerck]

This PR adds an MSVC workflow to compile GnuCOBOL and run the testsuite.

Note 1: we manually generate the tests/atconfig and tests/package.m4 files to avoid having to generate and call ./configure

Note 2: about 80 tests fail (I get the same results locally on an actual Windows machine)

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 65.86%. Comparing base (89c45a3) to head (7ced79d).

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@                  Coverage Diff                  @@
##           gcos4gnucobol-3.x     #140      +/-   ##
=====================================================
- Coverage              65.87%   65.86%   -0.01%     
=====================================================
  Files                     32       32              
  Lines                  59483    59483              
  Branches               15709    15709              
=====================================================
- Hits                   39182    39181       -1     
- Misses                 14282    14283       +1     
  Partials                6019     6019              

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

[GitMensch]

The one missing and important thing is to make the testsuite log available as artifact (if this does not exist then the generated config.h will be useful, just from experience).

You likely also want to run the dist script from build_windows to additional provide the resulting binaries for easy test.

Currently all the run tests are missing. Guess: some of the dlls are not found, but the testsuite.log will show what the issue really is.

[GitMensch]

You likely want to make the package string dynamic, for example by copying it from configure.ac

[GitMensch]

You likely either want to run that in any case (should be quite fast after getting the cache) or want to do that once a day/week, so you do get updates also without manually dropping the cache.

[ddeclerck]

@GitMensch This is still WIP, ignore for now )

[GitMensch]

Ah, now it is a draft ;-) Just thought about some issues I've seen when working on the appimage definition - while this isn't linked by commit message or manually, I think this is thought to implement #48 #103.

[GitMensch]

... and of course, I am interested in this one:

about 80 tests fail (I get the same results locally on an actual Windows machine)

Last time I've run the tests there were much less failures (and additional 1-2 when using the debug version as that raised some overflow/C RTS check).

[ddeclerck]

... and of course, I am interested in this one:

about 80 tests fail (I get the same results locally on an actual Windows machine)

Last time I've run the tests there were much less failures (and additional 1-2 when using the debug version as that raised some overflow/C RTS check).

I haven't investigated yet, but I seem to see a lot of linker errors: "LNK2005: DllMain already defined".
Have you met such errors before ?

[GitMensch]

I don't remember these, if you need a clue after inspecting the testsuite log and possibly make check TESTSUITEFLAGS="--recheck" COBOL_FLAGS="-g" (at least locally, where you may also add a -v in those), then feel free drop me a note.

[ddeclerck]

Seems I can't go further until the xz mess is solved...

Downloading https://github.com/tukaani-project/xz/archive/v5.4.4.tar.gz
[DEBUG] Trying to hash C:\vcpkg\downloads\tukaani-project-xz-v5.4.4.tar.gz.7420.part
[DEBUG] C:\vcpkg\downloads\tukaani-project-xz-v5.4.4.tar.gz.7420.part has hash a9fd8148a6693d5b48848274fb48cca0b3f11ba4548399c7a232d26d9f07346a709eda8e010ca2d08e29bf085f208969fdeece19b37b8539fe99ea8d518c5788
error: Failed to download from mirror set
error: File does not have the expected hash:
url: https://github.com/tukaani-project/xz/archive/v5.4.4.tar.gz

[GitMensch]

It seems the downloads on GitHub and vcpkg are working again (see microsoft/vcpkg#37893).

Can you go on with this PR, please? Making it visible which tests do fail is useful and allows us to work on fixing them, especially after comparing them with the failed MinGW tests.

[ddeclerck]

@GitMensch

It seems the downloads on GitHub and vcpkg are working again (see microsoft/vcpkg#37893).

Can you go on with this PR, please? Making it visible which tests do fail is useful and allows us to work on fixing them, especially after comparing them with the failed MinGW tests.

I resumed my work on this PR now that xz is back.

I have 17 tests that fail (I'll investigate), but for info, here is the list (any analysis of yours is welcome):

• test 10 : bad exit code (1 instead of 0)
• tests 18, 118, 1169 : linking error "DllMain already defined"
• test 26 : bad output, probably because of incomplete substitutions (not backslash-aware)
• test 27 : linking error "Unresolved external symbol f"
• tests 803, 804, 805 : runtime error "Redirection not supported"
• test 849 : runtime error "attempt to reference invalid memory address"
• tests 855, 856, 857, 859, 860, 861, 862 : all those are related to the new profiling feature ; processes end with code 127 ; I'll start by checking those

You can check the testsuite.log file here:
https://github.com/OCamlPro/gnucobol/actions/runs/8803078630/artifacts/1440008425

[GitMensch]

we manually generate the tests/atconfig and tests/package.m4 files to avoid having to generate and call ./configure

Note in general: there is a reason for the atlocal_win. If not done already I suggest checking the README under build_windows.

Comments:

• test 849 is a hairy beast: we write to internal storage to test if GnuCOBOL can recognize that - but as the memory layout outside of lvl 01 is not guaranteed (at not until this extra feature is available under GC4, and even then we want to check the then "old 'random' memory layout") it is hard to get it right: write too much / bad place and there's an abort before GnuCOBOL generated modules can even recognize; write too less and it won't be a problem on environment X [MacOS for example was different, iff I remember correctly, also enabled C runtime checks for hardening may as well break the test "before"]...
  --> I'd suggest to inspect this last and be prepared for ignoring this (to get it right you'd need a bunch of memory dump output or better a source-level debugger and inspecting memory there) - and if you get it right, then the test mail fail under a different environment
• tests 803-805: that's related to PDCurses - you either need to disable this (see atlocal_win) or potentially use a different version/port of PDCurses
• tests 26+27 -see discussion in add dependencies options and -fcopybook-deps  #109 (where I've request to move this out to a separate PRs); but test 26 should be fixable by minor adjustment in the testsuite's $SED use
• test 10, maybe the filename is not prog.s - check with a listing directory what is happening; I'm quite sure that this worked before with MSVC (and we explicit dropped the "now compile the prog.s and run it", because MSVC generated assembly does not necessary compile, it is "informational only" (whatever this means in this context)
• 18, 118+119 - these look like necessary adjustments in the cl.exe/(link.exe command line cobc uses

[ddeclerck]

we manually generate the tests/atconfig and tests/package.m4 files to avoid having to generate and call ./configure

Note in general: there is a reason for the atlocal_win. If not done already I suggest checking the README under build_windows.

I do use atlocal_win (almost unmodified, just using the Release configuration - should I use Debug instead ?)

I think I checked the README file when I wrote this workflow (it was over a month ago). Took some liberties with the instructions...

[GitMensch]

Release is fine, but as Debug includes runtime checks it is often more useful.
... of course: best would be running both Release and Debug after each other or, as in this case: just as a parallel workflow that uses the same definition, just two configurations.

[ddeclerck]

• test 10, maybe the filename is not prog.s - check with a listing directory what is happening; I'm quite sure that this worked before with MSVC (and we explicit dropped the "now compile the prog.s and run it", because MSVC generated assembly does not necessary compile, it is "informational only" (whatever this means in this context)

Indeed, the file is named prog.asm. According to the documentation, when the file name given to the /Fa option has no extension, it will use .asm by default. In cobc.c, function process_compile, we have :

	if (output_name) {
		name = output_name;
	} else {
		name = file_basename (fn->source, NULL);
#ifndef	_MSC_VER
		strcat (name, ".s");
#endif
	}

I'd say we should just add the .s extension regardless of the compiler used. Was there any reason not to do that ? (maybe different behavior with older cl.exe versions ?)

[ddeclerck]

• tests 26+27 -see discussion in add dependencies options and -fcopybook-deps  #109 (where I've request to move this out to a separate PRs); but test 26 should be fixable by minor adjustment in the testsuite's $SED use

27 seems to be a different issue. We're referring to a function that is only declared but not defined. That seems to be okay when building a library with gcc, but I don't think MSVC allows that (I belive it needs an import library for that).

[GitMensch]

Rechecked:

• all VC versions that GC3 supports (2008+) have the same rule
• earlier versions of Visual Studio and msbuiild (I guess before 2013/2015) only recognize ".asm" as assembly source (newer also can cope with .s) and then will execute masm on the file, creating an object file
• the masm binary is either ml or ml64

I therefore think that:

• For _MSVC we should explicit strcat ".asm" - so it is clearly "on purpose", ideally with a comment /* earlier versions of msbuild don't recognize .s */.
• The test (10 is about the output files) can then test for prog.s || prog.asm (maybe more if there's a default)
• For MSVC we should explicit call ml / ml64 - can you please add a define in build_windows/config.h.iin #ädefine AS "ml.exe" and use it for the MSVC compile of assembly?
• We likely can then also use another $COMPILE prog.cob -S -o prog.s followed by $COMPILE prog.s.

[GitMensch]

Your analysis for the failing test 27 is correct - could you try to create an import library (for any compiler) up-front?

[ddeclerck]

Ah, I did indeed missed 9bd5c8c, maybe "just" bring the GC4 and GC3 CI definitions up to the same state for now? Should nearly be a copy/paste, no?

Yes, that should be doable. As this PR is for GC3, I'll add all the CIs made for GC4 to this (MSYS1 & MSYS2).

[GitMensch]

hm,, the last push changed the name to the wrong file again ? [I've previously edited it it to the correct one: run_fundamental.at)

[ddeclerck]

hm,, the last push changed the name to the wrong file again ? [I've previously edited it it to the correct one: run_fundamental.at)

I'm not sure what you're talking about; is this related to this PR ?

[GitMensch]

yes, I've pushed because I've seen the test still hang, and then found that the sed used operates on the wrong file - as it currently does again.

[ddeclerck]

yes, I've pushed because I've seen the test still hang, and then found that the sed used operates on the wrong file - as it currently does again.

Ah, you pushed to my own branch, that's why. I assume people never do that, and I happily force-push to my branch to keep a single commit.

[GitMensch]

No problem, but as noted you need:

- sed -i '/AT_SETUP(\[MOVE to edited item (4)\])/a AT_SKIP_IF(\[true\])' testsuite.src/run_functions.at
+ sed -i '/AT_SETUP(\[MOVE to edited item (4)\])/a AT_SKIP_IF(\[true\])' testsuite.src/run_fundamental.at

[ddeclerck]

No problem, but as noted you need:

- sed -i '/AT_SETUP(\[MOVE to edited item (4)\])/a AT_SKIP_IF(\[true\])' testsuite.src/run_functions.at
+ sed -i '/AT_SETUP(\[MOVE to edited item (4)\])/a AT_SKIP_IF(\[true\])' testsuite.src/run_fundamental.at

Yup, I noticed that.

Of course, on the long term, those will be investigated locally.

[GitMensch]

Of course, on the long term, those will be investigated locally.

Sounds good. That may have been faster (but that's hard to know up front) - the CI now hangs at test 577 / 1304.

[ddeclerck]

So, here we are. The hangs are caused by popup windows that display errors detected by the runtime checker - of course in a CI environment without a GUI you can't see them. Most of those errors are of the kind shown in this screenshot:

So we basically have to be very pedantic about casts with data loss and explicitly express which bits we drop.
I hate MSVC so much.

[GitMensch]

https://learn.microsoft.com/en-us/windows/win32/debug/error-mode says:

Best practice is that all applications call the process-wide SetErrorMode function with a parameter of SEM_FAILCRITICALERRORS at startup. This is to prevent error mode dialogs from hanging the application.

So, in both CI cases and when running as a service, we'd like to crash the application instead of opening a popup.

During debugging on a GnuCOBOL developer environment the messages as is, including the "retry to debug", is useful.

What are your thoughts about that? Adding a call on MSVC to the startup code in libcob (also executed from cobc) to disable that popup in all cases but a special environment variable is set?

[GitMensch]

Of course that's a runtime check which is useful (and therefore explicit enabled for the debug compile) and similar things also happen with sanitizers in gcc + clang.

As you have the setup: can you check those occurrences and use bitmask for explicit loss of data?

[ddeclerck]

https://learn.microsoft.com/en-us/windows/win32/debug/error-mode says:
What are your thoughts about that? Adding a call on MSVC to the startup code in libcob (also executed from cobc) to disable that popup in all cases but a special environment variable is set?

That sounds reasonable. We could use the CI environment variable, which should be defined by most CI frameworks (at least it is on GitHub, and I believe also on Appveyor ?)

[ddeclerck]

As you have the setup: can you check those occurrences and use bitmask for explicit loss of data?

I really have to go back to the GC3/4 merge.

I spent yesterday's afternoon giving @engboris all the info about this MSVC CI and prepared for him a script that reproduces the CI steps locally, so he should be able to take over this matter.

I'd go as far as suggesting to merge this PR in its current state (as it is already very helpful for its purpose), and open a new one dedicated to the fixes.

[GitMensch]

I'd say %CI% is a possible option if we leave it otherwise on; I'm looking forward for @engboris to implement this and start checking the data-loss, bitmasking where expected, which should go into two separate PRs.

Thanks for improving the CI so far. The only thing that seems to be missing and would be nice if you could add it is the binary artifacts for MSVC, created after build by executing build_windows\makedist.cmd and upload that, maybe you can inspect that before getting back to the merge, or Boris can handle this as a first PR.

[ddeclerck]

The only thing that seems to be missing and would be nice if you could add it is the binary artifacts for MSVC, created after build by executing build_windows\makedist.cmd and upload that, maybe you can inspect that before getting back to the merge, or Boris can handle this as a first PR.

Oh I couldn't get this to work ; here's what I get (using the commented bits at the end of windows-msvc.yml):

Setup using created GnuCOBOL distribution -x64-...
Setup Visual Studio (x64/amd64)...

cl.exe already in PATH
no further initialization is done for the C compiler

cl.exe now in PATH:
C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.40.33807\bin\Hostx64\x64\cl.exe
Compilateur d'optimisation Microsoft (R) C/C++ version 19.40.33812 pour x64
Copyright (C) Microsoft Corporation. Tous droits réservés.

cl : Ligne de commande error D8003 : nom de fichier de la source absent

Setting environment for GnuCOBOL.
Building extras with

cobc had unexpected return value -1073741515, running verbose again...
C:\Users\David\Programmes\msys64\home\David\gc4-msys1\gc3-msvc\build_windows\distnew\bin_x64\cobc.exe
Abort!