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: use the correct expander for cram stanzas #10165

Closed
wants to merge 1 commit into from
Closed

fix: use the correct expander for cram stanzas #10165

wants to merge 1 commit into from

Conversation

rgrinberg
Copy link
Member

We should use the expander where the stanza is defined, rather than the
expander of the test.

Signed-off-by: Rudi Grinberg [email protected]

@rgrinberg
fix: use the correct expander for cram stanzas
0968740 
We should use the expander where the stanza is defined, rather than the
expander of the test.

Signed-off-by: Rudi Grinberg <[email protected]>

<!-- ps-id: 56efa496-8491-4481-bb80-044f423de902 -->
@emillon
Copy link
Collaborator

emillon commented Feb 28, 2024

Concretely, how does this problem manifest itself? relative locks are interpreted in the wrong directory?

@rgrinberg
Copy link
Member Author

The expander fetches the values it expands depending on the directory. For example, suppose you have:

(env
 (_
  (setenv (foo bar))
(cram
 (deps %{env:foo=bar}))

The expansion of deps should take into account env in that directory.

It's not the only thing however, other percent forms are interpreted relative to a particular directory.

@rgrinberg
Copy link
Member Author

I realize my explanation above is a little opaque. The programming discipline that I'm trying to convey is pretty important for keeping the dune language consistent, so it deserves a better explanation. It can be stated as:

Suppose we have a percent form %{foo} in foo/dune. We should always expand this percent form using the expander obtained from Super_context.expander ~dir:foo. Using an expander from a different directory is incorrect because the evaluation of the percent form depends on the directory in which it is defined. That's all there is it to it really.

@rgrinberg rgrinberg modified the milestone: 3.15.0 Mar 1, 2024
@rgrinberg
Copy link
Member Author

Something is borked with this PR so I replaced it with #10185

@rgrinberg rgrinberg closed this Mar 1, 2024
@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

@emillon emillon Awaiting requested review from emillon

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

Successfully merging this pull request may close these issues.
2 participants
@rgrinberg @emillon