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: allow looking up artifacts everywhere #10169

Merged
merged 1 commit into from
Feb 29, 2024
Merged

fix: allow looking up artifacts everywhere #10169

merged 1 commit into from
Feb 29, 2024

Conversation

rgrinberg
Copy link
Member

@rgrinberg rgrinberg commented Feb 29, 2024 •
edited
Loading

Before this PR, we had a function to fetch the expansion for a directory:

Super_context.expander : dir:Path.Build.t -> Expander.t

The expander returned was incomplete however. In particular, it wasn't able to expand forms such as %{cma:..}. The end result is that whether forms such as %{cma:..} were expandable in a particular field in a dune file was basically random.

After this PR, we the expander returned by the function above is always able to expand these forms. So this makes the dune language both more powerful and more consistent.

@rgrinberg rgrinberg force-pushed the ps/rr/fix__allow_looking_up_artifacts_everywhere branch from 4059119 to 3bfb062 Compare February 29, 2024 07:51
@rgrinberg rgrinberg requested a review from nojb February 29, 2024 11:38
@nojb
Copy link
Collaborator

nojb commented Feb 29, 2024

Can you say a few words about what the PR is doing? Thanks!

@rgrinberg
Copy link
Member Author

Updated the description

nojb
nojb approved these changes Feb 29, 2024
View reviewed changes
Copy link
Collaborator

@nojb nojb left a comment

Choose a reason for hiding this comment

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

Looks like a nice cleanup. LGTM, modulo a question (below).

src/dune_rules/expander.ml
Comment on lines +163 to +164
if Path.Build.is_root dir
then User_error.raise ~loc [ Pp.text "cannot escape the workspace root directory" ];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this check as general as it could be? What happens if we write %{cma:../../../foo} in the test (so that we go way outside the root directory)?

Copy link
Member Author

Choose a reason for hiding this comment

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

I added a test - It doesn't go through.

By the way, there's still a way to do something illegal here. One can write:

%{cma:../another-context/foo}

And use the relative path to escape to another context. We should definitely forbid that as well. It's a much larger change though.

@rgrinberg rgrinberg force-pushed the ps/rr/fix__allow_looking_up_artifacts_everywhere branch from 3bfb062 to 4cf7445 Compare February 29, 2024 17:41
@rgrinberg rgrinberg mentioned this pull request Feb 29, 2024
Open
@rgrinberg rgrinberg added this to the 3.15.0 milestone Feb 29, 2024
@rgrinberg rgrinberg force-pushed the ps/rr/fix__allow_looking_up_artifacts_everywhere branch from 4cf7445 to be7e2a7 Compare February 29, 2024 17:56
@rgrinberg
fix: allow looking up artifacts everywhere
af39ab8 
Signed-off-by: Rudi Grinberg <me@rgrinberg.com>

<!-- ps-id: f31ed293-c931-4432-a1a4-258c7f7570cf -->
@rgrinberg rgrinberg force-pushed the ps/rr/fix__allow_looking_up_artifacts_everywhere branch from be7e2a7 to af39ab8 Compare February 29, 2024 18:22
@rgrinberg rgrinberg merged commit 7bf04fa into main Feb 29, 2024
26 of 27 checks passed
@rgrinberg rgrinberg deleted the ps/rr/fix__allow_looking_up_artifacts_everywhere branch February 29, 2024 19:12
@Leonidas-from-XIV Leonidas-from-XIV mentioned this pull request Mar 26, 2024
Closed
Leonidas-from-XIV added a commit to Leonidas-from-XIV/opam-repository that referenced this pull request Mar 26, 2024
@Leonidas-from-XIV
[new release] dune (15 packages) (3.15.0~alpha1)
68b4763 
CHANGES:

### Added

