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

pass LDFLAGS to ocamlmklib via -ldopt #124

Merged
merged 2 commits into from
Jan 2, 2023
Merged

Conversation

mroch
Copy link
Contributor

@mroch mroch commented Apr 27, 2022

ocamlmklib supports some but not all linker flags. we have some LDFLAGS that are not supported by ocamlmklib, so they need to be passed via -ldopt. this PR prepends -ldopt to each word in LIBS (which is set to LDFLAGS in ./configure).

`ocamlmklib` supports some but not all linker flags. we have some `LDFLAGS` that are not supported by `ocamlmklib`, so they need to be passed via `-ldopt`. this PR prepends `-ldopt` to each word in `LIBS` (which is set to `LDFLAGS` in ./configure).
@mroch
Copy link
Contributor Author

mroch commented Apr 27, 2022

specifically, our LDFLAGS cause errors like this:

Unknown option -Wl,-rpath-link,/foo/bar/glibc/2.30/lib
Unknown option -Wl,--hash-style,gnu

@antoinemine
Copy link
Collaborator

Thanks for the tentative fix. Two remarks:

  • would it be possible to write $(OCAMLMKLIB) $(DEBUG) -failsafe -o zarith $+ -lopt "$(LIBS)" in the Makefile ? That would be much simpler (no need for a new variable or a foreach)
  • if we keep using a new variable, avoid calling it LDFLAGS as it is already used for C compilers ; maybe use MLMKLIBFLAGS or another name

@mroch
Copy link
Contributor Author

mroch commented May 3, 2022

would it be possible to write $(OCAMLMKLIB) $(DEBUG) -failsafe -o zarith $+ -lopt "$(LIBS)" in the Makefile ? That would be much simpler (no need for a new variable or a foreach)

indeed, that works. thanks!

@antoinemine antoinemine merged commit 96c122a into ocaml:master Jan 2, 2023
@antoinemine
Copy link
Collaborator

I noticed a regression after merging : make test no longer works after a make without install.
I've committed a tentative fix 5393460 which applies -ldopt only for LDFLAGS, leaving LIBS as-is. Could you check that it is still OK for you ?

Also #73 might render these changes moot anyway, if we switch to dune. If you have time, could you tell me whether you can build zarith using dune with #73 ?

@xavierleroy
Copy link
Contributor

It is prudent to not use -ldopt for -llib library names or anything that is not a linker command-line option. For one thing, -llib library specifications must go last on the C linker's command line, and in a specific order, while ocamlmklib will put options first, in no specific order. For another thing, there may be Windows-specific handling of -llib library specifications.

@antoinemine
Copy link
Collaborator

Well, currently we have:

  • a list of libraries, that is detected by configure and added as -l options without -ldopt to ocamlmklib
  • the user-supplied LDFLAGS, which is passed through -ldopt

Would that be adequate, or do we need something different /more sophisticated?

@xavierleroy
Copy link
Contributor

I think it's good. At least, nothing wrong can happen when LDFLAGS is not given, which is the most frequent case.

@xavierleroy
Copy link
Contributor

I was too optimistic. It looks like -L/path/to/gmp/lib must be passed to ocamlmklib directly, not escaped behind a -ldopt flag, otherwise it's not honored where it is needed. I'm trying to narrow down the problem.

@xavierleroy
Copy link
Contributor

Here is the problem. ocamlmklib passes -ldopt xxx options to the linker when building dllzarith.so, the shared object that is used by bytecode executables.

However, for native code, a static archive libzarith.a is built, which is then statically linked by ocamlopt every time the ZArith library is used. -lyyy and -Lzzz options given to ocamlmklib are recorded in the zarith.cmxa file generated by ocamlmklib, and passed to the linker by ocamlopt, but xxx options given to ocamlmklib via -ldopt xxx are not recorded in zarith.cmxa, thus not passed to the linker by ocamlopt. So, -L/path/to/gmp/lib is not passed to the linker, and linking fails.

This can be viewed as a bug in ocamlmklib. But while it is there, -ldopt cannot be used to pass "important" linker options such as -L/path/to/gmp/lib. This currently breaks the OPAM build of ZArith on macos ARM64. So, I'm going to revert this PR and we'll try to find a better solution later.

xavierleroy added a commit to xavierleroy/Zarith that referenced this pull request Jul 19, 2023
`-L/path/to/gmp/lib` must be passed as is to `ocamlmklib`, not behind a `-ldopt` flag, otherwise it is ignored in static linking situations.
xavierleroy added a commit that referenced this pull request Jul 19, 2023
`-L/path/to/gmp/lib` must be passed as is to `ocamlmklib`, not behind a `-ldopt` flag, otherwise it is ignored in static linking situations.
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.

3 participants