Fix -no-shared-libs support#2160
Conversation
Detail added to the printed output in GPR#1691, but not exposed from the module itself.
Changes in GPR#1063 weren't applied to nodynlink.ml
In -custom mode, then bytecode executable is first made in /tmp and this filename leaked if the executable fails the -compat-32 test.
ocamltest links bytecode executables in custom-runtime mode if they have C files, but it should also do this for any .cma files which require C stubs on platforms which don't support dynamic C libraries.
If the toplevel doesn't support shared libraries, skip any toplevel tests which will load cma files which have C stubs (e.g. unix.cma). The present implementation is suboptimal - really, it would be better to create a custom toplevel, but it only affects one test at the moment.
ocamltest/ocaml_actions.ml
Outdated
| what (String.concat " " commandline) exit_status) in | ||
| (Result.fail_with_reason reason, env) | ||
| end | ||
| end else (result, env) |
There was a problem hiding this comment.
The code of the else branch has just been updated for indentation
There was a problem hiding this comment.
(oh, and to use the backend variable declared earlier)
f6a30c4 to
66417aa
Compare
|
(I ran another precheck job: https://ci.inria.fr/ocaml/job/precheck/168/ . I'm afraid there is not much else I can do to help. ) |
66417aa to
a397b18
Compare
|
Thanks @gasche! Perhaps @mshinwell can cast an eye over 4190b3b quickly? a813fe7 just adds The remainder of the commits affect the testsuite only, so if @shindere has time to review then great, but equally the changes only affect the tests and do allow us to get CI fully working again. |
|
@dra27 yes yes I will review this, it's on my list.
Just a bit overwhelmed these days but will try to do it ASAP.
If you feel I am taking too much time for your taste, don't feel blocked
and just go ahead, I can also review the testsuite part after merge and
submit another PR, at least as far as I am concerned that would be fine
too.
|
ocamltest/ocaml_actions.ml
Outdated
| close_in ic; | ||
| List.compare_length_with toc.Cmo_format.lib_dllibs 0 <> 0 | ||
| else | ||
| assert false (* How have non-cma files ended up on the libraries? *) |
There was a problem hiding this comment.
A corrupted or truncated cma file would trigger this assertion. Better to output a nice error message. Or maybe just ignore the file: I presume it will be passed to the compiler, which will report the error to the user.
There was a problem hiding this comment.
I've put the fix for this in a separate commit as it'll want reviewing
ocamltest/ocaml_actions.ml
Outdated
| what (String.concat " " commandline) exit_status) in | ||
| (Result.fail_with_reason reason, env) | ||
| end | ||
| end else (result, env) |
| let iter_initial_units _ = () | ||
| let num_globals_inited = not_available | ||
|
|
||
| let fold_initial_units ~init ~f:_ = not_available init |
There was a problem hiding this comment.
The only functional difference with @mshinwell's patch is that adapt_filename, num_globals_inited and fold_initial_units raise Failure here instead of returning dummy values. Unless Mark has an objection, I think that's OK.
There was a problem hiding this comment.
I don't follow the "Also GitHub UI being unclear...libraries." comment?
|
@dra27 do you need some help with that one? Let me know if you'd like me to propose a rebase |
|
Sorry for the lag - I have this rebased and fixed the relevant parts of |
|
No problem, David. Let me know if there's anything I can help with.
|
The --disable-shared option is not presently possible on the native Windows ports, though this isn't (yet) enforced by configure.
a397b18 to
b8bc678
Compare
4c95820 to
27def61
Compare
|
Tsk, wrong branch in recheck. Now testing the correct code in precheck#179. @shindere / @damiendoligez - has something happened to the mingw32 worker? It was missing the |
27def61 to
c0040e1
Compare
| # Allow the MSVC linker to be found even if ld isn't installed. | ||
| # User-specified LD still takes precedence. | ||
| AC_CHECK_TOOLS([LD],[ld link]) | ||
| # libtool expects host_os=mingw for native Windows |
There was a problem hiding this comment.
Since the autoconf, I've allowed getting --disable-shared to work as expected to form part of this GPR. That has also expanded on a problem @shindere identified that --enable-shared doesn't work on MSVC (it does work for Cygwin and the mingw-w64 compilers). I did some in-depth poking into the code generated by LT_INIT and determined that the problem is that autoconf appears to assume that "native Windows" will be an OS called mingw - there is code in LT_INIT which will happily detect the output of cl, but it can't work with $host_os = windows.
This is making me wonder if it would be better for the MSVC ports to be identified not by pc-windows but by pc-mingw32 or cl-mingw32 or msvc-mingw32 - i.e. use a different part of the triple to identify it? I don't know - and haven't yet made any effort to find out - what other projects have done about this, but it seems worth determining this now, before i686-pc-windows becomes the only way of identifying MSVC.
Thoughts @shindere?
There was a problem hiding this comment.
That sounds fishy. Is autoconf really that ill-adapted to MSVC toolchains? What is $host_os when we run on cygwin?
There was a problem hiding this comment.
It's not quite the right question - autoconf has reasonable support for cl as a compiler, but it does expect a Unix-like OS to be running on ... i.e. it expects the OS to be either cygwin or mingw (they're just the only two it's aware of). So it's objecting to (or rather, ignoring!) windows as the declared OS.
|
We fixed the mingw-32 worker, it was just missing the flexdll directory in its PATH. |
|
Damien Doligez (2019/01/22 06:22 -0800):
That sounds fishy. Is autoconf really that ill-adapted to MSVC
toolchains?
Yeah I think they didn't go that far in this direction, due to a lack of
resources to maintain that bit.
What is $host_os when we run on cygwin?
'cygwin' on both 32 and 64 bits ports.
|
|
David Allsopp (2019/01/15 04:25 -0800):
Tsk, wrong branch in recheck. Now testing the correct code in
[precheck#179](https://ci.inria.fr/ocaml/job/precheck/179/).
So, first of all, many thanks for the great work you did!
@shindere / @damiendoligez - has something happened to the mingw32
worker? It was missing the `OCAML_ARCH` variable, which I've restored
Thanks a lot for that! I am not sure why the variable was missing, but
as far as I remember, that slave had a problem so was destroyed and
re-created. Most likely, the person who re-created forgot to define the
variable.
but it also seems to be missing flexdll
As @damiendoligez explained, it was actually installed but not defined
in the PATH when the Jenkins job was started. This is now fixed.
(see
https://ci.inria.fr/ocaml/job/precheck/179/flambda=false,label=ocaml-mingw-32/console).
I have finally uploaded a public key to Inria CI, but I don't think I
have access to the workers at the moment?
No, indeed, you don't. I'll figure that out and send you a private
e-mail ASAP this works.
|
|
David Allsopp (2019/01/15 20:47 +0000):
dra27 commented on this pull request.
> @@ -365,7 +365,11 @@ AS_IF([test x"$enable_unix_lib" = "xno" -o x"$enable_str_lib" = "xno"],
# Allow the MSVC linker to be found even if ld isn't installed.
# User-specified LD still takes precedence.
AC_CHECK_TOOLS([LD],[ld link])
+# libtool expects host_os=mingw for native Windows
Since the autoconf, I've allowed getting `--disable-shared` to work as
expected to form part of this GPR. That has also expanded on a problem
@shindere identified that `--enable-shared` doesn't work on MSVC (it
does work for Cygwin and the mingw-w64 compilers). I did some in-depth
poking into the code generated by `LT_INIT` and determined that the
problem is that autoconf appears to assume that "native Windows" will
be an OS called mingw - there is code in `LT_INIT` which will happily
detect the output of `cl`, but it can't work with `$host_os` =
`windows`.
Well, many thanks for having clarified this!
This is making me wonder if it would be better for the MSVC ports to
be identified not by `pc-windows` but by `pc-mingw32` or `cl-mingw32`
or `msvc-mingw32` - i.e. use a different part of the triple to
identify it? I don't know - and haven't yet made any effort to find
out - what other projects have done about this, but it seems worth
determining this now, before `i686-pc-windows` becomes the only way of
identifying MSVC.
@damiendoligez and I have discussed the matter and, at the moment, we
tend to prefer the names we currently have, because we find them
clearer. Also, historically speaking, I started to use libtool because
there were things configure couldn't guess on its own, even on Unix
systems. But then, for Windows, I found myself fixing many of libtool's
guesses to make them fix what we had before. So, recently, I came to
wonder which of libtool's guesses we are still actually using on
Windows. So it's not even obvious to me we should even call LT_INIT on
Windows, in the long run, and that probably will have to be figured out.
All that being said, I don't have storng opinions on this so if you do,
@dra27, there is definitely room for discussion.
|
|
@dra27 Is this good to merge now? I'm waiting for this before I do the beta. |
|
I think my only question was whether we wanted to rename the osname for Windows ... as long as we're happy with the "hack" at the moment for |
|
Perfect, thanks! Should I squash the commits or do you think it's better to keep the history? |
|
Could we keep the history, just for bisection purposes? |
|
Merged, thanks! |
|
Has it been cherry-picked? (not sure from GH interface if it's displaying 4.08 commits in the wrong order, or if they're missing...) |
While trying to get Cygwin64 CI to do something useful, I came across various
-no-shared-libslimitations.nodynlink.mlfile, which is done hereutils/config.mli, which is now done hereocamlc -compat-32when running with-customis included (the name of the temporary bytecode executable in/tmpwould be displayed instead of the final executable name)no-shared-librariesocamltest action. It would also be possible to do something more intelligent inocamlrunhere (i.e. haveocamlrun ./something-compiled-with-customrealise that it doesn't need interpreting)-customif any of the libraries (e.g.unix.cma) have C stubs. This is done by querying the cma files directly.unboxed-primitive-argstest, I opted just to skip it.