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

dune-site does not work with opam #4198

Closed
kit-ty-kate opened this issue Feb 4, 2021 · 11 comments
Closed

dune-site does not work with opam #4198

kit-ty-kate opened this issue Feb 4, 2021 · 11 comments

Comments

@kit-ty-kate
Copy link
Member

kit-ty-kate commented Feb 4, 2021

Expected Behavior

dune-site should work when used in a package installed by opam.
In my opinion the destination directory given by dune-site should always return exactly one directory:

  • either _build/install/default/<dir> when called from the source code by dune
  • or ../<dir> when called from a directory called bin/ (e.g. _build/install/default/bin/ or $prefix/bin)
  • default to /usr/local/<dir> otherwise (or maybe give just None)

Actual Behavior

The current implementation of dune-site does not work when the package in installed by opam.
The destination directory given by dune-site returns [] when the program is called directly without having called dune install before.

Reproduction

$ cat dune-project
(lang dune 2.8)
(using dune_site 0.1)

(package
 (name test)
 (sites (lib_root test)))
$ cat dune
(executable
 (public_name test)
 (libraries dune-site))

(generate_sites_module
 (module test_libdir)
 (sites test))

(install
 (section (site (test test)))
 (files a))
$ touch a
$ cat test.ml
let _ = List.hd Test_libdir.Sites.test
$ dune build -p test
$ ./_build/install/default/bin/test
Fatal error: exception Failure("hd")

Specifications

  • Version of dune (output of dune --version): 2.8.2
@bobot
Copy link
Collaborator

bobot commented Feb 4, 2021

I believe there is two problem in one. The one about opam is the most problematic.

Context

We use binary substitution for implementing different features (e.g build_info, sites) in order to keep the built binary independent of these informations and so to be able to use more the cache.

However dune does the binary substitution only during promotion and installation through (dune install). So it is not done when opam use the install file which points directly inside _build.

Possibilities?

  • use dune install instead of using an install file
  • create a substituted file inside _build/for_installation

@kit-ty-kate
Copy link
Member Author

I believe there is two problem in one.

Sorry for mixing things up.

Possibilities?

* use `dune install` instead of using an install file

* create a substituted file inside `_build/for_installation`

I don't think dune install is an option. The same thing will be encountered by linux/BSD distributions that might use _build/default/install/* as the set of directory to install and find the same problem only at runtime.

I'm curious though, I'm not sure to understand why binary substitution is required here. To my untrained eye it seems to me like an overkill solution for this but I'm probably missing something.

@bobot
Copy link
Collaborator

bobot commented Feb 4, 2021

I don't think dune install is an option. The same thing will be encountered by linux/BSD distributions that might use _build/default/install/* as the set of directory to install and find the same problem only at runtime.

Distribution uses (or should use) dune install --prefix=/usr --destdir=/tmp/destdir, I don't see any drawbacks for them.

I'm curious though, I'm not sure to understand why binary substitution is required here. To my untrained eye it seems to me like an overkill solution for this but I'm probably missing something.

In a nutshell, you don't want to put the information in the source, otherwise you can't reuse the cache of for example one local opam switch in another.

@ejgallego
Copy link
Collaborator

The fact that we rewrite the binary always seemed a bit tricky to me, here the sites / version info seems to be a runtime dependency, so why not just generate a file binary.exe.dune-metadata which the utility libraries can read to obtain this info?

For example, the current approach creates a bit of pain when adding the real binaries to path which is needed for some cases where we have non-dunerized setup use our apps.

@bobot
Copy link
Collaborator

bobot commented Apr 28, 2021

To fix the problem the binaries will not need to be rewritten for opam like layout (_build follow this layoutà). It should fix most problem. Adding another file doesn't really help, the file needs to be found.

@ejgallego
Copy link
Collaborator

Adding another file doesn't really help, the file needs to be found.

I was thinking of having the invariant the file is in the same location than the binary.

@bobot
Copy link
Collaborator

bobot commented May 6, 2021

