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

Rewrite unused package names #367

Merged

Conversation

Leonidas-from-XIV
Copy link
Member

@Leonidas-from-XIV Leonidas-from-XIV commented Dec 30, 2022

This PR started small and then continued snowballing a little as it was growing.

What this does is when deduplication is disabled (opt-in) to determine which dependency should build which dune packages (by scanning OPAM files for dune build -p <name>), this info is saved into the lockfile.

From there, when pulling, the it will process dune files and rename public names that are not to be built to avoid clashing.

@Leonidas-from-XIV Leonidas-from-XIV force-pushed the rewrite-package-names branch 3 times, most recently from 84a7c83 to a88183b Compare January 13, 2023 15:52
@Leonidas-from-XIV Leonidas-from-XIV changed the title WIP Rewrite unused package names Rewrite unused package names Jan 17, 2023
@Leonidas-from-XIV Leonidas-from-XIV marked this pull request as ready for review January 17, 2023 15:49
lib/dune_file.ml Outdated Show resolved Hide resolved
lib/dune_file.ml Outdated Show resolved Hide resolved
lib/dune_file.ml Outdated Show resolved Hide resolved
in
Set.mem test_against keep

let rename_library t ~keep renames stanzas =
Copy link
Collaborator

Choose a reason for hiding this comment

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

One trick I recently learned is that you can patch the dune file of a subdirectory without writing in that directory: you can mark that directory as data_only_dirs and use (subdir) to set the contents of the (virtual) dune file in there. I don't know if that applies to that case, but that can be useful if you don't want to change the contents of the unpacked tarball.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's an interesting trick, but I can unfortunately see a number of problems with nested directories where I need to modify foo/dune but there is a foo/bar/dune that would has to stay the same (or be also moved up using subdir).

Rewriting it this way I can't easily ignore dune files in OCaml (and hoping they don't create issues). The cleanest way would probably be to collect all dune stanzas and dump them into a top level dune file but then I would also need to know which dune-files are actually being evaluated (e.g. no point to collect dune files from data_only_dirs which might actually break the build if collected). I feel this is getting dangerously close to a half-baked implementation of the rule-discovery logic of Dune itself.

But this does point at the necessity of eventually implementing such a feature in Dune itself.

lib/pull.ml Outdated Show resolved Hide resolved
This is a bit better than the long tuple with somewhat unclear
semantics.
This has the advantage that the rewriting step is reproducible.
@Leonidas-from-XIV Leonidas-from-XIV merged commit 4fee1a4 into tarides:main Jan 31, 2023
@Leonidas-from-XIV Leonidas-from-XIV deleted the rewrite-package-names branch January 31, 2023 13:07
Leonidas-from-XIV added a commit to Leonidas-from-XIV/opam-monorepo that referenced this pull request Feb 14, 2023
…-package-names"

This reverts commit 4fee1a4, reversing
changes made to 99c0c0a.
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