Introduce Local_package module #1554
Conversation
rgrinberg
force-pushed
the
installed-package
branch
2 times, most recently
from
ea8cd7d
to
1f8059a
Compare
|
I'm also planning on reversing the dependency on super_context and local_pacakge. This will be useful twofold:
|
|
I glanced over the PR and it looks good to me. As a rule of thumb, taking out chunks of code from big modules to put them into smaller logical units is generally a good idea. |
rgrinberg
force-pushed
the
installed-package
branch
2 times, most recently
from
a2babd6
to
17af1d3
Compare
This package is used to contain all the information required for installing a package Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>
Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>
Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>
these two are almost the same Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>
Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>
rgrinberg
force-pushed
the
installed-package
branch
from
17af1d3
to
cbf59a7
Compare
|
I need this for other PR's, so I interpreted diml's comment as an approval as I've accepted the only suggestion. Feel free to make further suggestions/refactorings as this part of the code still needs a lot of work. |
| @@ -226,90 +223,41 @@ module Gen(P : Params) = struct | |||
| | Default -> true | |||
| | Opam _ -> false | |||
|
|
|||
| let bin_entries, other_stanzas, install_paths_per_package = | |||
There was a problem hiding this comment.
Sorry, I didn't read this PR in detail initially. This needed to be done at the toplevel of this module, in order for the rules producing symlinks in _build/install/default/bin to be setup ASAP. I can't find where this code was moved
There was a problem hiding this comment.
Ah I see. Where do we depend on the rules being setup before anything else?
There was a problem hiding this comment.
In Artifacts.create, we simply list _build/install/default/bin to find out the list of public binaries. This avoids duplicating the logic to interpret install stanzas. However, this requires the install rules to be setup at this point. The install rules for libraries requires to know the list of modules, which is done via Dir_contents which interprets stanza like rule or copy_files. Interpreting these stanzas requires expanding percent forms and so requires the Artifacts.t. Given that, the install rules for bin must be setup before anything else. We introduced this in #1354.
There was a problem hiding this comment.
Oh yeah, I remember now. It's quite unfortunate that this is a bit fragile. Do you have a test case for this as well, or shall I add one?
There was a problem hiding this comment.
I don't have one, adding one is a good idea.
CHANGES: - Fix regression introduced by ocaml/dune#1554 reported in: ocaml/dune#734 (comment) (ocaml/dune#1612, @rgrinberg) - Fix `dune external-lib-deps` when preprocessors are not installed (ocaml/dune#1607, @diml)
This package is used to contain all the information required for installing a package.
The old code was overdoing the amount of package maps that it was using. The purpose of this new code is to address that issue. I didn't realize that this refactoring would take so long, so there are still some parts that I've left unfinished. I think it's already an improvement over what we've had before, but I'd like to get some suggestions from the other maintainers before committing more time to this.