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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix -no-shared-libs support #2160

Merged
merged 10 commits into from Feb 1, 2019

Conversation

Projects
None yet
5 participants
@dra27
Copy link
Contributor

commented Nov 23, 2018

While trying to get Cygwin64 CI to do something useful, I came across various -no-shared-libs limitations.

  • #1063 didn't update the nodynlink.ml file, which is done here
  • #1691 failed to update utils/config.mli, which is now done here
  • A small bug in the error message for ocamlc -compat-32 when running with -custom is included (the name of the temporary bytecode executable in /tmp would be displayed instead of the final executable name)
  • The caml-tex test requires a new no-shared-libraries ocamltest action. It would also be possible to do something more intelligent in ocamlrun here (i.e. have ocamlrun ./something-compiled-with-custom realise that it doesn't need interpreting)
  • When shared library support is not available, ocamltest should link with -custom if any of the libraries (e.g. unix.cma) have C stubs. This is done by querying the cma files directly.
  • A similar check is made for running the toplevel with a library. In theory, this could build a custom toplevel but as it only affects the unboxed-primitive-args test, I opted just to skip it.

dra27 added some commits Nov 22, 2018

Expose Config.supports_shared_libraries
Detail added to the printed output in GPR#1691, but not exposed from the
module itself.
Fix support for -no-shared-libs
Changes in GPR#1063 weren't applied to nodynlink.ml
Fix -compat-32 error message in -custom mode
In -custom mode, then bytecode executable is first made in /tmp and this
filename leaked if the executable fails the -compat-32 test.
Fix using -custom with -no-shared-libs
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.
Skip toplevel tests in -no-shared-libs
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.
what (String.concat " " commandline) exit_status) in
(Result.fail_with_reason reason, env)
end
end else (result, env)

This comment has been minimized.

Copy link
@dra27

dra27 Nov 23, 2018

Author Contributor

The code of the else branch has just been updated for indentation

This comment has been minimized.

Copy link
@dra27

dra27 Nov 23, 2018

Author Contributor

(oh, and to use the backend variable declared earlier)

This comment has been minimized.

Copy link
@damiendoligez

damiendoligez Dec 13, 2018

Member

Also libraries.

@dra27 dra27 force-pushed the dra27:fix-no-shared-libs branch from f6a30c4 to 66417aa Nov 28, 2018

@damiendoligez damiendoligez added this to the 4.08 milestone Dec 6, 2018

@dra27 dra27 referenced this pull request Dec 12, 2018

Closed

Fix "nodynlink" #2197

@damiendoligez damiendoligez added the bug label Dec 12, 2018

@gasche

This comment has been minimized.

Copy link
Member

commented Dec 12, 2018

(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. )

@dra27 dra27 force-pushed the dra27:fix-no-shared-libs branch from 66417aa to a397b18 Dec 13, 2018

@dra27

This comment has been minimized.

Copy link
Contributor Author

commented Dec 13, 2018

Thanks @gasche! Perhaps @mshinwell can cast an eye over 4190b3b quickly? a813fe7 just adds Config.supports_shared_libraries which should arguably have been there already, so just needs a quick nod.

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.

@mshinwell

This comment has been minimized.

Copy link
Contributor

commented Dec 13, 2018

@dra27 Could you just replace the nodynlink.ml changes with the contents of #2197?

@shindere

This comment has been minimized.

Copy link
Contributor

commented Dec 13, 2018

@damiendoligez
Copy link
Member

left a comment

I've reviewed the changes and I have only one real change to suggest (removing the assert false). This PR will be good to merge as soon as you deal with that.

Show resolved Hide resolved 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? *)

This comment has been minimized.

Copy link
@damiendoligez

damiendoligez Dec 13, 2018

Member

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.

This comment has been minimized.

Copy link
@dra27

dra27 Jan 15, 2019

Author Contributor

I've put the fix for this in a separate commit as it'll want reviewing

Show resolved Hide resolved ocamltest/ocaml_actions.ml
what (String.concat " " commandline) exit_status) in
(Result.fail_with_reason reason, env)
end
end else (result, env)

This comment has been minimized.

