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.

Sign up for GitHub

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

Jump to bottom

fix(subst): subst for opam files in opam/ subdir #9895

Merged
merged 2 commits into from
Feb 8, 2024
Merged

fix(subst): subst for opam files in opam/ subdir #9895

merged 2 commits into from
Feb 8, 2024

Conversation

emillon
Copy link
Collaborator

@emillon emillon commented Feb 1, 2024 •
edited
Loading

Fixes #9862

@emillon emillon marked this pull request as ready for review February 5, 2024 10:15
rgrinberg
rgrinberg approved these changes Feb 5, 2024
View reviewed changes
Copy link
Member

@rgrinberg rgrinberg left a comment

Choose a reason for hiding this comment

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

The fix looks fine, but the implementation details of opam file locations are now split across 3 modules (Subst, Opam_create, and Package). Ideally, it would all exits in a single module, but at least you can make this less fragile but collecting all the opam files for the project with the help of Dune_project.packages and Package.opam_file and then get rid of this Package.is_opam_file function altogether.

@emillon
Copy link
Collaborator Author

emillon commented Feb 6, 2024

I was wondering about this as well. But I'm not sure that Dune_project is the right place to do that: we want to run the substitutions only at Path.root, not for opam files of vendored packages. Should we use Scope for that? Or move the functions to Dune_project as you said but only act if the project is defined at Path.root?

@rgrinberg
Copy link
Member

We're already going to run substitutions only for the root project as we're loading the project only in Path.root.

Note that my suggestion does not involve modifying dune_project.ml at all. Everything can be implemented on top of existing api's in bin/subst.ml. All you should need is Dune_project.packages and Package.opam_file.

@rgrinberg rgrinberg added this to the 3.14.0 milestone Feb 8, 2024
emillon and others added 2 commits February 8, 2024 12:38
@emillon @rgrinberg
fix(subst): handle opam files in opam directory
8886b25 
Fixes ocaml#9862

Signed-off-by: Etienne Millon <me@emillon.org>
@rgrinberg
refactor: [Vcs.files] should return source paths
fce4761 
Signed-off-by: Rudi Grinberg <me@rgrinberg.com>
@rgrinberg rgrinberg merged commit 0c92dc9 into ocaml:main Feb 8, 2024
24 of 27 checks passed
@rgrinberg
Copy link
Member

Ended up pushing the improvement. Btw, the test did not define a package in the project file which was wrong. We only support opam files in opam/ with corresponding package stanzas.

@emillon
Copy link
Collaborator Author

emillon commented Feb 9, 2024

Thanks Rudi!

@emillon emillon mentioned this pull request Feb 9, 2024
Closed
emillon added a commit to emillon/opam-repository that referenced this pull request Feb 9, 2024
@emillon
[new release] dune (15 packages) (3.14.0~alpha1)
5f67c5f 
CHANGES:

### Added

