Allow to embed the version inside executables #1930
Conversation
ghost
requested review from
bin/install_uninstall.ml
Outdated
| try | ||
| let s = Unix.readlink (Path.to_string path) in | ||
| Path.relative (Path.parent_exn path) s | ||
| with _ -> |
There was a problem hiding this comment.
Which exception is this designed to catch? The one raised by Unix.readlink? If that's teh case, I would simply surround Unix.readlink with that.
|
Do you plan to be able to allow the same thing for libraries? |
|
For library, we would need a fresh module so that different libraries can have their own build info. Maybe a field like this: and this would add a |
We don't need a fresh name, if the library is wrapped. So we could restrict the feature to wrapped library with a fixed module name. In any case, for uniformity could we plan the same syntax for executables and libraries ? (Something similar is also needed for |
|
Well, we can't reuse the same library. For instance, if Alternatively, we could pass the name of the library to the function call: |
Yes, they must be prefixed by the library name like any other module of the library. So in your example the modules are |
|
What would the feature look like from the point of view of the user? |
|
(basically I believe that we agree on the implementation, but not on how it's presented) |
That's fairly interesting as it would allow to easily dump the version of all libs used in by a particular executable. |
|
Yes, the disagreement is just on how the user access the feature And finally the difference is indeed very small with yours. It could be that instead of We would have: For each library in the Could we remove the need for the "%%VERSION%%" in the source code? |
|
I see. I feel like the Additionally, I feel slightly uneasy at the idea of implicitly adding modules to users' libraries. One the other hand, adding an implicit module at link time when the special
That seems fine yes. In fact, this file already exists: it's the opam file. |
|
I'll do the change to not have to write |
ghost
changed the title
ghost
mentioned this pull request
I agree that adding a module to a user's library is a bit weird, but it's also a bit weird to add to a user's executable. So I'm wondering why not support it in libraries in the first place? In our code, we use build information in quite a few places, and so we generate an |
|
Well, we are not really adding anything to executables here. Basically the user depends on the library In any case, libraries can also depend on Before libraries are linked into executable, the version information is already available. For instance in the META file. This PR is basically just about embedding the information into the final executable so that it stays available once the executable is deployed. But for instance, if you target opam then all this is almost useless: the version is already provided by opam. This PR just adds a bit of plumbing so that |
|
Sorry, I probably I misunderstood several things. I thought I think you're providing what we need, but just to clarify our needs are:
You may have eliminated this need, but to achieve the fast dev cycle, we currently leave a dummy string in our |
I think it is, but it is not added to the executable but to a dune library (Dune.build_info) which is linked into the executable. So technically yes, we are generating a module, but it is explicitly asked by the user. In the case of libraries we would have to add the module regardless of if the dune library is used. |
|
Yep, that's the idea. @agarwal I believe this PR ticks all the boxes. It also leaves a dummy string during development to achieve fast development cycle. However, the criterion for using a dummy string vs the real info is a bit different:
In this PR, the only time this rewriting happens is when calling |
That's good for us. Thank you! |
|
This doesn't seem to work when the |
|
That seems fine |
|
Re-opened: #2224 |
Is it me, or is it still the case that the |
|
I just gave it a try and it seems to work with Dune 2.5; with a |
|
Thanks for checking. That sounds like the situation I have, so it's not obvious. I'll try to extract a small repro. |
|
Just a minor update: I'm no longer convinced that the issue I see is related to |
|
Ok, I just tried this by pinning dune to #3589 and checking out ocaml-ppx/ocamlformat#1133. I don't see any output from the new flag, am I doing it wrong? |
|
So I just tried the same and it does work as expected for me. In particular, I don't get warning 58. I think we should focus on that first, because this is very odd. What version of OCaml are you using? Could you also try doing the build with |
|
I'm using ocaml 4.10.0. I just retried using |
|
For info, here is the |
|
What version of ? |
|
And also: in the ocamlformat repo |
|
but curiously And |
|
Sorry, I was missing a backquote in the ocamlfind command, it should have been: I wanted to check that it did have the
One thing we should check is that the placeholder does end up in the produced binary. Could you try the following: |
|
One thing I noticed looking at he log: you are using an opam local switch. I'm not using one. I wonder if that makes a difference. I'm trying with a local switch now to check. |
|
As a data point, I'm using a local switch, and it's working as expected (no cmx warning, substitution works). |
|
Just FTR, with the missing backquote ocamlfind is now happy: Neither This seems to be strange. I'd almost say that it's "just my machine" but it seems that @gpetiot has the same issue so there must be something going on. |
|
To make sure that the placeholder is linked in, do you have any input when you run: And if there are symbols: and (if this work, first one should display and second one: |
|
|
|
running gdb interactively seems to make it happy, but it doesn't find the symbol you indicated: For reference, the output of objdump is: |
|
OK, I found something! these symbols are only used by flambda. I tried to reproduce with a flambda switch, and indeed this does not perform any substitution. I'll investigate to see if there's some constant folding going on. |
|
(the cmx warning appears as well) |
|
Ah, interesting! @emillon maybe try this: That should prevent flambda from doing clever things. Though I would have thought the |
|
Yes that fixes it (the cmx warning is still there, though)! I'll open a PR on dune to fix that. |
|
Oh, I'm sorry, I completely forgot (to mention) that I am using an flambda-enabled compiler. |
This ensures that they are not inlined when using flambda. See discussion in ocaml#1930. Signed-off-by: Etienne Millon <[email protected]>
This ensures that they are not inlined when using flambda. This problem is only present in OCaml 4.10.0, so it is fine not to patch on 4.02 where `Sys.opaque_identity` is not available. See discussion in ocaml#1930. Signed-off-by: Etienne Millon <[email protected]>
This ensures that they are not inlined when using flambda. This problem is only present in OCaml 4.10.0, so it is fine not to patch on 4.02 where `Sys.opaque_identity` is not available. See discussion in ocaml#1930. Signed-off-by: Etienne Millon <[email protected]>
This ensures that they are not inlined when using flambda. This problem is only present in OCaml 4.10.0, so it is fine not to patch on 4.02 where `Sys.opaque_identity` is not available. See discussion in #1930. Signed-off-by: Etienne Millon <[email protected]>
This PR allows users to embed the version (as returned by
git describe --always --dirty) inside installed executables.Spec
The following expression will always evaluate to the version once the binary is installed:
During the build, it will be a dummy string, ensuring that embedding the version inside the executable doesn't hurt the development experience.
Implementation
When the string passed to
get_versionis already substituted, it is returned as it. This ensures that the right version is reported either for pinned or released packages.When the string
%%VERSOIN%%, the version is read from an internal buffer inside thedune.build-infolibrary. This buffer is a placeholder that is filled bydune install.Future work
Fill in the placeholder for binaries that are promoted to the source tree. This will give an easy workflow for users who deploy binaries directly without going through a package manager.