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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

reftest engine tweaks for Windows #5723

Merged
merged 3 commits into from Dec 7, 2023
Merged

Conversation

dra27
Copy link
Member

@dra27 dra27 commented Nov 9, 2023

On Windows, when run in GitHub Actions, the opam env in reftests is using a sh as the default shell (so we get ENV='value'; export ENV format). When run locally, I get cmd as the default shell, which causes conflicts as we get set ENV=value notation instead.

I had a deeper dive into this as part of looking at #5636 and #5714. Process hierarchy is a slightly strange thing on Windows - it's already the case that the ancestry returned gets "chopped" (process groups, etc. can do this). The ancestry being returned is not incorrect, or at least it is consistent - tools such as Process Explorer see the same thing.

For whatever reason, the GitHub Actions get Cygwin's sh.exe as one of the ancestor processes. Locally, it's make.exe for me 馃し I'm not totally sure why, but it doesn't particularly matter - the ancestry detection is correct when run from cmd or powershell, which is when it's needed. When native opam is run from a Cygwin shell, SHELL is set, and opam uses that.

So the only issue is the reftests, which don't propagate the SHELL environment variable. The first commit here rather than propagating it, simply sets it. We might consider doing this for Unix as well - I'm not sure whether running make tests from fish has the same problem. I think this could also be done with OPAMSHELL, but given that we weren't propagating the SHELL variable before, I don't think an issue with just setting it instead.

Also while looking at things for #5636, it's quite easy now to end up with the opam root or the temporary directory being translated to Cygwin notation which doesn't then get caught by the regex for conversion to BASEDIR or OPAMTMP. I've factored out the code which was doing the previous forward/backslash manipulation and exposed OpamSystem.apply_cygpath so now those directories are captured with backslashes, forward-slashes or in Cygwin notation.

@rjbou
Copy link
Collaborator

rjbou commented Nov 10, 2023

fixes #5377

Copy link
Collaborator

@rjbou rjbou left a comment

Choose a reason for hiding this comment

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

thanks!
I modified apply_cygpath comment, lgtm!

@rjbou rjbou merged commit db79051 into ocaml:master Dec 7, 2023
29 checks passed
Out of release : doc, test, install, etc. automation moved this from PR to done Dec 7, 2023
@dra27 dra27 deleted the reftest-windows branch December 22, 2023 10:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

2 participants