- Introduce a `(dynamic_include ..)` stanza. This is like `(include foo)` but
  allows `foo` to be the target of a rule. Currently, there are some
  limitations on the stanzas that can be generated. For example, public
  executables, libraries are currently forbidden. (ocaml/dune#9913, @rgrinberg)

- Introduce `$ dune promotion list` to print the list of available promotions.
  (ocaml/dune#9705, @moyodiallo)

- If Sherlodoc is installed, add a search bar in generated HTML docs (ocaml/dune#9772,
  @EmileTrotignon)

- Add `only_sources` field to `copy_files` stanza (ocaml/dune#9827, fixes ocaml/dune#9709,
  @jchavarri)

- The `(foreign_library)` stanza now supports the `(enabled_if)` field. (ocaml/dune#9914,
  @nojb)

### Fixed

- Fix `$ dune install -p` incorrectly recognizing packages that are supposed to
  be filtered (ocaml/dune#9879, fixes ocaml/dune#4814, @rgrinberg)

- subst: correctly handle opam files in opam/ subdirectory (ocaml/dune#9895, fixes ocaml/dune#9862,
  @emillon)

- Odoc private rules are not set up if a library is not available due to
  `enabled_if` (ocaml/dune#9897, @rgrinberg and @jchavarri)

### Changed

- When dune language 3.14 is enabled, resolve the binary in `(run %{bin:..}
  ..)` from where the binary is built. (ocaml/dune#9708, @rgrinberg)

- boot: remove single-command bootstrap. This was an alternative bootstrap
  strategy that was used in certain conditions. Removal makes the bootstrap a
  bit slower on Linux when only a single core is available, but bootstrap is
  now reproducible in all cases. (ocaml/dune#9735, fixes ocaml/dune#9507, @emillon)
emillon added a commit to emillon/opam-repository that referenced this pull request Feb 12, 2024
@emillon
[new release] dune (15 packages) (3.14.0)
bb57fde 
CHANGES:

### Added

- Introduce a `(dynamic_include ..)` stanza. This is like `(include foo)` but
  allows `foo` to be the target of a rule. Currently, there are some
  limitations on the stanzas that can be generated. For example, public
  executables, libraries are currently forbidden. (ocaml/dune#9913, @rgrinberg)

- Introduce `$ dune promotion list` to print the list of available promotions.
  (ocaml/dune#9705, @moyodiallo)

- If Sherlodoc is installed, add a search bar in generated HTML docs (ocaml/dune#9772,
  @EmileTrotignon)

- Add `only_sources` field to `copy_files` stanza (ocaml/dune#9827, fixes ocaml/dune#9709,
  @jchavarri)

- The `(foreign_library)` stanza now supports the `(enabled_if)` field. (ocaml/dune#9914,
  @nojb)

### Fixed

- Fix `$ dune install -p` incorrectly recognizing packages that are supposed to
  be filtered (ocaml/dune#9879, fixes ocaml/dune#4814, @rgrinberg)

- subst: correctly handle opam files in opam/ subdirectory (ocaml/dune#9895, fixes ocaml/dune#9862,
  @emillon)

- Odoc private rules are not set up if a library is not available due to
  `enabled_if` (ocaml/dune#9897, @rgrinberg and @jchavarri)

### Changed

- When dune language 3.14 is enabled, resolve the binary in `(run %{bin:..}
  ..)` from where the binary is built. (ocaml/dune#9708, @rgrinberg)

- boot: remove single-command bootstrap. This was an alternative bootstrap
  strategy that was used in certain conditions. Removal makes the bootstrap a
  bit slower on Linux when only a single core is available, but bootstrap is
  now reproducible in all cases. (ocaml/dune#9735, fixes ocaml/dune#9507, @emillon)
@emillon emillon mentioned this pull request Feb 12, 2024
Merged
@Alizter
Copy link
Collaborator

Alizter commented May 2, 2024

FTR this broke Coq's CI upstream. See https://coq.zulipchat.com/#narrow/stream/237656-Coq-devs-.26-plugin-devs/topic/.5BERROR.5D.20The.20comp**.20of.20coq-core.2Edev.20failed.20at.20.22dune.20subst.22.2E

@Alizter
Copy link
Collaborator

Alizter commented May 2, 2024

The issue is that there were opam files in the home directory which didn't get subbed before now they are. https://github.com/coq/opam/blob/master/.gitlab-ci.yml#L15

nberth pushed a commit to nberth/opam-repository that referenced this pull request Jun 18, 2024
@emillon @nberth
[new release] dune (15 packages) (3.14.0)
70a8164 
CHANGES:

### Added

- Introduce a `(dynamic_include ..)` stanza. This is like `(include foo)` but
  allows `foo` to be the target of a rule. Currently, there are some
  limitations on the stanzas that can be generated. For example, public
  executables, libraries are currently forbidden. (ocaml/dune#9913, @rgrinberg)

- Introduce `$ dune promotion list` to print the list of available promotions.
  (ocaml/dune#9705, @moyodiallo)

- If Sherlodoc is installed, add a search bar in generated HTML docs (ocaml/dune#9772,
  @EmileTrotignon)

- Add `only_sources` field to `copy_files` stanza (ocaml/dune#9827, fixes ocaml/dune#9709,
  @jchavarri)

- The `(foreign_library)` stanza now supports the `(enabled_if)` field. (ocaml/dune#9914,
  @nojb)

### Fixed

- Fix `$ dune install -p` incorrectly recognizing packages that are supposed to
  be filtered (ocaml/dune#9879, fixes ocaml/dune#4814, @rgrinberg)

- subst: correctly handle opam files in opam/ subdirectory (ocaml/dune#9895, fixes ocaml/dune#9862,
  @emillon)

- Odoc private rules are not set up if a library is not available due to
  `enabled_if` (ocaml/dune#9897, @rgrinberg and @jchavarri)

### Changed

- When dune language 3.14 is enabled, resolve the binary in `(run %{bin:..}
  ..)` from where the binary is built. (ocaml/dune#9708, @rgrinberg)

- boot: remove single-command bootstrap. This was an alternative bootstrap
  strategy that was used in certain conditions. Removal makes the bootstrap a
  bit slower on Linux when only a single core is available, but bootstrap is
  now reproducible in all cases. (ocaml/dune#9735, fixes ocaml/dune#9507, @emillon)
@silene silene mentioned this pull request Jun 19, 2024
Merged
erikmd added a commit to erikmd/opam-coq-archive that referenced this pull request Jun 19, 2024
@erikmd
fix: Workaround ocaml/dune#9895 (comment)
4e4ce82
skylee03 added a commit to skylee03/opam that referenced this pull request Jun 19, 2024
@skylee03
fix: workaround ocaml/dune#9895
a4cce95
@erikmd erikmd mentioned this pull request Jul 5, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rgrinberg rgrinberg rgrinberg approved these changes

@Leonidas-from-XIV Leonidas-from-XIV Awaiting requested review from Leonidas-from-XIV

@ElectreAAS ElectreAAS Awaiting requested review from ElectreAAS

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
3.14.0
Development

Successfully merging this pull request may close these issues.

dune subst and opam/ subfolder
3 participants
@emillon @rgrinberg @Alizter