- Add link flags to to `ocamlmklib` for ctypes stubs (ocaml/dune#8784, @frejsoya)

- Remove some unnecessary limitations in the expansions of percent forms in
  install stanza. For example, the `%{env:..}` form can be used to select files
  to be installed. (ocaml/dune#10160, @rgrinberg)

- Allow artifact expansion percent forms (`%{cma:..}`, `%{cmo:..}`, etc.) in
  more contexts. Previously, they would be randomly forbidden in some fields.
  (ocaml/dune#10169, @rgrinberg)

- Allow `%{inline_tests}` in more contexts (ocaml/dune#10191, @rgrinberg)

- Remove limitations on percent forms in the `(enabled_if ..)` field of
  libraries (ocaml/dune#10250, @rgrinberg)

- Support dialects in `dune describe pp` (ocaml/dune#10283, @emillon)

- Allow defining executables or melange emit stanzas with the same name in the
  same folder under different contexts. (ocaml/dune#10220, @rgrinberg, @jchavarri)

### Fixed

- coq: Delay Coq rule setup checks so OCaml-only packages can build in hybrid
  Coq/OCaml projects when `coqc` is not present. Thanks to @vzaliva for the
  test case and report (ocaml/dune#9845, fixes ocaml/dune#9818, @rgrinberg, @ejgallego)

- Fix conditional source selection with `select` on `bigarray` in OCaml 5
  (ocaml/dune#10011, @moyodiallo)

- melange: fix inconsistency in virtual library implementation. Concrete
  modules within a virtual library can now refer to its virtual modules too
  (ocaml/dune#10051, fixes ocaml/dune#7104, @anmonteiro)

- melange: fix a bug that would cause stale `import` paths to be emitted when
  moving source files within `(include_subdirs ..)` (ocaml/dune#10286, fixes ocaml/dune#9190,
  @anmonteiro)

- Dune file formatting: output utf8 if input is correctly encoded (ocaml/dune#10113,
  fixes ocaml/dune#9728, @moyodiallo)

- Fix expanding dependencies and locks specified in the cram stanza.
  Previously, they would be installed in the context of the cram test, rather
  than the cram stanza itself (ocaml/dune#10165, @rgrinberg)

- Fix bug with `dune exec --watch` where the working directory would always be
  set to the project root rather than the directory where the command was run
  (ocaml/dune#10262, @gridbugs)

- Regression fix: sign executables that are promoted into the source tree
  (ocaml/dune#10263, fixes ocaml/dune#9272, @emillon)

- Fix crash when decoding dune-package for libraries with `(include_subdirs
  qualified)` (ocaml/dune#10269, fixes ocaml/dune#10264, @emillon)

### Changed

- Remove the `--react-to-insignificant-changes` option. (ocaml/dune#10083, @rgrinberg)
Leonidas-from-XIV added a commit to Leonidas-from-XIV/opam-repository that referenced this pull request Apr 3, 2024
@Leonidas-from-XIV
[new release] dune (15 packages) (3.15.0)
950c529 
CHANGES:

### Added

- Add link flags to to `ocamlmklib` for ctypes stubs (ocaml/dune#8784, @frejsoya)

- Remove some unnecessary limitations in the expansions of percent forms in
  install stanza. For example, the `%{env:..}` form can be used to select files
  to be installed. (ocaml/dune#10160, @rgrinberg)

- Allow artifact expansion percent forms (`%{cma:..}`, `%{cmo:..}`, etc.) in
  more contexts. Previously, they would be randomly forbidden in some fields.
  (ocaml/dune#10169, @rgrinberg)

- Allow `%{inline_tests}` in more contexts (ocaml/dune#10191, @rgrinberg)

- Remove limitations on percent forms in the `(enabled_if ..)` field of
  libraries (ocaml/dune#10250, @rgrinberg)

- Support dialects in `dune describe pp` (ocaml/dune#10283, @emillon)

- Allow defining executables or melange emit stanzas with the same name in the
  same folder under different contexts. (ocaml/dune#10220, @rgrinberg, @jchavarri)

### Fixed

- coq: Delay Coq rule setup checks so OCaml-only packages can build in hybrid
  Coq/OCaml projects when `coqc` is not present. Thanks to @vzaliva for the
  test case and report (ocaml/dune#9845, fixes ocaml/dune#9818, @rgrinberg, @ejgallego)

- Fix conditional source selection with `select` on `bigarray` in OCaml 5
  (ocaml/dune#10011, @moyodiallo)

- melange: fix inconsistency in virtual library implementation. Concrete
  modules within a virtual library can now refer to its virtual modules too
  (ocaml/dune#10051, fixes ocaml/dune#7104, @anmonteiro)

- melange: fix a bug that would cause stale `import` paths to be emitted when
  moving source files within `(include_subdirs ..)` (ocaml/dune#10286, fixes ocaml/dune#9190,
  @anmonteiro)

- Dune file formatting: output utf8 if input is correctly encoded (ocaml/dune#10113,
  fixes ocaml/dune#9728, @moyodiallo)

- Fix expanding dependencies and locks specified in the cram stanza.
  Previously, they would be installed in the context of the cram test, rather
  than the cram stanza itself (ocaml/dune#10165, @rgrinberg)

- Fix bug with `dune exec --watch` where the working directory would always be
  set to the project root rather than the directory where the command was run
  (ocaml/dune#10262, @gridbugs)

- Regression fix: sign executables that are promoted into the source tree
  (ocaml/dune#10263, fixes ocaml/dune#9272, @emillon)

- Fix crash when decoding dune-package for libraries with `(include_subdirs
  qualified)` (ocaml/dune#10269, fixes ocaml/dune#10264, @emillon)

### Changed

- Remove the `--react-to-insignificant-changes` option. (ocaml/dune#10083, @rgrinberg)
@Leonidas-from-XIV Leonidas-from-XIV mentioned this pull request Apr 3, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nojb nojb nojb approved these changes

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

Successfully merging this pull request may close these issues.

None yet
2 participants
@rgrinberg @nojb