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

eif: fix name collision in same folder for exes and melange emits #10220

Merged

Conversation

jchavarri
Copy link
Collaborator

@jchavarri jchavarri commented Mar 5, 2024

This applies (somehow) the idea mentioned in #10179 (comment).

Part of the fix for #10222.

In order to keep the duplication check active for actual cases where 2 libs are enabled in the same folder, a new field is added to the group_part type to keep track of the enable state of either libs, exes or melange-emits.

For cases when a collision is detected when both stanzas are enabled, the previous behavior remains.

If one of the stanzas is disabled, then we filter out the disabled stanzas from the list before proceeding. I guess we could filter the stanzas upfront, but I believe keeping the disable ones as much as possible might enable use cases where the user is trying to target the artifact of a disabled stanza, so Dune can error out appropriately (see related #10026). Edit: in the end, the stanzas are filtered upfront as there were not many upsides to do it lazily at the moment.

Signed-off-by: Javier Chávarri <javier.chavarri@gmail.com>
Signed-off-by: Javier Chávarri <javier.chavarri@gmail.com>
src/dune_rules/modules.ml Outdated Show resolved Hide resolved
Signed-off-by: Javier Chávarri <javier.chavarri@gmail.com>
Signed-off-by: Javier Chávarri <javier.chavarri@gmail.com>
Signed-off-by: Javier Chávarri <javier.chavarri@gmail.com>
Signed-off-by: Javier Chávarri <javier.chavarri@gmail.com>
-> libs:(Library.t * _ * Modules.t * Path.Build.t Obj_dir.t) list
-> exes:(_ * _ * Modules.t * Path.Build.t Obj_dir.t) list
-> libs:(Library.t * _ * Modules.t * Path.Build.t Obj_dir.t * Toggle.t) list
-> exes:(_ * _ * Modules.t * Path.Build.t Obj_dir.t * Toggle.t) list
Copy link
Member

Choose a reason for hiding this comment

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

It seems like you pass the toggle to artifacts_obj but you don't do anything with it, any reason for that?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I used it originally, but I guess I can remove it now and just pass the right list of libs, exes and emits directly from the caller, if that's the better approach.

Copy link
Member

@rgrinberg rgrinberg Mar 8, 2024

Choose a reason for hiding this comment

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

I'm not sure if it's the better approach, so I'm looking at the code right to find out. I'll list my thought process below. You'll have to repeat in a few different places, so it might be useful.

First, I need to check where are the callers of Artifacts_obj that use a library name. Looks like there's only one caller of:

val lookup_library : t -> Lib_name.t -> Lib_info.local option

That lives in Expander:

    (match Artifacts_obj.lookup_library artifacts name with
     | None -> does_not_exist ~what:"Library" (Lib_name.to_string name)
     | Some lib ->
       Mode.Dict.get (Lib_info.archives lib) mode
       |> Action_builder.List.map ~f:(fun fn ->
         let fn = Path.build fn in
         let+ () = Action_builder.path fn in
         Value.Path fn))

So now I think about what's going to happen if we pretend that a disabled library does not exist. Since this code is part of the expander, to reproduce this behavior I'd run the following command dune build %{cma:dir/disabled-lib}.

From the perspective of the user, dune telling me this does not exist would be rather undesirable. So we need to improve the situation somehow. To do so, we need Artifacts_obj.lookup_library to propagate the library being disabled.

I make the conclusion that your decision is indeed correct and we need to pass enabled/disabled to Artifacts_obj

Hope that helps.

Copy link
Collaborator Author

@jchavarri jchavarri Mar 8, 2024

Choose a reason for hiding this comment

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

Just to make sure I understand, this process would help on the eventual case that in some future Dune sets "fake" rules for all the disabled libraries in the current context right? As we confirmed the other day that there's no need to set rules for disabled libs right now.

I also wonder, in this hypothetic future where there are fake rules for disabled libraries (for better user errors), how would we deal with the case where two libraries are defined in the same folder with the same name, but one defined on each context? I assume we would skip the creation of fake rules in that case?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, we discussed using fake rules to make the error messages better. And indeed the decision was to forego them for now, as it's quite complex to implement. However, there's other cases where we can improve the error messages without much complexity. Expanding percent forms like we have here is an obvious candidate.

In your hypothetical case we would skip the fake rules. As they would overlap with actual rules.

Copy link
Member

Choose a reason for hiding this comment

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

I added a test case for this in #10245

Looks like we don't handle disabled libraries correctly

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The change requests didn't specify if this is something that should be fixed in this PR. Is that the case? In any case, the PR doesn't seem to change the current behavior.

Copy link
Member

Choose a reason for hiding this comment

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

No, there's no need to fix it here.

@jchavarri
Copy link
Collaborator Author

@rgrinberg Sorry, this PR is not yet ready. The CI failures helped me understand there is something borked on the way setup_emit_js_rules is called (from Gen_rules.gen_rules), because the right context is not taken into account, and so there is a mismatch between that function and Lib_rules.rules (which sets the rules for melange cmj). I still don't fully understand the problem.

Signed-off-by: Javier Chávarri <javier.chavarri@gmail.com>
Signed-off-by: Javier Chávarri <javier.chavarri@gmail.com>
Signed-off-by: Javier Chávarri <javier.chavarri@gmail.com>
@jchavarri jchavarri force-pushed the improve-ml-sources-same-folder-collision branch from ea9def8 to 5189cc4 Compare March 8, 2024 09:30
Signed-off-by: Javier Chávarri <javier.chavarri@gmail.com>
@@ -1,4 +1,4 @@
enabled_if has a limitation: it attempts building even if enabled_if evaluates to false.
enabled_if and build_if have similar behavior
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

cc @emillon. This PR seems to fix the case found in #7899.

Copy link
Member

Choose a reason for hiding this comment

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

The old behavior was in fact correct and this PR breaks it. The description of this test might be the source of confusion. There's no limitation anywhere, it's just that the different semantics were chosen for enabled_if on tests.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should be fixed in 050aed8.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just as a side note, if I'm understanding the current behavior properly, it is quite confusing from a user perspective. enabled_if semantics are now different depending on where it's used:

  • In executable stanzas, the field means the executable is not built if it evaluates to false
  • In test stanzas:
    • when all alias is used, the field means the test executable is always built, regardless its evaluated value
    • but for runtest alias it means the executable will not be built if it evaluates to false (like executable stanzas)

I found this comment that explains the use case for it: #7899 (comment).

It's probably too late, but wonder if it would have been possible to leave the behavior of enabled_if consistent, and allow advanced use cases to use a combination of rule and executable, or add a new field test_if.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, in hindsight it would be better to do it your way. We could revisit it for 4.0

Comment on lines +37 to +39
Error: Alias "melange" specified on the command line is empty.
It is not defined in . or any of its descendants.
[1]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These errors might be more meaningful to users than the previous behavior (not building anything because the alias is "empty", but not erroring either).

Signed-off-by: Javier Chávarri <javier.chavarri@gmail.com>
@@ -45,6 +45,7 @@ module Modules = struct
* (Loc.t * Module.Source.t) Module_trie.t
Copy link
Collaborator

Choose a reason for hiding this comment

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

is it time we make this a record instead? that'd be a whole lot easier to follow what each member of this group_part is.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree. I can do so in a follow-up PR.

src/dune_rules/ml_sources.ml Outdated Show resolved Hide resolved
src/dune_rules/ml_sources.ml Outdated Show resolved Hide resolved
src/dune_rules/ml_sources.ml Outdated Show resolved Hide resolved
src/dune_rules/ml_sources.ml Outdated Show resolved Hide resolved
jchavarri and others added 4 commits March 10, 2024 13:10
Signed-off-by: Javier Chávarri <javier.chavarri@gmail.com>
Co-authored-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>
Signed-off-by: Javier Chávarri <javier.chavarri@gmail.com>
Signed-off-by: Javier Chávarri <javier.chavarri@gmail.com>
Signed-off-by: Javier Chávarri <javier.chavarri@gmail.com>
@@ -1,4 +1,4 @@
enabled_if has a limitation: it attempts building even if enabled_if evaluates to false.
enabled_if and build_if have similar behavior
Copy link
Member

Choose a reason for hiding this comment

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

The old behavior was in fact correct and this PR breaks it. The description of this test might be the source of confusion. There's no limitation anywhere, it's just that the different semantics were chosen for enabled_if on tests.

-> libs:(Library.t * _ * Modules.t * Path.Build.t Obj_dir.t) list
-> exes:(_ * _ * Modules.t * Path.Build.t Obj_dir.t) list
-> libs:(Library.t * _ * Modules.t * Path.Build.t Obj_dir.t * Toggle.t) list
-> exes:(_ * _ * Modules.t * Path.Build.t Obj_dir.t * Toggle.t) list
Copy link
Member

Choose a reason for hiding this comment

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

I added a test case for this in #10245

Looks like we don't handle disabled libraries correctly

Signed-off-by: Javier Chávarri <javier.chavarri@gmail.com>
Signed-off-by: Javier Chávarri <javier.chavarri@gmail.com>
jchavarri and others added 4 commits March 12, 2024 08:05
Signed-off-by: Javier Chávarri <javier.chavarri@gmail.com>
_
Signed-off-by: Rudi Grinberg <me@rgrinberg.com>
_
Signed-off-by: Rudi Grinberg <me@rgrinberg.com>
_
Signed-off-by: Rudi Grinberg <me@rgrinberg.com>
@rgrinberg
Copy link
Member

I improved the PR as follows:

  1. stop propagating enabled_if when they aren't being used. we should propagate them to improve error messages in the future, but we can save that until its actually needed.
  2. correctly handle build_if for tests
  3. avoid evaluating enabled_if on melange.emit unless it's needed (to avoid possible cycles)
  4. do not compute modules for disabled stanzas.

Should be good once you add a CHANGES entry

@rgrinberg rgrinberg added this to the 3.15.0 milestone Mar 12, 2024
Signed-off-by: Javier Chávarri <javier.chavarri@gmail.com>
Signed-off-by: Javier Chávarri <javier.chavarri@gmail.com>
Signed-off-by: Javier Chávarri <javier.chavarri@gmail.com>
@jchavarri
Copy link
Collaborator Author

@rgrinberg Thanks for the improvements!

I updated the tests in b22ae6f, and added a changelog entry. Please merge if everything looks fine.

@rgrinberg rgrinberg merged commit ffa6e1f into ocaml:main Mar 13, 2024
27 checks passed
@jchavarri jchavarri deleted the improve-ml-sources-same-folder-collision branch March 13, 2024 14:14
Leonidas-from-XIV added a commit to Leonidas-from-XIV/opam-repository that referenced this pull request Mar 26, 2024
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
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)
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.

None yet

3 participants