Fix testsuite 8 on Windows/MSYS2 #128

ddeclerck · 2023-12-11T12:38:52Z

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.

GitMensch

Thank you for tackling this.
Adjusting a "known format" is reasonable here (but at least above the first one: please add a note about the MSYS behavior of calling with full-path, which we need to remove here.

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

tests/testsuite.src/run_misc.at

tests/testsuite.src/used_binaries.at

ddeclerck · 2023-12-11T16:50:13Z

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 ?

GitMensch · 2023-12-11T17:00:36Z

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 · 2023-12-12T09:30:06Z

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

codecov-commenter · 2023-12-12T09:31:10Z

Codecov Report

All modified and coverable lines are covered by tests ✅

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

❗ 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

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

GitMensch · 2023-12-30T18:56:16Z

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 · 2024-01-09T15:08:27Z

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 · 2024-01-09T15:15:11Z

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

ddeclerck · 2024-01-09T15:28:32Z

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 · 2024-01-09T16:02:27Z

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 · 2024-01-10T12:52:01Z

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 · 2024-01-10T14:01:07Z

"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 · 2024-01-22T15:10:10Z

@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 · 2024-01-22T15:24:53Z

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

ddeclerck · 2024-01-22T15:56:17Z

Done !

GitMensch · 2024-02-20T14:55:52Z

Can you please rebase and adjust the title?

ddeclerck · 2024-02-20T15:09:06Z

That's been done.

GitMensch · 2024-04-20T19:57:32Z

tests/testsuite.src/used_binaries.at

+AT_CHECK([$COMPILE prog.cob], [0], [], [])
 AT_CHECK([$COBCRUN_DIRECT ./prog], [0], [OK], [])


The only point of testing the execution was to verify that this does work - without the recompile, because we adjust the broken environment before calling the C compiler.

The goal is good, but if we don't find a way to ensure that cobc's setenv reaches the program envrionment of the called C compiler, I'd rather let this fail "from time to time" instead of globally accepting a failure here.

I'd suggest to move this to a draft until someone finds the way to tweak this "correctly" (I guess that means setting some environment variable; it would likely be reasonable to ask the MSYS2 folks (possibly by creating an issue in their tracker - if this happens under MSYS2).

ddeclerck requested a review from GitMensch December 11, 2023 15:57

GitMensch requested changes Dec 11, 2023

View reviewed changes

tests/testsuite.src/run_misc.at Outdated Show resolved Hide resolved

tests/testsuite.src/run_misc.at Outdated Show resolved Hide resolved

tests/testsuite.src/run_misc.at Outdated Show resolved Hide resolved

tests/testsuite.src/used_binaries.at Show resolved Hide resolved

GitMensch linked an issue Dec 11, 2023 that may be closed by this pull request

Test 809 fails on Windows #123

Open

ddeclerck force-pushed the fix_win_tests branch from f232f15 to e506ac9 Compare December 12, 2023 09:28

ddeclerck linked an issue Dec 12, 2023 that may be closed by this pull request

Test 8 fails on Windows #124

Open

ddeclerck force-pushed the fix_win_tests branch from e506ac9 to 3327e68 Compare January 18, 2024 21:12

Fix testsuite 8, 808 and 809 on Windows/MSYS2

b951376

ddeclerck force-pushed the fix_win_tests branch from 3327e68 to b951376 Compare February 20, 2024 15:07

ddeclerck changed the title ~~Fix testsuite 8, 808 and 809 on Windows/MSYS2~~ Fix testsuite 8 Feb 20, 2024

ddeclerck changed the title ~~Fix testsuite 8~~ Fix testsuite 8 on Windows/MSYS2 Feb 20, 2024

GitMensch reviewed Apr 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix testsuite 8 on Windows/MSYS2 #128

Fix testsuite 8 on Windows/MSYS2 #128

ddeclerck commented Dec 11, 2023 •

edited

Loading

GitMensch left a comment

ddeclerck commented Dec 11, 2023

GitMensch commented Dec 11, 2023

ddeclerck commented Dec 12, 2023

codecov-commenter commented Dec 12, 2023 •

edited

Loading

GitMensch commented Dec 30, 2023

ddeclerck commented Jan 9, 2024 •

edited

Loading

GitMensch commented Jan 9, 2024

ddeclerck commented Jan 9, 2024

GitMensch commented Jan 9, 2024

ddeclerck commented Jan 10, 2024

GitMensch commented Jan 10, 2024

ddeclerck commented Jan 22, 2024

GitMensch commented Jan 22, 2024 via email

ddeclerck commented Jan 22, 2024

GitMensch commented Feb 20, 2024

ddeclerck commented Feb 20, 2024

GitMensch Apr 20, 2024

		AT_CHECK([$COMPILE prog.cob], [0], [], [])
		AT_CHECK([$COBCRUN_DIRECT ./prog], [0], [OK], [])

Fix testsuite 8 on Windows/MSYS2 #128

Are you sure you want to change the base?

Fix testsuite 8 on Windows/MSYS2 #128

Conversation

ddeclerck commented Dec 11, 2023 • edited Loading

GitMensch left a comment

Choose a reason for hiding this comment

ddeclerck commented Dec 11, 2023

GitMensch commented Dec 11, 2023

ddeclerck commented Dec 12, 2023

codecov-commenter commented Dec 12, 2023 • edited Loading

Codecov Report

GitMensch commented Dec 30, 2023

ddeclerck commented Jan 9, 2024 • edited Loading

GitMensch commented Jan 9, 2024

ddeclerck commented Jan 9, 2024

GitMensch commented Jan 9, 2024

ddeclerck commented Jan 10, 2024

GitMensch commented Jan 10, 2024

ddeclerck commented Jan 22, 2024

GitMensch commented Jan 22, 2024 via email

ddeclerck commented Jan 22, 2024

GitMensch commented Feb 20, 2024

ddeclerck commented Feb 20, 2024

GitMensch Apr 20, 2024

Choose a reason for hiding this comment

ddeclerck commented Dec 11, 2023 •

edited

Loading

codecov-commenter commented Dec 12, 2023 •

edited

Loading

ddeclerck commented Jan 9, 2024 •

edited

Loading