Skip to content

Conversation

@toots
Copy link
Contributor

@toots toots commented Feb 25, 2024

I'm in the process of updating https://github.com/ocaml-cross/opam-cross-windows/ to support OCaml 5.1.1 and there are very little changes to configure required now! Pretty exciting!

This one of two changes: flexlink needs to be called as a shell command, not a windows command when cross-compiling.

I took the approach of assuming that the cmd /c prefix needs to be added only when the build host is windows. That seems like a logical decision to me.

@gasche
Copy link
Member

gasche commented Feb 25, 2024

(This is for @dra27, @MisterDA)

@dra27
Copy link
Member

dra27 commented Feb 28, 2024

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 flexlink.exe -where in configure.ac?

(I don't mind the change as-is; I'm just trying to understand it more)

@toots
Copy link
Contributor Author

toots commented Feb 28, 2024

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 flexlink.exe -where in configure.ac?

(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 flexlink.exe would work for us.

@dra27
Copy link
Member

dra27 commented Feb 28, 2024

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?

@dra27
Copy link
Member

dra27 commented Feb 28, 2024

(I also can't see that it breaks any native system - Cygwin installs executables with .exe for maximal interop)

@toots
Copy link
Contributor Author

toots commented Feb 28, 2024

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?

Ok, program check switched to flexlink.exe, call switched to native call unconditionally.

@dra27
Copy link
Member

dra27 commented Feb 28, 2024

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 $build... when building on native Windows, we're building with Cygwin or MSYS2, so the regex should instead be *-pc-msys|*-pc-cygwin*.

There is a possible alternative: the test itself is trying to be sure that the flexlink command which configure has seen (which, especially in Cygwin, might be a Cygwin-specific shebang script) can be executed by the native-Windows compiler which is about to produced. In theory, we could therefore do this cmd /c test when $build != $host, but I think looking at one of the logs you posted that opam-windows-cross still has to configure OCaml with $host = $target = x86_64-w64-mingw64, so this doesn't (yet) work.

@dra27
Copy link
Member

dra27 commented Feb 28, 2024

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

@toots toots force-pushed the native-cmd-mingw branch 2 times, most recently from 8eeee5a to 1605273 Compare February 28, 2024 20:12
@toots
Copy link
Contributor Author

toots commented Feb 28, 2024

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

Works for me! Changes applied.

@toots toots changed the title Call flexlink natively when build machine is not windows. Only check that flexlink can be executed when building on a native windows machine. Feb 28, 2024
@dra27
Copy link
Member

dra27 commented Feb 28, 2024

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?

@toots
Copy link
Contributor Author

toots commented Feb 28, 2024

Thanks for the careful review!

@shindere
Copy link
Contributor

shindere commented Feb 29, 2024 via email

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.

4 participants