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

Add link flags ocamlmklib when using ctypes stubs. #8784

Merged
merged 2 commits into from
Mar 14, 2024

Conversation

frejsoya
Copy link
Contributor

@frejsoya frejsoya commented Sep 27, 2023

Add link flags for external dependencies when using ctypes.

Required when the external lib is not statically linked, in which case
linker needs to be aware at runtime what libs to call. This is the case
for

  • dune exec .bc
  • dune utop
  • mdx tests

That is add to ocamlmklib, link flags such as -l -L
from ctypes if info is available.

If dependent library are not installed in expected locations, env
variable such as LD_LIBRARY_PATH must be extended when required program
is run, depending on os. This may be fixed by adding relocatable paths to
link flags.

@frejsoya frejsoya force-pushed the ctypes-add-stubs-link-flags branch 3 times, most recently from e4de22d to 17f5f39 Compare September 30, 2023 15:36
@frejsoya frejsoya changed the title WIP: Add link flags to ocamlmklib. Add link flags ocamlmklib when using ctypes stubs. Sep 30, 2023
@frejsoya frejsoya force-pushed the ctypes-add-stubs-link-flags branch from 17f5f39 to d8a1c30 Compare September 30, 2023 15:38
@frejsoya frejsoya marked this pull request as ready for review September 30, 2023 15:39
@frejsoya
Copy link
Contributor Author

frejsoya commented Sep 30, 2023

This change simply copies what is done in exe_rules.ml

It took some time to figure out that lib_rules.ml has to check explicitly what ctypes believes are the link flags vs. it being contained within in ctype_rules.ml:gen_rules

In the initial PR a test-case for #7146 is a repro about link failures/path expanding. While the test now seemingly "works", The actual problem is that this PR to discards existing c_library_flags stanza.

@frejsoya frejsoya force-pushed the ctypes-add-stubs-link-flags branch 4 times, most recently from d8d72b1 to 097b245 Compare October 2, 2023 06:09
@frejsoya frejsoya marked this pull request as draft October 2, 2023 07:04
@frejsoya frejsoya force-pushed the ctypes-add-stubs-link-flags branch from 097b245 to 522121d Compare October 4, 2023 13:12
@frejsoya frejsoya marked this pull request as ready for review October 5, 2023 08:02
@frejsoya frejsoya force-pushed the ctypes-add-stubs-link-flags branch from 522121d to 26d8b0c Compare October 5, 2023 08:06
@lukstafi
Copy link

I face the problem of ocamlmklib not getting the library that it's supposed to link with for both the manual "vendored" setup, and using pkg_config (where I generate the .pc file manually).

@rgrinberg
Copy link
Member

@lukstafi does this PR fix the problem for you?

@lukstafi
Copy link

lukstafi commented Jan 12, 2024

@lukstafi does this PR fix the problem for you?

Yes! For (build_flags_resolver pkg_config), this PR solves the problem -completely-. For (build_flags_resolver (vendored ...)), with this PR (not synced/rebased) I regress to issue #108. Then, I can ameliorate that one with e.g. CAML_LD_LIBRARY_PATH=./_build/default/lib:$CAML_LD_LIBRARY_PATH dune exec examples/tut01_hello_world.bc.
Strangely/unfortunately, (env (_ (env-vars (CAML_LD_LIBRARY_PATH "./_build/default/lib:...")))) does not work. Edit: vendored also works with just this PR and dune clean; dune build; dune exec -- but I double-checked that dune clean; CAML_LD_LIBRARY_PATH=... dune exec is an alternative way to get it working.

