gnucobol icon indicating copy to clipboard operation
gnucobol copied to clipboard

Fix testsuite 8 on Windows/MSYS2

Open ddeclerck opened this issue 1 year ago • 16 comments

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 avatar Dec 11 '23 12:12 ddeclerck

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

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 ?

ddeclerck avatar Dec 11 '23 16:12 ddeclerck

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".

GitMensch avatar Dec 11 '23 17:12 GitMensch

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

ddeclerck avatar Dec 12 '23 09:12 ddeclerck

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
@@                  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.

codecov-commenter avatar Dec 12 '23 09:12 codecov-commenter

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.

GitMensch avatar Dec 30 '23 18:12 GitMensch

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 ?

ddeclerck avatar Jan 09 '24 15:01 ddeclerck

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

GitMensch avatar Jan 09 '24 15:01 GitMensch

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).

ddeclerck avatar Jan 09 '24 15:01 ddeclerck

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).

GitMensch avatar Jan 09 '24 16:01 GitMensch

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 ?

ddeclerck avatar Jan 10 '24 12:01 ddeclerck

"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).

GitMensch avatar Jan 10 '24 14:01 GitMensch

@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 ?

ddeclerck avatar Jan 22 '24 15:01 ddeclerck

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

Thank you!

GitMensch avatar Jan 22 '24 15:01 GitMensch

Done !

ddeclerck avatar Jan 22 '24 15:01 ddeclerck

Can you please rebase and adjust the title?

GitMensch avatar Feb 20 '24 14:02 GitMensch

That's been done.

ddeclerck avatar Feb 20 '24 15:02 ddeclerck