Using a file at the same location than the binary, is a possibility that as drawbacks but advantage. I prefer to stick to the current solution but if you want to look at it by writing an RFC, you need to be careful with:

  • libraries can also have site information, and libraries can be dynamically linked.
  • It is not always possible to find the real place of a binary, we must at least know which ones

For this issue, I think the main problem is that we let opam take directly from the _build directory of dune without care. I would propose that project that activate the site feature would need to run an additional dune build @install_by_opam (it could be added automatically to the opam file generated by dune) which would create the substituted binaries. The install file would points to this new binary.

The idea of using the opam layout by default is complicated by installed libraries that use sites.

@ejgallego
Copy link
Collaborator

An additional problem with binary rewriting is that it will perform build in dune install, right?

If so that breaks use cases doing sudo dune install .

I am indeed not sure that the current model is the right one, it seems to me that the binary rewriting is breaking a quite established workflow and now suddenly we have to tell a lot of users "you are all doing it wrong".

@ivg
Copy link
Member

ivg commented May 6, 2021

BInary rewriting also sounds like a big no-no to me. First of all, it breaks an established paradigm that the build artifact could be used and packed as it is.

Next, the problem that dune is trying to solve is well-known and is being addressed in many ways by many tools. To some extent, using binary rewriting, yes that is what the dynamic loader (ld.so) is doing. But only on the load time and it never touches the artifact. Practically, all solutions rely on the availability of a central program of authority with a well-known location that is the administrative resource for information about library/packages/sites location. As an escape hatch, we also have specific command-lines options (like -R in gcc) that still enable us to hardcode paths if so is needed and environment variables as the last resort measure (e.g., LD_LIBRARY_PATH). Finally, there is the default set of paths that could be hardcoded in all binaries, e.g., /usr/lib.

To summarize, the well-established approach is to keep this information in an external executable. In our case, it could be ocaml itself, or opam (sounds like a bad idea, but just lets add for the sake of completeness), or some specialized binary that had to be installed whenever ocaml is installed. Yes, the main drawback is that this makes OCaml programs non-independent and require some runtime support. The same is true for C programs, as ld.so is the same runtime support.

Ideally, and this is what historically OCaml was doing, we should strive for full compatibility with C runtime. On Linux, I think it is possible, we have plenty of options, e.g., loader scripts to adapt our binaries to the environment. On the other operating systems, the story and traditions are different. Both Windows and macOS tend to hardcode the library information in the binaries, so trying to bring our view to those operating systems will contradict the established traditions and turn OCaml binaries into second-class citizens.

Another option, especially, for sites and dynamic linking is to store this information in a library that provides those services. So that any OCaml binary that requires this site information (or needs to load a plugin) will be able to query this information through the library interface. Yes, it is moving the problem to another location, but, at least, this location is a single location.

TL;DR;

  1. This runtime information shall be available through a service (a binary/framework/library).
  2. The problem is not new and well-known and community-acceptable solutions exist. Other languages, including C, are experiencing the same problems. So we need to investigate their solutions.
  3. Binary rewriting breaks the least surprize principle and should be avoided by all means.

@bobot
Copy link
Collaborator

bobot commented May 19, 2021

An additional problem with binary rewriting is that it will perform build in dune install, right?

No it does the rewriting on the fly during copying. sudo dune install should not be broken. It is not binary rewriting per-se it is more smart-copying :)

BInary rewriting also sounds like a big no-no to me. First of all, it breaks an established paradigm that the build artifact could be used and packed as it is.

The build artifact are not directly available; they are inside _build, I prefer to keep the liberty that dune is the only one to interact with them.

The site feature of dune is already I think a service, even if it could be improved. I agree that there are other possibilities and I'm happy to discuss them. There are a lot of constraints (e.g be shared-cache friendly, don't control the dynamic linking of libraries) and the current solution is perhaps not the best. However I would like to keep this issue as fixing the problem without changing the whole feature.

@bobot
Copy link
Collaborator

bobot commented May 19, 2021

During the current dev meeting we converged on another possibility:

  • Add dune install --destdir=opam_destdir --create-install-file as the last build command in opam generated file
  • It would do the substitution as usual then create an install file for opam to install the files
  • copy and rewrite there only the files that needs rewriting

