-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Only check that flexlink can be executed when building on a native windows machine. #12992
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
Conversation
ed529e7 to
ce99e5a
Compare
|
Am I correct that this is being done in order to ensure that executable is being seen, rather than a shell script? (I was looking in particular at https://github.com/ocaml-cross/opam-cross-windows/tree/main/packages/flexdll-windows/flexdll-windows.0.42/files). If that is the case, can this be fixed by instead always calling (I don't mind the change as-is; I'm just trying to understand it more) |
I can only speak for my use-case. I think calling |
|
If that works, I think it's a little bit cleaner/clearer - perhaps just with a comment that it's done to ensure that it calls an executable and not any wrapper? |
|
(I also can't see that it breaks any native system - Cygwin installs executables with |
Ok, program check switched to |
|
I had managed to read two lines of the original diff the wrong way around - 38559e1 is not necessary (sorry!). I fully see what you were doing originally - the only issue is that it's not the correct value for There is a possible alternative: the test itself is trying to be sure that the |
|
The TL;DR is that I think this should be sufficient: diff --git a/configure.ac b/configure.ac
index 1016470045..3ff128ff2d 100644
--- a/configure.ac
+++ b/configure.ac
@@ -970,8 +970,11 @@ AS_IF([test x"$supports_shared_libraries" != 'xfalse'], [
OCAML_TEST_FLEXLINK([$flexlink], [$flexdll_chain],
[$internal_cppflags], [$host])
- AS_CASE([$host],
- [*-w64-mingw32*|*-pc-windows],
+ # When building on Cygwin/MSYS2, flexlink may be a shell script which
+ # then cannot be executed by ocamlc/ocamlopt. Having located flexlink,
+ # ensure it can be executed from a native Windows process.
+ AS_CASE([$build],
+ [*-pc-msys|*-pc-cygwin*],
[flexlink_where="$(cmd /c "$flexlink" -where 2>/dev/null)"
AS_IF([test -z "$flexlink_where"],
[AC_MSG_ERROR(m4_normalize([$flexlink is not executable from a native |
8eeee5a to
1605273
Compare
Works for me! Changes applied. |
1605273 to
dfe3ee9
Compare
|
Excellent! As ever, thank you for continuing to push these changes up towards us. @shindere - please could you have a quick double-check of this and if you're happy, squash it? |
|
Thanks for the careful review! |
10ff5dc to
37e761a
Compare
|
this all looks very good to me yes, many thanks!
I particularly appreciated the comment that has been added to
`configure.ac`, which I found very clear.
I took the liberty to force-push a squashed version to the PR branch
because squashing is easier forme that way than from the web interface,
I hope nobody minds but please feel free to let me know, should that be
the case.
|
I'm in the process of updating https://github.com/ocaml-cross/opam-cross-windows/ to support OCaml
5.1.1and there are very little changes toconfigurerequired now! Pretty exciting!This one of two changes:
flexlinkneeds to be called as a shell command, not a windows command when cross-compiling.I took the approach of assuming that the
cmd /cprefix needs to be added only when the build host is windows. That seems like a logical decision to me.