Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix testsuite 8 on Windows/MSYS2 #128

Closed

Conversation

ddeclerck
Copy link
Contributor

@ddeclerck ddeclerck commented Dec 11, 2023

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.

Copy link
Collaborator

@GitMensch GitMensch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 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 GitMensch linked an issue Dec 11, 2023 that may be closed by this pull request
@ddeclerck
Copy link
Contributor Author

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
Copy link
Collaborator

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
Copy link
Contributor Author

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

@codecov-commenter
Copy link

codecov-commenter commented Dec 12, 2023

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.

@ddeclerck ddeclerck linked an issue Dec 12, 2023 that may be closed by this pull request
@GitMensch
Copy link
Collaborator

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
Copy link
Contributor Author

ddeclerck commented Jan 9, 2024

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
Copy link
Collaborator

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

@ddeclerck
Copy link
Contributor Author

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
Copy link
Collaborator

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
Copy link
Contributor Author

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
Copy link
Collaborator

"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
Copy link
Contributor Author

@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
Copy link
Collaborator

GitMensch commented Jan 22, 2024 via email

@ddeclerck
Copy link
Contributor Author

Done !

@GitMensch
Copy link
Collaborator

Can you please rebase and adjust the title?

@ddeclerck ddeclerck changed the title Fix testsuite 8, 808 and 809 on Windows/MSYS2 Fix testsuite 8 Feb 20, 2024
@ddeclerck ddeclerck changed the title Fix testsuite 8 Fix testsuite 8 on Windows/MSYS2 Feb 20, 2024
@ddeclerck
Copy link
Contributor Author

That's been done.

Comment on lines +406 to 407
AT_CHECK([$COMPILE prog.cob], [0], [], [])
AT_CHECK([$COBCRUN_DIRECT ./prog], [0], [OK], [])
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ddeclerck closed this Oct 4, 2024
@ddeclerck ddeclerck deleted the fix_win_tests branch October 4, 2024 11:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Test 8 fails on Windows Test 809 fails on Windows
3 participants