Skip to content

Conversation

@kit-ty-kate
Copy link
Member

@kit-ty-kate kit-ty-kate commented Mar 12, 2025

The added set -e is here to harden the script and make sure such failures are caught early. Otherwise the script will simply ignore failing commands and carry on.

@kit-ty-kate kit-ty-kate force-pushed the restore-posix-compat branch from 04a44cf to 8e0ff16 Compare March 12, 2025 15:31
@gerdstolpmann
Copy link
Contributor

find not POSIX? What a crap, it's here: https://pubs.opengroup.org/onlinepubs/9799919799/utilities/find.html (POSIX-1.2024)

I wonder why we have to consider ancient implementations like MSYS2 that never got updated. If you want a working build system on Windows, run a Linux shell. It's all there.

@kit-ty-kate kit-ty-kate changed the title Restore POSIX compatibility of the configure script Make the configure script fail on command failures Mar 12, 2025
@kit-ty-kate kit-ty-kate force-pushed the restore-posix-compat branch from 8e0ff16 to 0211a88 Compare March 12, 2025 16:25
@kit-ty-kate
Copy link
Member Author

Arf sorry my bad, i worked from (flawed) memory and didn't bother to double-check what i was saying. Sorry again.

I removed that change, however the first commit about hardening the script against failures would be greatly appreciated. I think what happened with the failure on msys2 (from someone i was helping get things working) was that PATH was badly configured and find ended up being find.exe from Windows instead of find from msys2 and thus failed. However the lack of set -e made it fail silently and ended up not installing the META file for findlib which gave the impression of a working ocamlfind install but broke packages requiring that findlib library.

@dra27
Copy link
Member

dra27 commented Apr 2, 2025

Out of curiosity to check what else may be going on, where did the problem originally arise? find is part of findutils on both MSYS2 and Cygwin and, more importantly, it's a dependency of Base - i.e. it should always be installed.

It sounds like the original issue was a user's PATH not being correctly set, but that may be worth checking further?

@kit-ty-kate
Copy link
Member Author

It sounds like the original issue was a user's PATH not being correctly set, but that may be worth checking further?

yes, i believe the user had their MSYS2 path after C:\Windows\system32 in their PATH
Is opam not reordering PATH when choosing a POSIX layer other than the builtin cygwin? Thinking about it, it might be a worthwhile extra check to have during opam init if we don't.

However i think this PR is desirable regardless of what opam does

'find -exec cmd {} ;' will call cmd for each file, however if the command has a non-zero exit code find will simply ignore the error code and continue as if nothing happened. Piping into read is much simply at the cost of not supporting filenames containing \n but ocamlfind doesn't have any and extremely unlikely thing to happen so this is safe to do.
@kit-ty-kate
Copy link
Member Author

kit-ty-kate commented Apr 5, 2025

I had to debug yet another issue where ocamlfind simply ignored a segfault and continued the installation as if nothing happened resulting in a broken installation.

This time the problem of the user was that they were using dash master as their main POSIX shell but it happens to have a serious bug which makes sh -c 'echo test >> "${1%.in}"' sh src/findlib/META.in segfault (reported upstream here).
However find -exec cmd {} ; simply ignores failures of cmd and just carries on (per its POSIX spec). I've added a commit to fail early which would've reduced the time to debug this issue by several days and would've been less frustrating for the user and maintainers of the downstream projects where the error appeared (since the ocamlfind installed in a broken state). I've detailed the choice of replacement in the commit message.

@kit-ty-kate
Copy link
Member Author

Any chance this could be merged? Users keep hitting this issue

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.

3 participants