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

Don't disable dynlink in -static switches #14664

Merged
merged 2 commits into from
Sep 13, 2019
Merged

Don't disable dynlink in -static switches #14664

merged 2 commits into from
Sep 13, 2019

Conversation

copy
Copy link
Contributor

@copy copy commented Aug 10, 2019

This removes the -no-shared-libs flag from the configuration of static switches. Note that this switch doesn't produce static binaries by default, only when -ccopt -static is passed to ocamlopt.

This fixes #14663 and backtracking/ocamlgraph#80 (verified by running opam install ppx_deriving ocamlgraph). It's also related to #3608.

One downside of this change is that it replace a compile-time error (Dynlink isn't built when -no-shared-libs is configured) with a run-time error (Failure "Dynamic loading not supported"). This only affects executables compiled with -ccopt -static.

cc @etanol

@camelus
Copy link
Contributor

camelus commented Aug 10, 2019

🌤️ opam-lint warnings 124dc1f
  • ocaml-variants.4.01.0+musl+static has some warnings:

    • warning 25: Missing field 'authors'
    • warning 35: Missing field 'homepage'
    • warning 36: Missing field 'bug-reports'
    • warning 37: Missing field 'dev-repo'
  • ocaml-variants.4.02.1+musl+static has some warnings:

    • warning 25: Missing field 'authors'
    • warning 35: Missing field 'homepage'
    • warning 36: Missing field 'bug-reports'
    • warning 37: Missing field 'dev-repo'
  • ocaml-variants.4.02.3+musl+static has some warnings:

    • warning 25: Missing field 'authors'
    • warning 35: Missing field 'homepage'
    • warning 36: Missing field 'bug-reports'
    • warning 37: Missing field 'dev-repo'
  • ocaml-variants.4.05.0+musl+flambda has some warnings:

    • warning 25: Missing field 'authors'
    • warning 35: Missing field 'homepage'
    • warning 36: Missing field 'bug-reports'
    • warning 37: Missing field 'dev-repo'
  • ocaml-variants.4.05.0+musl+static+flambda has some warnings:

    • warning 25: Missing field 'authors'
    • warning 35: Missing field 'homepage'
    • warning 36: Missing field 'bug-reports'
    • warning 37: Missing field 'dev-repo'
  • ocaml-variants.4.06.0+musl+flambda has some warnings:

    • warning 25: Missing field 'authors'
    • warning 35: Missing field 'homepage'
    • warning 36: Missing field 'bug-reports'
    • warning 37: Missing field 'dev-repo'
  • ocaml-variants.4.06.0+musl+static+flambda has some warnings:

    • warning 25: Missing field 'authors'
    • warning 35: Missing field 'homepage'
    • warning 36: Missing field 'bug-reports'
    • warning 37: Missing field 'dev-repo'
  • ocaml-variants.4.06.1+musl+flambda has some warnings:

    • warning 25: Missing field 'authors'
    • warning 35: Missing field 'homepage'
    • warning 36: Missing field 'bug-reports'
    • warning 37: Missing field 'dev-repo'
  • ocaml-variants.4.06.1+musl+static+flambda has some warnings:

    • warning 25: Missing field 'authors'
    • warning 35: Missing field 'homepage'
    • warning 36: Missing field 'bug-reports'
    • warning 37: Missing field 'dev-repo'
  • ocaml-variants.4.07.1+musl+flambda has some warnings:

    • warning 25: Missing field 'authors'
    • warning 35: Missing field 'homepage'
    • warning 36: Missing field 'bug-reports'
    • warning 37: Missing field 'dev-repo'
  • ocaml-variants.4.07.1+musl+static+flambda has some warnings:

    • warning 25: Missing field 'authors'
    • warning 35: Missing field 'homepage'
    • warning 36: Missing field 'bug-reports'
    • warning 37: Missing field 'dev-repo'
  • These packages passed lint tests: ocaml-variants.4.08.0+musl+flambda, ocaml-variants.4.08.0+musl+static+flambda, ocaml-variants.4.08.1+musl+flambda, ocaml-variants.4.08.1+musl+static+flambda


☀️ Installability check (+6)
  • new installable packages (6): ocaml-variants.4.05.0+musl+flambda ocaml-variants.4.06.0+musl+flambda ocaml-variants.4.06.1+musl+flambda ocaml-variants.4.07.1+musl+flambda ocaml-variants.4.08.0+musl+flambda ocaml-variants.4.08.1+musl+flambda

@etanol
Copy link
Contributor

etanol commented Aug 10, 2019

Thanks for this, and apologies for not taking a look sooner.

One downside of this change is that it replace a compile-time error (Dynlink isn't built when -no-shared-libs is configured) with a run-time error (Failure "Dynamic loading not supported"). This only affects executables compiled with -ccopt -static.

Also, thanks for the explanation. I had been confused a bit about musl's dynamic loading support.

I didn't notice that musl has two dlopen implementations:

  1. The dummy one, used in statically linked musl
  2. The one from the dynamic linker/loader, for dynamically linked executables compiled with musl-gcc

For some reason, I understood that musl did not support dynamic loading at all. But I was obviously wrong.

In summary, these changes are perfectly fine. But they raise a different question. Since these variants now allow for both dynamic and static linking against musl libc, shouldn't we remove the +static+ tag from the name? It looks confusing.

@ozanmakes
Copy link

Is this switch intended for distributions that ship with GCC by default? I've been successfully using <version>+flambda to build statically linked binaries on Alpine Linux without using this switch. I thought I had observed smaller binaries with +musl+static+flambda, but now I can not reproduce it.

Also, this switch does not build on Alpine Linux as it is, since Alpine does not have a musl-gcc binary, and instead comes with gcc and x86_64-alpine-linux-musl-gcc.

@kit-ty-kate
Copy link
Member

For what it's worth, the original ocaml-variants package included both with +static and without +static: #2025

@etanol
Copy link
Contributor

etanol commented Aug 16, 2019

@kit-ty-kate good point. Plus the musl variant could be identical to the standard one when compiled under Apline Linux.

@copy
Copy link
Contributor Author

copy commented Aug 17, 2019

@osener That's correct, these switches are intended for static compilation on platforms that use glibc by default, but allow you to install a musl toolchain separately (Debian, Arch Linux, …). On Alpine, you can continue using a regular switch. The size difference might have been due to the -Os flag that this switch adds by default, although I believe it's overwritten by most build systems anyway.

Since these variants now allow for both dynamic and static linking against musl libc, shouldn't we remove the +static+ tag from the name? It looks confusing.

For what it's worth, the original ocaml-variants package included both with +static and without +static

I'm a bit wary about removing switches that people might be using. I'll leave the old switches with the fix and add new +musl switches. The last remaining difference (from the original MR) between the +musl+static and the +musl switches is then that in the former, the OCaml compiler itself is compiled statically.

These switches can be still be used to statically link, but don't
statically link on the OCaml compiler itself.
@copy
Copy link
Contributor Author

copy commented Aug 17, 2019

Something else I found: Starting in 4.09, you will be able to create a musl switch simply by setting CC at switch creation time: CC=musl-gcc opam switch create test 4.09.0+beta1.

Previously this didn't work unless you had X11 header files in your musl toolchain to compile graphslib. graphs was moved out of the compiler recently, fixing this issue.

@copy
Copy link
Contributor Author

copy commented Sep 2, 2019

This should be good to merge from my point of view. Any objections/questions?

@etanol
Copy link
Contributor

etanol commented Sep 3, 2019

No objection. I think the static tag is a bit of a lie currently, because the generated OCaml compiler binaries are still dynamically linked. But that would be a different issue.

@mseri
Copy link
Member

mseri commented Sep 13, 2019

I think this cleans up confusion, I am going to merge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ppx_deriving cannot be installed in the -static switches
6 participants