OCamlPro / gnucobol

A clone of the sourceforge GnuCOBOL compiler from COBOL to C.
https://get-superbol.com
GNU Lesser General Public License v3.0
16 stars 20 forks source link

Fix testsuite 8 on Windows/MSYS2 #128

Open ddeclerck opened 7 months ago

ddeclerck commented 7 months ago

EDIT: fixes for 808 and 809 already merged

This fixes tests number 8, 808 and 809 on Windows with MSYS2.

Test 8 fails randomly during the call to gcc, as it tries to access the temporary directory. Since the objective of this test is to check the compilation behavior when tweaking TMP/TEMP/TMPDIR, I resorted to a workaround: I simply replaced $COMPILE by $COMPILE_ONLY.

As for tests 808 and 809, they fail because of differences in the way paths are shown on Windows VS Unix. Since the expected output uses relative Unix-style paths, I used sed to pre-process the output to remove any absolue Windows-style prefix and replace them with ./.

The only remaining failing test, 771, is a bit more tricky ; it will be handled separately.

ddeclerck commented 7 months ago

The replacement with [[^[:cntrl:]]] needs an adjustment, that won't work on AIX or Solaris outside of gsed.

You suggested two solutions :

Which one do you like most ?

GitMensch commented 7 months ago

You suggested two solutions :

* using `awk '{sub(/Started by [[:print:]]*prog.exe/, "Started by .\/prog")}1'`
* or just replacing `[[^[:cntrl:]]]` by `.*`

Which one do you like most ?

I don't mind. If we do use awk for that in the testsuite then this may be better (note: I haven't checked the portability of that and if it works!), otherwise I'd opt for the "more simple sed".

ddeclerck commented 7 months ago

I went for the simpler sed, since that's what's already used in the testsuite.

codecov-commenter commented 7 months ago

Codecov Report

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

Comparison is base (5713357) 65.86% compared to head (b951376) 65.86%.

: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 #128 +/- ## ===================================================== - Coverage 65.86% 65.86% -0.01% ===================================================== Files 32 32 Lines 59481 59481 Branches 15708 15708 ===================================================== - Hits 39178 39176 -2 - Misses 14282 14284 +2 Partials 6021 6021 ```

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

GitMensch commented 7 months ago

There are two generated Makefiles added in this PR, those should be dropped.

The changes in run_misc.at are ready for svn, not sure about the other as we explicit adjust variables to let the C compiler work fine, and this call is dropped by COMPILE_ONLY.

ddeclerck commented 6 months ago

There are two generated Makefiles added in this PR, those should be dropped.

Indeed, I guess I committed too quickly.

The changes in run_misc.at are ready for svn, not sure about the other as we explicit adjust variables to let the C compiler work fine, and this call is dropped by COMPILE_ONLY.

Well, GCC seems to expect TMPDIR to point to some valid location. Under those circumstances, shouldn't an invalid TMPDIR be considered as an error instead of a mere warning ?

GitMensch commented 6 months ago

At least with my installation setting TMPDIR=/banana doesn't raise any error with GCC.

ddeclerck commented 6 months ago

Using GCC 13.2 under MSYS2/UCRT, seems it in fact only considers the TMP variable (while GCC docs only mentions TMPDIR). TMP=/banana gcc prog.c systematically fails for me (works fine under Linux though).

GitMensch commented 6 months ago

If TMP is unset, then it uses TEMP, if that's unset then it may use the userprofile...

I'm OK in general to adjust GnuCOBOL's behavior but a broken TMPDIR, but I'm not sure if aborting in common.c would be good...

Note that the real issue is that the changed variable is not correctly exported, this can break a bunch of things for other tests (in the future), too. Ideally we can tell the runtime environment (which seems to be some MSYS here) to correctly export this variable (and in the future: others).

ddeclerck commented 6 months ago

Note that the real issue is that the changed variable is not correctly exported, this can break a bunch of things for other tests (in the future), too. Ideally we can tell the runtime environment (which seems to be some MSYS here) to correctly export this variable (and in the future: others).

I'm not sure to understand. To you suggest this is a bug in the MSYS shell ?

GitMensch commented 6 months ago

"Not a bug but a feature" - several environment variables are converted between Windows and Posix path, some are just not taken over (and it seems that some variables will have the values which they had when the MSYS process was started).

Environment variables are also related C runtime used; when it "switches" you may see "old" values; though this is mostly an issue within a single process, calling a new one normally provides a "clear start". If you want to see that then you may be able to check the chain of processes and the environment variables set, for example with process explorer (would need the compiler process to "wait" instead of end, but maybe one could just create a gcc.cmd and have that in PATH before gcc, which then does a set /p var=starting... before the gcc (by full path) call.

Given that - I'm not sure how this should be tackled - as noted an error may be fine, but in this case cobc would have to pass the instruction "abort on error" (maybe a separate entry function?) when getting the path by libcob (or duplicate that code into cobc and adjust it there).

ddeclerck commented 6 months ago

@GitMensch Should I merge into svn the fixes for tests 808/809 (run_misc.at) now - leaving the fix for test 8 (used_binaries.at) for later ?

GitMensch commented 6 months ago

If that's not too much to ask: yes, please do, this should get the false positives down to one.

Thank you!

ddeclerck commented 6 months ago

Done !

GitMensch commented 5 months ago

Can you please rebase and adjust the title?

ddeclerck commented 5 months ago

That's been done.