In my case, I would go ahead using (build_flags_resolver pkg_config) (it's cleaner despite the extra complexity).

Edit: Another complication! dune clean; dune exec does not work with PR + (build_flags_resolver pkg_config). But dune clean; dune build; dune exec works; dune clean; CAML_LD_LIBRARY_PATH=... dune exec also works.

@frejsoya
Copy link
Contributor Author

frejsoya commented Jan 12, 2024

Note i have little clue about dune internals and even more so with ctypes. Similar its been decades since i dealt with linkers/shared libraries.

There are a few things that need to be correct if i recall correctly.

  1. dllstub.so must be locatable by ocamlrun https://v2.ocaml.org/manual/runtime.html#s:ocamlrun-dllpath
  2. afaik ocamlrun ends with dlopen(dllstub.so). So any dependencies on libraries needed by dllstub needs to be resolvable as well.

For the vendored libraries (non pkg-config) either manually setting LD_LIBRARY_PATH at runtime or adding the right rolcoatable (rpath) linker flags could resolve that part.

(See rpath discussion below wrt to CMAKE.

https://gitlab.kitware.com/cmake/community/-/wikis/doc/cmake/RPATH-handling

@lukstafi
Copy link

For the vendored libraries (non pkg-config) either manually setting LD_LIBRARY_PATH at runtime or adding the right rolcoatable (rpath) linker flags could resolve that part.

So it looks to me rpath is not helpful in case of dllstubs because it would require different build flags / binaries for development and for install.

@lukstafi
Copy link

Another complication, perhaps not related to this PR: bytecode expect-test framework tests: (library (inline_tests) (modes byte) ...) are not run by dune test -- it only triggers a build.

@rgrinberg
Copy link
Member

Another complication, perhaps not related to this PR: bytecode expect-test framework tests: (library (inline_tests) (modes byte) ...) are not run by dune test -- it only triggers a build.

Would you like to report a separate issue for this? Preferably with a minimal example that reproduces the issue.

Signed-off-by: Frej Soya <frej.soya@gmail.com>
Required when the external lib is not statically linked, in which case
linker needs to be aware at runtime what libs to call. This is the case
for

- dune exec <file>.bc
- dune utop
- mdx tests

That is add to ocamlmklib, link flags such as -l<libname> -L<libdir>
from ctypes if info is available.

If dependent library are not installed in expected locations, env
variable such as LD_LIBRARY_PATH must be extended when required program
is run, depending on os. This may be fixed by adding relocatable paths to
link flags.

Signed-off-by: Frej Soya <frej.soya@gmail.com>
@rgrinberg rgrinberg force-pushed the ctypes-add-stubs-link-flags branch from 7229052 to 2ad6ac9 Compare March 13, 2024 22:31
@rgrinberg rgrinberg added this to the 3.15.0 milestone Mar 13, 2024
@rgrinberg rgrinberg merged commit 6848420 into ocaml:main Mar 14, 2024
27 checks passed
Leonidas-from-XIV added a commit to Leonidas-from-XIV/opam-repository that referenced this pull request Mar 26, 2024
CHANGES:

### Added

- Add link flags to to `ocamlmklib` for ctypes stubs (ocaml/dune#8784, @frejsoya)

- Remove some unnecessary limitations in the expansions of percent forms in
  install stanza. For example, the `%{env:..}` form can be used to select files
  to be installed. (ocaml/dune#10160, @rgrinberg)

- Allow artifact expansion percent forms (`%{cma:..}`, `%{cmo:..}`, etc.) in
  more contexts. Previously, they would be randomly forbidden in some fields.
  (ocaml/dune#10169, @rgrinberg)

- Allow `%{inline_tests}` in more contexts (ocaml/dune#10191, @rgrinberg)

- Remove limitations on percent forms in the `(enabled_if ..)` field of
  libraries (ocaml/dune#10250, @rgrinberg)

- Support dialects in `dune describe pp` (ocaml/dune#10283, @emillon)

- Allow defining executables or melange emit stanzas with the same name in the
  same folder under different contexts. (ocaml/dune#10220, @rgrinberg, @jchavarri)

### Fixed

- coq: Delay Coq rule setup checks so OCaml-only packages can build in hybrid
  Coq/OCaml projects when `coqc` is not present. Thanks to @vzaliva for the
  test case and report (ocaml/dune#9845, fixes ocaml/dune#9818, @rgrinberg, @ejgallego)

- Fix conditional source selection with `select` on `bigarray` in OCaml 5
  (ocaml/dune#10011, @moyodiallo)

- melange: fix inconsistency in virtual library implementation. Concrete
  modules within a virtual library can now refer to its virtual modules too
  (ocaml/dune#10051, fixes ocaml/dune#7104, @anmonteiro)

- melange: fix a bug that would cause stale `import` paths to be emitted when
  moving source files within `(include_subdirs ..)` (ocaml/dune#10286, fixes ocaml/dune#9190,
  @anmonteiro)

- Dune file formatting: output utf8 if input is correctly encoded (ocaml/dune#10113,
  fixes ocaml/dune#9728, @moyodiallo)

- Fix expanding dependencies and locks specified in the cram stanza.
  Previously, they would be installed in the context of the cram test, rather
  than the cram stanza itself (ocaml/dune#10165, @rgrinberg)

- Fix bug with `dune exec --watch` where the working directory would always be
  set to the project root rather than the directory where the command was run
  (ocaml/dune#10262, @gridbugs)

- Regression fix: sign executables that are promoted into the source tree
  (ocaml/dune#10263, fixes ocaml/dune#9272, @emillon)

- Fix crash when decoding dune-package for libraries with `(include_subdirs
  qualified)` (ocaml/dune#10269, fixes ocaml/dune#10264, @emillon)

### Changed

- Remove the `--react-to-insignificant-changes` option. (ocaml/dune#10083, @rgrinberg)
Leonidas-from-XIV added a commit to Leonidas-from-XIV/opam-repository that referenced this pull request Apr 3, 2024
CHANGES:

### Added

- Add link flags to to `ocamlmklib` for ctypes stubs (ocaml/dune#8784, @frejsoya)

- Remove some unnecessary limitations in the expansions of percent forms in
  install stanza. For example, the `%{env:..}` form can be used to select files
  to be installed. (ocaml/dune#10160, @rgrinberg)

- Allow artifact expansion percent forms (`%{cma:..}`, `%{cmo:..}`, etc.) in
  more contexts. Previously, they would be randomly forbidden in some fields.
  (ocaml/dune#10169, @rgrinberg)

- Allow `%{inline_tests}` in more contexts (ocaml/dune#10191, @rgrinberg)

- Remove limitations on percent forms in the `(enabled_if ..)` field of
  libraries (ocaml/dune#10250, @rgrinberg)

- Support dialects in `dune describe pp` (ocaml/dune#10283, @emillon)

- Allow defining executables or melange emit stanzas with the same name in the
  same folder under different contexts. (ocaml/dune#10220, @rgrinberg, @jchavarri)

### Fixed

- coq: Delay Coq rule setup checks so OCaml-only packages can build in hybrid
  Coq/OCaml projects when `coqc` is not present. Thanks to @vzaliva for the
  test case and report (ocaml/dune#9845, fixes ocaml/dune#9818, @rgrinberg, @ejgallego)

- Fix conditional source selection with `select` on `bigarray` in OCaml 5
  (ocaml/dune#10011, @moyodiallo)

- melange: fix inconsistency in virtual library implementation. Concrete
  modules within a virtual library can now refer to its virtual modules too
  (ocaml/dune#10051, fixes ocaml/dune#7104, @anmonteiro)

- melange: fix a bug that would cause stale `import` paths to be emitted when
  moving source files within `(include_subdirs ..)` (ocaml/dune#10286, fixes ocaml/dune#9190,
  @anmonteiro)

- Dune file formatting: output utf8 if input is correctly encoded (ocaml/dune#10113,
  fixes ocaml/dune#9728, @moyodiallo)

- Fix expanding dependencies and locks specified in the cram stanza.
  Previously, they would be installed in the context of the cram test, rather
  than the cram stanza itself (ocaml/dune#10165, @rgrinberg)

- Fix bug with `dune exec --watch` where the working directory would always be
  set to the project root rather than the directory where the command was run
  (ocaml/dune#10262, @gridbugs)

- Regression fix: sign executables that are promoted into the source tree
  (ocaml/dune#10263, fixes ocaml/dune#9272, @emillon)

- Fix crash when decoding dune-package for libraries with `(include_subdirs
  qualified)` (ocaml/dune#10269, fixes ocaml/dune#10264, @emillon)

### Changed

- Remove the `--react-to-insignificant-changes` option. (ocaml/dune#10083, @rgrinberg)
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