bobot added a commit to bobot/jbuilder that referenced this issue May 21, 2021
        - Substitute the files in an intermediate directory if needed
        - Backward compatible

        Fixes ocaml#4198 ocaml#4212
bobot added a commit to bobot/jbuilder that referenced this issue May 25, 2021
        - Substitute the files in an intermediate directory if needed
        - Backward compatible

        Fixes ocaml#4198 ocaml#4212
bobot added a commit to bobot/jbuilder that referenced this issue May 25, 2021
        - Substitute the files in an intermediate directory if needed
        - Backward compatible

        Fixes ocaml#4198 ocaml#4212

Signed-off-by: François Bobot <francois.bobot@cea.fr>
bobot added a commit to bobot/jbuilder that referenced this issue May 27, 2021
        - Substitute the files in an intermediate directory if needed
        - Backward compatible

        Fixes ocaml#4198 ocaml#4212

Signed-off-by: François Bobot <francois.bobot@cea.fr>
bobot added a commit to bobot/jbuilder that referenced this issue May 28, 2021
        - Substitute the files in an intermediate directory if needed
        - Backward compatible

        Fixes ocaml#4198 ocaml#4212

Signed-off-by: François Bobot <francois.bobot@cea.fr>
bobot added a commit that referenced this issue Jun 9, 2021
        - Substitute the files in an intermediate directory if needed
        - Backward compatible

        Fixes #4198 #4212

Signed-off-by: François Bobot <francois.bobot@cea.fr>
bobot added a commit to bobot/jbuilder that referenced this issue Jun 14, 2021
        - Substitute the files in an intermediate directory if needed
        - Backward compatible

        Fixes ocaml#4198 ocaml#4212

Signed-off-by: François Bobot <francois.bobot@cea.fr>
bobot added a commit to bobot/jbuilder that referenced this issue Jun 14, 2021
        - Substitute the files in an intermediate directory if needed
        - Backward compatible

        Fixes ocaml#4198 ocaml#4212

Signed-off-by: François Bobot <francois.bobot@cea.fr>
bobot added a commit to bobot/jbuilder that referenced this issue Jun 15, 2021
        - Substitute the files in an intermediate directory if needed
        - Backward compatible

        Fixes ocaml#4198 ocaml#4212

Signed-off-by: François Bobot <francois.bobot@cea.fr>
bobot added a commit to bobot/jbuilder that referenced this issue Jun 24, 2021
        - Substitute the files in an intermediate directory if needed
        - Backward compatible

        Fixes ocaml#4198 ocaml#4212

Signed-off-by: François Bobot <francois.bobot@cea.fr>
@bobot bobot closed this as completed in 51e4940 Jun 24, 2021
ejgallego added a commit to ejgallego/opam-repository that referenced this issue Jun 29, 2021
…ator, dune-private-libs, dune and dune-build-info (2.9.0)

CHANGES:

- Add `(enabled_if ...)` to `(mdx ...)` (ocaml/dune#4434, @emillon)

- Add support for instrumentation dependencies (ocaml/dune#4210, fixes ocaml/dune#3983, @nojb)

- Add the possibility to use `locks` with the cram tests stanza (ocaml/dune#4480, @voodoos)

- Allow to set up merlin in a variant of the default context
  (ocaml/dune#4145, @TheLortex, @voodoos)

- Add `(package ...)` to `(mdx ...)` (ocaml/dune#4691, fixes ocaml/dune#3756, @emillon)

- Handle renaming of `coq.kernel` library to `coq-core.kernel` in Coq 8.14 (ocaml/dune#4713, @proux01)

- Fix generation of merlin configuration when using `(include_subdirs
  unqualified)` on Windows (ocaml/dune#4745, @nojb)

- Fix bug for the install of Coq native files when using `(include_subdirs qualified)`
  (ocaml/dune#4753, @ejgallego)

- Allow users to specify install target directories for `doc` and
  `etc` sections. We add new options `--docdir` and `--etcdir` to both
  Dune's configure and `dune install` command. (ocaml/dune#4744, fixes ocaml/dune#4723,
  @ejgallego, thanks to @JasonGross for reporting this issue)

- Fix issue where Dune would ignore `(env ... (coq (flags ...)))`
  declarations appearing in `dune` files (ocaml/dune#4749, fixes ocaml/dune#4566, @ejgallego @rgrinberg)

- Disable some warnings on Coq 8.14 and `(lang coq (>= 0.3))` due to
  the rework of the Coq "native" compilation system (ocaml/dune#4760, @ejgallego)

- Fix a bug where instrumentation flags would be added even if the
  instrumentatation was disabled (@nojb, ocaml/dune#4770)

- Fix ocaml/dune#4682: option `-p` takes now precedence on environement variable
  `DUNE_PROFILE` (ocaml/dune#4730, ocaml/dune#4774, @bobot, reported by @dra27 ocaml/dune#4632)

- Fix installation with opam of package with dune sites. The `.install` file is
  now produced by a local `dune install` during the build phase (ocaml/dune#4730, ocaml/dune#4645,
  @bobot, reported by @kit-ty-kate ocaml/dune#4198)

- Fix multiple issues in the sites feature (ocaml/dune#4730, ocaml/dune#4645 @bobot, reported by @Lelio-Brun
  ocaml/dune#4219, by @Kakadu ocaml/dune#4325, by @toots ocaml/dune#4415)
ejgallego added a commit to ejgallego/opam-repository that referenced this issue Jul 1, 2021
…ator, dune-private-libs, dune and dune-build-info (2.9.0)

CHANGES:

- Add `(enabled_if ...)` to `(mdx ...)` (ocaml/dune#4434, @emillon)

- Add support for instrumentation dependencies (ocaml/dune#4210, fixes ocaml/dune#3983, @nojb)

- Add the possibility to use `locks` with the cram tests stanza (ocaml/dune#4480, @voodoos)

- Allow to set up merlin in a variant of the default context
  (ocaml/dune#4145, @TheLortex, @voodoos)

- Add `(package ...)` to `(mdx ...)` (ocaml/dune#4691, fixes ocaml/dune#3756, @emillon)

- Handle renaming of `coq.kernel` library to `coq-core.kernel` in Coq 8.14 (ocaml/dune#4713, @proux01)

- Fix generation of merlin configuration when using `(include_subdirs
  unqualified)` on Windows (ocaml/dune#4745, @nojb)

- Fix bug for the install of Coq native files when using `(include_subdirs qualified)`
  (ocaml/dune#4753, @ejgallego)

- Allow users to specify install target directories for `doc` and
  `etc` sections. We add new options `--docdir` and `--etcdir` to both
  Dune's configure and `dune install` command. (ocaml/dune#4744, fixes ocaml/dune#4723,
  @ejgallego, thanks to @JasonGross for reporting this issue)

- Fix issue where Dune would ignore `(env ... (coq (flags ...)))`
  declarations appearing in `dune` files (ocaml/dune#4749, fixes ocaml/dune#4566, @ejgallego @rgrinberg)

- Disable some warnings on Coq 8.14 and `(lang coq (>= 0.3))` due to
  the rework of the Coq "native" compilation system (ocaml/dune#4760, @ejgallego)

- Fix a bug where instrumentation flags would be added even if the
  instrumentatation was disabled (@nojb, ocaml/dune#4770)

- Fix ocaml/dune#4682: option `-p` takes now precedence on environement variable
  `DUNE_PROFILE` (ocaml/dune#4730, ocaml/dune#4774, @bobot, reported by @dra27 ocaml/dune#4632)

- Fix installation with opam of package with dune sites. The `.install` file is
  now produced by a local `dune install` during the build phase (ocaml/dune#4730, ocaml/dune#4645,
  @bobot, reported by @kit-ty-kate ocaml/dune#4198)

- Fix multiple issues in the sites feature (ocaml/dune#4730, ocaml/dune#4645 @bobot, reported by @Lelio-Brun
  ocaml/dune#4219, by @Kakadu ocaml/dune#4325, by @toots ocaml/dune#4415)
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

No branches or pull requests

4 participants