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

Relocation Capability to the build info #2905

Closed
wants to merge 1 commit into from

Conversation

rgrinberg
Copy link
Member

I find that the discussion in #1185 derailed us from implementing a fairly simple feature. I don't think it's necessary to get cover all use cases exhaustively before it's useful. So I've started a prototype that should cover most bases. If it doesn't, we'll always be able to extend it.

The PR is still not fully functional, but if there's a positive response from the maintainers, I can finish it up.

cc the parties who were interested in this feature: @bobot @emillon @ejgallego

Add the interface to the build info library. Since the relocation
library needs some build information.

Signed-off-by: Rudi Grinberg <[email protected]>
@ejgallego
Copy link
Collaborator

Thanks @rgrinberg ! Also cc @rlepigre

Copy link
Collaborator

@emillon emillon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a nice trick to use build-info to store this. I think this corresponds to what @bobot had in mind. I'm a bit curious how it will work when an executable is linked against several libraries that use this.

@rgrinberg
Copy link
Member Author

I'm a bit curious how it will work when an executable is linked against several libraries that use this.

As far as I understand, this should not make much of a difference. We create the implementation of this library only when creating the executable, so it does not have matter if we have 1 or more libraries.

@ghost
Copy link

ghost commented Nov 27, 2019

I think it's good to work on this :)

Here is a thought I had last time I was thinking of this feature: let's say you have two libraries A and B that read a configuration file in etc. A is installed and B is part of the workspace. For A, etc should be the global one while for B it should be the local one. Note sure this matters too much, especially for etc, but it might for share for instance. Maybe we could return a list of paths to consider instead of just one?

Another thought: the produced binaries are no longer relocatable, right? Given that we hardcode the opam prefix in them. Maybe we could instead store the location of etc and share as a relative path from Sys.executable_name.

@rgrinberg
Copy link
Member Author

Here is a thought I had last time I was thinking of this feature: let's say you have two libraries A and B that read a configuration file in etc. A is installed and B is part of the workspace. For A, etc should be the global one while for B it should be the local one. Note sure this matters too much, especially for etc, but it might for share for instance. Maybe we could return a list of paths to consider instead of just one?

Isn't this solved by having the look up functions take a package argument? E.g. if the package is installed, it would be looked up from there. If the package is local, the build location would be preferred.

Another thought: the produced binaries are no longer relocatable, right? Given that we hardcode the opam prefix in them. Maybe we could instead store the location of etc and share as a relative path from Sys.executable_name.

We can solve this with virtual libraries as @emillon mentioned earlier. But here's a simpler way to make it work. Introduce:

val set_lookup : (Location.t -> path) -> unit

Then, it's just a matter of having the user link the library that calls this first. IMO that's the simplest approach to allow for whatever custom implementations users might want.

@ghost
Copy link

ghost commented Nov 27, 2019

Isn't this solved by having the look up functions take a package argument? E.g. if the package is installed, it would be looked up from there. If the package is local, the build location would be preferred.

Indeed. After there might be packages that define installation "sites", such as the "site-packages" of emacs. I believe Coq has something like this as well. But in any case we can leave this problem for later.

val set_lookup : (Location.t -> path) -> unit

How would that work? In particular, how do we avoid hardcoding the opam prefix in to the executable?

@rgrinberg
Copy link
Member Author

How would that work? In particular, how do we avoid hardcoding the opam prefix in to the executable?

Well the implementation provided to set_lookup would essentially ignore it. In pseaudo-code, it would be something like:

set_lookup(fun location ->
  let dir = Filename.dirname Sys.executable_name in
  (* find path of location relative to dir *)

@ghost
Copy link

ghost commented Nov 27, 2019

But why do we need this to be settable? And I don't understand the story with virtual libraries either.

@rgrinberg
Copy link
Member Author

But why do we need this to be settable? And I don't understand the story with virtual libraries either.

Well, the default implementation is going to respect the opam prefix as we've said. To make the binary relocatable, we need a different lookup scheme for artifacts. Rather than hard coding look ups based on the executable, we could just give the user total control in their binaries.

With virtual libraries, the idea was to make the relocation library virtual and have the relocatable binary use cases be an implementation of this virtual library.

@ghost
Copy link

ghost commented Nov 28, 2019

I'm just not seeing the whole story here. Just to make sure we are on the same page, the goal here is to build a single foo.exe that might be executed in all these cases:

  1. during the build, as part of code generation or the test suite for instance
  2. when installed in viarous opam roots
  3. when installed in /usr/bin or some other directory outside of opam

And in all these cases it should be able to find some config/data files. Do you agree?

@rgrinberg
Copy link
Member Author

Yes, I agree with those goals.

I expect that 1. will be covered by dune setting an environment variable to override the locations.

  1. & 3. will be covered by setting an appropriate option when building dune. E.g. when we build dune in opam, we’ll have $ ocaml configure.ml —-etc-dir %{set by opam}.

Perhaps the point of confusion is that I'm not trying to address relocatable binaries at all. I don't see the need to avoid hard coding the opam prefix in the binary. If someone wants a different look up behavior (to have a relocatable binary for example), I suggest they just override the lookup mechanism at runtime.

@ghost
Copy link

ghost commented Nov 28, 2019

Ah, well the point is the shared cache. You can't share artifacts that have hardcoded paths like this because the etc directories will be different between different opam roots/switches.

@rgrinberg
Copy link
Member Author

There was some offline discussion about this. Jeremie's comment above refers to that binaries should avoid having hard coded strings in them. It would make completely unsharable in our upcoming artifacts cache. In particular, we want binaries different from opam switches to be shareable.

This means that we'd prefer approach that relied on runtime information to detect the location of artifacts.

@rgrinberg rgrinberg closed this Apr 24, 2020
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