Copy link
@damiendoligez

damiendoligez Dec 13, 2018

Member

Also libraries.

let iter_initial_units _ = ()
let num_globals_inited = not_available

let fold_initial_units ~init ~f:_ = not_available init

This comment has been minimized.

Copy link
@damiendoligez

damiendoligez Dec 13, 2018

Member

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.

This comment has been minimized.

Copy link
@dra27

dra27 Jan 15, 2019

Author Contributor

I don't follow the "Also libraries." comment? GitHub UI being unclear...

Show resolved Hide resolved utils/config.mli
@shindere

This comment has been minimized.

Copy link
Contributor

commented Jan 9, 2019

@dra27 do you need some help with that one?

Let me know if you'd like me to propose a rebase

@dra27

This comment has been minimized.

Copy link
Contributor Author

commented Jan 14, 2019

Sorry for the lag - I have this rebased and fixed the relevant parts of configure.ac, but I have two tests which are unexpectedly failing (I think one will need a trivial update for Cygwin and the other is autoconf-related). I'll push a revised branch once I've fixed those two.

@shindere

This comment has been minimized.

Copy link
Contributor

commented Jan 14, 2019

dra27 added some commits Jan 15, 2019

Propagate --disable-shared correctly on Unix
The --disable-shared option is not presently possible on the native
Windows ports, though this isn't (yet) enforced by configure.

@dra27 dra27 force-pushed the dra27:fix-no-shared-libs branch from a397b18 to b8bc678 Jan 15, 2019

@dra27 dra27 force-pushed the dra27:fix-no-shared-libs branch 2 times, most recently from 4c95820 to 27def61 Jan 15, 2019

@dra27

This comment has been minimized.

Copy link
Contributor Author

commented Jan 15, 2019

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 OCAML_ARCH variable, which I've restored but it also seems to be missing flexdll (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?

@dra27 dra27 force-pushed the dra27:fix-no-shared-libs branch from 27def61 to c0040e1 Jan 15, 2019

@@ -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

This comment has been minimized.

Copy link
@dra27

dra27 Jan 15, 2019

Author Contributor

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?

This comment has been minimized.

Copy link
@damiendoligez

damiendoligez Jan 22, 2019

Member

That sounds fishy. Is autoconf really that ill-adapted to MSVC toolchains? What is $host_os when we run on cygwin?

This comment has been minimized.

Copy link
@dra27

dra27 Jan 22, 2019

Author Contributor

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.

@damiendoligez

This comment has been minimized.

Copy link
Member

commented Jan 22, 2019

We fixed the mingw-32 worker, it was just missing the flexdll directory in its PATH.

@shindere

This comment has been minimized.

Copy link
Contributor

commented Jan 22, 2019

@shindere

This comment has been minimized.

Copy link
Contributor

commented Jan 22, 2019

@shindere

This comment has been minimized.

Copy link
Contributor

commented Jan 22, 2019

@damiendoligez

This comment has been minimized.

Copy link
Member

commented Feb 1, 2019

@dra27 Is this good to merge now? I'm waiting for this before I do the beta.
I've relaunched the precheck as https://ci.inria.fr/ocaml/job/precheck/187/ after telling Jenkins to keep precheck builds a little longer...

@dra27

This comment has been minimized.

Copy link
Contributor Author

commented Feb 1, 2019

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 LT_INIT, I think this is good to go.

@damiendoligez

This comment has been minimized.

Copy link
Member

commented Feb 1, 2019

Perfect, thanks!

Should I squash the commits or do you think it's better to keep the history?

@dra27

This comment has been minimized.

Copy link
Contributor Author

commented Feb 1, 2019

Could we keep the history, just for bisection purposes?

@damiendoligez damiendoligez merged commit 23d582f into ocaml:trunk Feb 1, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@damiendoligez

This comment has been minimized.

Copy link
Member

commented Feb 1, 2019

Merged, thanks!

@dra27

This comment has been minimized.

Copy link
Contributor Author

commented Feb 2, 2019

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...)

@dra27 dra27 referenced this pull request Mar 1, 2019

Merged

4.08 version of GPR#2160 #2275

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.