Add/update Windows workflows

ddeclerck commented 4 months ago

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 commented 4 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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

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

Additional details and impacted files

```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

ddeclerck commented 4 months ago

@GitMensch This is still WIP, ignore for now )

GitMensch commented 4 months ago

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 commented 4 months ago

... 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 commented 4 months ago

... 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 commented 4 months ago

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 commented 4 months ago

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 commented 3 months ago

It seems the downloads on GitHub and vcpkg are working again (see https://github.com/microsoft/vcpkg/issues/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 commented 3 months ago

@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 commented 3 months ago

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 #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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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

ddeclerck commented 2 months ago

Worked on bit on this. Had to fix a few things.

Now only those failed tests remain : 10 (save-temps in sub-directory), 26 (cobc diagnostics show caret) and 850 (runtime check: write to internal storage) Note sure about 8 (is it still random, or solved for good ?).

Also for some unknown reason, 481 (Minimal lines per listing pages) fails locally on my Windows machine, but not in the CI (apparently substitutions not pereformed)...

GitMensch commented 2 months ago

The fail wit 8 may be only applicable to GCC, not sure. What do you think of using make checkall here and also || make check TESTSUITEFLAGS=--recheck --verbose, to have the few failing test details also in the console log?

Note: the -llibsomething seems not portable... Any idea how to work around that?

ddeclerck commented 2 months ago

The fail wit 8 may be only applicable to GCC, not sure.

Testing locally, it seems MSVC is affected by this too (though it seems to work in the CI for some reason).

What do you think of using make checkall here and also || make check TESTSUITEFLAGS=--recheck --verbose, to have the few failing test details also in the console log?

Well, I was willing to avoid the "make" path, to speed up the CI (less MSYS2 packages to install, no need to run configure...). Plus, I've just tested on my Windows machine, and I can't seem to get make check to work with the GnuCOBOL built for MSVC. Depending on the path I take, either it insists on (re)building an MSYS2 GnuCOBOL and using that one, or it accepts using the MSVC GnuCOBOL but passes additional GCC-only flags, resulting in failure...

Note: the -llibsomething seems not portable... Any idea how to work around that?

Not really. To my understanding this is just because MSVC does not add a "lib" prefix to libraries. Maybe this could be tweaked directly in GnuCOBOL, when passing a library, check whether the file exists with or without the "lib" prefix and adjust accordingly (works as long as both do not exist at the same time).

GitMensch commented 2 months ago

Well, I was willing to avoid the "make" path, to speed up the CI (less MSYS2 packages to install, no need to run configure...). Plus, I've just tested on my Windows machine, and I can't seem to get make check to work with the GnuCOBOL built for MSVC. Depending on the path I take, either it insists on (re)building an MSYS2 GnuCOBOL and using that one, or it accepts using the MSVC GnuCOBOL but passes additional GCC-only flags, resulting in failure...

This should only be the case when you don't use the atlocal_win part.

For the -llib - my recent commit should work around that (by compiling to libfilec and filec). Checking the library name at compile time would mean to know the complete lookup path of the linker, that's unlikely to be added completely in a portable way.

I haven't checked the caret test yet and we possibly need the ml.exe/ml64.exe part done.

GitMensch commented 2 months ago

Well, I was willing to avoid the "make" path, to speed up the CI (less MSYS2 packages to install, no need to run configure...). Plus, I've just tested on my Windows machine, and I can't seem to get make check to work with the GnuCOBOL built for MSVC. Depending on the path I take, either it insists on (re)building an MSYS2 GnuCOBOL and using that one, or it accepts using the MSVC GnuCOBOL but passes additional GCC-only flags, resulting in failure...

Checking again what you did, that's fine. My suggested adjustment is:

-bash -c "./testsuite"
+bash -c "./testsuite || ./testsuite --recheck --verbose"

GitMensch commented 2 months ago

Tests 6 + 10 seems to be the same and likely an error in how cobc executes cl:

executing:  cl.exe /c /I "D:\a\gnucobol\gnucobol" /I
        "C:\vcpkg\installed\x64-windows\include" /nologo /MD /c
        /Fa"prog.asm" /Fo"prog.asm"
        "C:\Users\RUNNER~1\AppData\Local\Temp\cob1364_0.c"
return status:  1
stdout:
cobc (GnuCOBOL) 3.3-dev.20240516
Built     May 16 2024 12:40:00  Packaged  May 16 2024 12:38:35 UTC
C version (Microsoft) 1939
  C:\Users\RUNNER~1\AppData\Local\Temp\cob1364_0.c : fatal error C1083: Cannot open compiler generated file: 'prog.asm': Permission denied

Can you have a look at that command line and a fix?

If that works locally then it is likely a timing issue and it would help to mv the file that's there from the last creation.

For now the other two tests fail reproducible, I suggest for the MSVC workflow you edit AT_XFAIL in there before generating the testsuite - then this could be pulled in and monitor for new failures.

ddeclerck commented 2 months ago

executing: cl.exe /c /I "D:\a\gnucobol\gnucobol" /I "C:\vcpkg\installed\x64-windows\include" /nologo /MD /c /Fa"prog.asm" /Fo"prog.asm" "C:\Users\RUNNER~1\AppData\Local\Temp\cob1364_0.c"

Looks like the reason is because both /Fa and /Fo point to the same file (I was able to reproduce the error just calling cl.exe).

ddeclerck commented 2 months ago

In cobc.c:8216 we have :

    sprintf (cobc_buffer, cb_source_debugging ?
        "%s /c %s %s /Od /MDd /Zi /FR /c /Fa\"%s\" /Fo\"%s\" \"%s\"" :
        "%s /c %s %s     /MD          /c /Fa\"%s\" /Fo\"%s\" \"%s\"",
            cobc_cc, cobc_cflags, cobc_include, name,
            name, fn->translate);

Changing this way solves the problem :

    sprintf (cobc_buffer, cb_source_debugging ?
        "%s /c %s %s /Od /MDd /Zi /FR /c /Fa\"%s\" /Fo\"%s\" \"%s\"" :
        "%s /c %s %s     /MD          /c /Fa\"%s\" /Fo\"%s\" \"%s\"",
            cobc_cc, cobc_cflags, cobc_include, name,
            fn->object, fn->translate);

Does that sound reasonable ?

GitMensch commented 2 months ago

Yes, and if that works for you locally and in the CI build - just commit that upstream.

ddeclerck commented 2 months ago

As for test 26, making this change in the _return_path function in atlocal_win seems to work locally under both MSYS2 and Cygwin (waiting for CI to complete) :

    if test "$OSTYPE" = "cygwin" -o "$OSTYPE" = "msys"; then
        cygpath -pm "$1"
    else

(ie adding msys and using -pm instead of -pw)

GitMensch commented 2 months ago

simon@mingw-msys ~
$ echo $OSTYPE
msys

simon@mingw-msys ~
$ cygpath
bash.exe": cygpath: command not found

simon@mingw-msys ~
$ env | grep -i msys
PS1=\[\033]0;$MSYSTEM:\w\007
MSYSTEM=MINGW32

simon@msys2-mingw32 ~
MSYS2_NOSTART=yes
WD=D:\dev\msys64\usr\bin\
MSYSTEM=MINGW32
MSYSTEM_CARCH=i686
MSYSTEM_CHOST=i686-w64-mingw32
PS1=\[\e]0;\w\a\]\n\[\e[32m\]\u@\h \[\e[35m\]$MSYSTEM\[\e[0m\] \[\e[33m\]\w\[\e[0m\]\n\$
MSYSTEM_PREFIX=/mingw32

We therefore cannot check only for "msys", but a separate test should work, -o x$MSYSTEM_CHOST != x seems to be ok. This could be set manually, of course, but at least I've only seen that for MSYSTEM_PREFIX so far.

ddeclerck commented 2 months ago

After checking, it messes up the CI anyways, so...

ddeclerck commented 2 months ago

Seems we're all good now.

GitMensch commented 2 months ago

We'd expect a hand written valid one to be assembled correctly. As noted earlier, we possibly need to define and use a macro with ml/ml64 to reach that.

Am 17. Mai 2024 16:31:45 MESZ schrieb OCP David Declerck @.***>:

@ddeclerck commented on this pull request.

@@ -8217,7 +8217,7 @@ process_compile (struct filename *fn) "%s /c %s %s /Od /MDd /Zi /FR /c /Fa\"%s\" /Fo\"%s\" \"%s\"" : "%s /c %s %s /MD /c /Fa\"%s\" /Fo\"%s\" \"%s\"", cobc_cc, cobc_cflags, cobc_include, name,

name, fn->translate);

Well, since MSVC's assembly listing is "informational", we're not expecting this to work, do we ? Even calling ml64.exe directly on the .asm file, I get some errors.

-- Reply to this email directly or view it on GitHub: https://github.com/OCamlPro/gnucobol/pull/140#discussion_r1605109821 You are receiving this because you were mentioned.

Message ID: @.***>

ddeclerck commented 2 months ago

simon@mingw-msys ~
$ echo $OSTYPE
msys

simon@mingw-msys ~
$ cygpath
bash.exe": cygpath: command not found

simon@mingw-msys ~
$ env | grep -i msys
PS1=\[\033]0;$MSYSTEM:\w\007
MSYSTEM=MINGW32

I'm curious about how you obtain these environments. Whether I start the msys2 shell from the exe wrappers (msys2.exe, ucrt64.exe...) or the msys2_shell.cmd script, I have all of the MSYSTEM, MSYSTEM_CARCH, MSYSTEM_CHOST and MSYSTEM_PREFIX variables defined, and cygpath available in PATH.

GitMensch commented 2 months ago

That's the original MSYS (the one that has WinXP and possible older support), not MSYS2. As their site is still down and the Internet archive use is cumbersome, I'd grab them from Arnold's site - this is what he also uses to create the official (small) MinGW package.

GitMensch commented 1 month ago

@ddeclerck What's the state with this PR (for both the 3.x and the gc4 branch)?

ddeclerck commented 1 month ago

@ddeclerck What's the state with this PR (for both the 3.x and the gc4 branch)?

Well, I consider this to be "good enough" for its purpose, i.e. ensuring we don't break Windows MSVC builds. That's also what I'm using for the GC4 CI.

Note the changes to cobc.c and atlocal_win, which should probably be commited upstream independently for this PR.

GitMensch commented 1 month ago

feel free to check in those two changes upstream (in this case also an entry in tests/Changelog, please)

GitMensch commented 1 month ago

It seems there is an issue with caching WinFlexBison which you may want to inspect in any case I suggest to run the build_windows/makedist.cmd and upload the result as artifact

as there are more C RTS checks in the debug build you may consider building both Release and Debug (if I remember right the debug one actually shows an not-yet-resolved issue in the MSVC builds)

Would it be reasonable to run NIST as well?

GitMensch commented 1 month ago

In any case there should be a build matrix to build and test both win32 and x64 platforms (and possibly both Release and Debug).

Oh and just out of interest - would it be complicated to also test with VC2013 (as oldest-supported version in 3.x - not relevant for trunk)?

In this case vcpkg cannot be used but something like the following would work instead

wget https://versaweb.dl.sourceforge.net/project/gnucobol/dependencies/windows/old/win_prerequistes_vc11.7z
7z.exe x win_prerequistes_vc11.7z
move gmp.h mpir.h
cd Win32  &&  copy "Release\libvbisam.dll" "Debug\"

GitMensch commented 1 month ago

Looks nice, so if this runs then it should be merged (and then the same commit also be applied to gc4).

ddeclerck commented 1 month ago

Looks nice, so if this runs then it should be merged (and then the same commit also be applied to gc4).

Well, there are indeed failures under debug configurations, as well as two tests that I had to exclude as they would hang (only in debug):

CURRENCY SIGN WITH PICTURE SYMBOL (run_misc.at)
FUNCTION RANDOM (run_functions.at)

(aren't you on vacation ? :D)

GitMensch commented 1 month ago

So, are you inspecting those failures/hangs locally? I guess they will directly stop at the right place when you execute the failing prog.exe under debugger control. These are only runtime, right?

GitMensch commented 3 weeks ago

ping @ddeclerck for checking the issues above (even if they are 3.x - but they are likely identical in 4.x)

ddeclerck commented 3 weeks ago

ping @ddeclerck for checking the issues above (even if they are 3.x - but they are likely identical in 4.x)

I'm overwhelmed, can't do that for now. @engboris, could you give me a hand ?

engboris commented 3 weeks ago

ping @ddeclerck for checking the issues above (even if they are 3.x - but they are likely identical in 4.x)

I'm overwhelmed, can't do that for now. @engboris, could you give me a hand ?

I'd like to help (and I'm always eager to learn new things) but I'll probably need a significant onboarding as I have currently no knowledge of Github CI / Windows MSVC.

ddeclerck commented 3 weeks ago

ping @ddeclerck for checking the issues above (even if they are 3.x - but they are likely identical in 4.x)

I'm overwhelmed, can't do that for now. @engboris, could you give me a hand ?

I'd like to help (and I'm always eager to learn new things) but I'll probably need a significant onboarding as I have currently no knowledge of Github CI / Windows MSVC.

I'll show you (hopefully by the end of the week ?).

GitMensch commented 2 weeks ago

Just checking about the state @ddeclerck @engboris. I think there was more discussion about the CI somewhere (including which tests and artifacts would be reasonable), but at least https://github.com/OCamlPro/gnucobol/pull/147#issuecomment-2226872297 hints at running make mingwdist and providing the two resulting artifacts (for both MSYS1 and MSYS2) and similar there is build_windows\makedist.cmd` to create artifacts for the the MSVC builds.

ddeclerck commented 2 weeks ago

Forgot to mention it, but the MYSY1 and MSYS2 CIs (on the GC4 branch) have already been updated according to https://github.com/OCamlPro/gnucobol/pull/147#issuecomment-2226872297.

As for MSVC, it's on the toto-list, but as a matter of fact, all the team is overwhelmed, so it will take some time.

GitMensch commented 2 weeks ago

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

ddeclerck commented 2 weeks ago

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 commented 1 week ago

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 commented 1 week ago

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 ?

OCamlPro / gnucobol

Add/update Windows workflows #140

Codecov Report