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

feat: support libraries with the same name in multiple contexts #10307

Merged
merged 38 commits into from
Apr 1, 2024

Conversation

anmonteiro
Copy link
Collaborator

this is #10179 + my commits

@anmonteiro anmonteiro force-pushed the anmonteiro/multi-context-libs branch 4 times, most recently from a9306df to 02b7186 Compare March 26, 2024 07:01
@anmonteiro anmonteiro marked this pull request as ready for review March 26, 2024 07:29
@anmonteiro
Copy link
Collaborator Author

anmonteiro commented Mar 26, 2024

@rgrinberg @jchavarri let's continue here. This is:

Should be ready for an actual first round of code review now.

src/dune_rules/lib.mli Outdated Show resolved Hide resolved
Copy link
Collaborator

@jchavarri jchavarri left a comment

Choose a reason for hiding this comment

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

Thanks a lot for your work on this! ❤️

I left a few questions, the most relevant one being in lib-collision-public-same-folder.t.

If you want me to work on any of this, please lmk!

src/dune_rules/lib_info.mli Outdated Show resolved Hide resolved
Comment on lines 44 to 45
Error: Library with name "foo" is defined in two folders (_build/default/b
and _build/default/a) and shares a name with library "bar". Either change one
Copy link
Collaborator

Choose a reason for hiding this comment

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

This message is incorrect. foo is not defined in two folders. Maybe it's necessary to have two different ways of handling these errors: one for private name collision and one for public name collision.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

well, the libraries are defined in 2 folders, and share a name. that's what I tried to convey in the new error message. the shared name is bar.foo which we don't surface, but that doesn't make the error message incorrect

Copy link
Member

Choose a reason for hiding this comment

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

I agree the message could be rewarded. Perhaps something like:

This library name "bar.foo" is already defined in b/dune:3

Copy link
Collaborator

Choose a reason for hiding this comment

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

but that doesn't make the error message incorrect

I got confused by this part:

Library with name "foo" is defined in two folders (_build/default/b and _build/default/a)

I saw no library "foo" defined in those two folders, but I was looking at the private names. I guess the source of my confusion comes from never looking at the second part of the public lib name (after the dot) as the library name, but rather the private one.

Error: Library foo is defined twice:
- dune:6
- dune:3
Error: Multiple rules generated for _build/default/foo.cmxs:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I thought the introduction of Id.t (or Sentinel.t) would get rid of this error. This is precisely why I introduced find_invalid in the original PR. The feedback from @rgrinberg was:

find_invalid shouldn't be needed at all if we improve how the library database is constructed. Previously, it was the case that every library database had a unique set of names. So one could go from Library.t to Lib.t just by looking it up using the name. With this PR, such a look up is no longer reliable.

I didn't understand how introducing ids would fix this error at the time, and I am confused now. How can ids help with the problem of public libs? The reason why it happens is because the collision happens on the private name, but to resolve public libraries we use the public_libs db resolve, we never go through the "all libs" db one.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Attempt to fix in anmonteiro#10 (still requires the introduction of Library.private_name, I am not able to see a way around this).

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 think we'll want @rgrinberg to review those changes. I don't know exactly what semantics we want here.

Copy link
Member

Choose a reason for hiding this comment

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

I think the old error was good. The user clearly made a mistake here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

what do you think about comparing by obj_dir instead of exposing the private name (anmonteiro#10 (comment))? I guess that wouldn't cover private names in different directories.

Copy link
Collaborator

@jchavarri jchavarri Mar 27, 2024

Choose a reason for hiding this comment

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

| Library.T { visibility = Public pub; _ } ->
let+ lib = Lib.DB.find public_libs (Public_lib.name pub) in
(match lib with
| None ->
(* Skip hidden or unavailable libraries. TODO we should assert
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 comment still relevant?

with
| None ->
Memo.return (With_multiple_results.resolve_result Resolve_result.not_found)
| Some [] -> assert false
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think there's something like Code_error that could be used here instead of plain assert?

| Library lib | Hidden_library lib ->
let info = Lib.info lib in
Lib_info.sentinel info
| Deprecated_library_name _ -> assert false
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this impossible?

src/dune_rules/lib.ml Show resolved Hide resolved
@jchavarri
Copy link
Collaborator

In 86bd245 I removed some changes that were copied over from early work, but are not necessary. The diff in dune_package.ml(i) is now cleaner and the total diff size of the PR went down a bit (from +935 −305 to +921 −292).

src/dune_rules/lib.ml Outdated Show resolved Hide resolved
@rgrinberg
Copy link
Member

There's still something fishy about we're handling Ml_sources.t. E.g. consider this:

diff --git a/test/blackbox-tests/test-cases/lib-collision/lib-collision-private-same-folder.t b/test/blackbox-tests/test-cases/lib-collision/lib-collision-private-same-folder.t
index 9c7391ac9..706869002 100644
--- a/test/blackbox-tests/test-cases/lib-collision/lib-collision-private-same-folder.t
+++ b/test/blackbox-tests/test-cases/lib-collision/lib-collision-private-same-folder.t
@@ -7,7 +7,9 @@ the same folder.
 
   $ cat > dune << EOF
   > (library
-  >  (name foo))
+  >  (name foo)
+  >  (libraries xxx)
+  >  (optional))
   > (library
   >  (name foo))
   > EOF
@@ -15,9 +17,9 @@ the same folder.
 Without any consumers of the libraries
 
   $ dune build
-  File "dune", line 3, characters 0-21:
-  3 | (library
-  4 |  (name foo))
+  File "dune", line 5, characters 0-21:
+  5 | (library
+  6 |  (name foo))
   Error: Library "foo" appears for the second time in this directory
   [1]

I would imagine that this should work. Not necessarily blocking this PR, but this code still needs tuning.

else Lib.DB.Resolve_result.not_found
| Some (Found lib) -> Memo.return (Lib.DB.Resolve_result.found lib)
| Some (Deprecated_library_name lib) ->
Memo.return (Lib.DB.Resolve_result.redirect_in_the_same_db lib)
Copy link
Member

Choose a reason for hiding this comment

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

Now that I think about it, this seems wrong. We should be redirecting the new_public_name into the public lib DB rather then the same db. This isn't an issue introduced by this PR though.

src/dune_rules/scope.ml Outdated Show resolved Hide resolved
@anmonteiro anmonteiro force-pushed the anmonteiro/multi-context-libs branch from 4ec27c2 to ec2fbd2 Compare March 30, 2024 06:38
@anmonteiro
Copy link
Collaborator Author

Thanks for your latest round of review. I addressed your comments in the latest commits.

I made a note to fix both those cases (1. the (optional) case; 2. the case where we should be redirecting to the public DB) once this diff is merged.

@anmonteiro
Copy link
Collaborator Author

Removing library IDs from Deprecated_library_name causes some test failures, because we're using name -> id -> resolved_library for name resolution. I believe the simplest approach is adding them back.

@rgrinberg
Copy link
Member

rgrinberg commented Mar 30, 2024

Which tests fail? I believe they'd be incorrect. Consider this dune file:

(deprecated_library_name (old_public_name a) (new_public_name b))
(library (public_name b) (enabled_if false))
(library (name x) (libraries a))

Where we have an external library b that is found. Where should the a resolve? If you're following the id it should be the disabled library in the dune file - which is wrong.

@anmonteiro
Copy link
Collaborator Author

Which tests fail?

the ones around deprecated_library_name in test/blackbox-tests/test-cases/deprecated-library-name/features.t

@anmonteiro
Copy link
Collaborator Author

Where we have an external library b that is found. Where should the a resolve? If you're following the id it should be the disabled library in the dune file - which is wrong.

I believe what's failing is the simplest case possible: (deprecated_library_name (old_public_name a) (new_public_name b)) and (libraries a) somewhere.

Since we resolve name -> library_id, and there's no such entry in the map for deprecated libraries, we stopped learning how to resolve a.

Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>
Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>
Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>
Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>
Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>
@anmonteiro anmonteiro force-pushed the anmonteiro/multi-context-libs branch from c6414a4 to 7e992fc Compare April 1, 2024 22:54
@anmonteiro
Copy link
Collaborator Author

Thank you @rgrinberg @jchavarri for the guidance and reviews here. I've added a changelog entry and will merge upon a green CI build.

@anmonteiro anmonteiro merged commit 73c224d into ocaml:main Apr 1, 2024
26 of 27 checks passed
@anmonteiro anmonteiro deleted the anmonteiro/multi-context-libs branch April 1, 2024 23:16
anmonteiro added a commit to anmonteiro/dune that referenced this pull request Apr 2, 2024
Since ocaml#10307, dune started allowing libraries to share the same name.
Thus, checking `enabled_if` isn't enough for libraries; dune now needs
to check whether the library is available before tracking it in
ml_sources

Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>
@anmonteiro
Copy link
Collaborator Author

btw, I ran this change through nix-overlays across all packages (nix-ocaml/nix-overlays#1365) and there are no regressions.

anmonteiro added a commit to anmonteiro/dune that referenced this pull request Apr 2, 2024
Since ocaml#10307, dune started allowing libraries to share the same name.
Thus, checking `enabled_if` isn't enough for libraries; dune now needs
to check whether the library is available before tracking it in
ml_sources

Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>
anmonteiro added a commit to anmonteiro/dune that referenced this pull request Apr 2, 2024
Since ocaml#10307, dune started allowing libraries to share the same name.
Thus, checking `enabled_if` isn't enough for libraries; dune now needs
to check whether the library is available before tracking it in
ml_sources

Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>
pmwhite pushed a commit to pmwhite/dune that referenced this pull request Apr 2, 2024
…l#10307)

* feat: support libraries with the same name in multiple contexts


Signed-off-by: Javier Chávarri <javier.chavarri@gmail.com>
Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>
Signed-off-by: Rudi Grinberg <me@rgrinberg.com>
Co-authored-by: Javier Chávarri <javier.chavarri@gmail.com>
Co-authored-by: Rudi Grinberg <me@rgrinberg.com>
anmonteiro added a commit to anmonteiro/dune that referenced this pull request Apr 3, 2024
Since ocaml#10307, dune started allowing libraries to share the same name.
Thus, checking `enabled_if` isn't enough for libraries; dune now needs
to check whether the library is available before tracking it in
ml_sources

Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>
anmonteiro added a commit to anmonteiro/dune that referenced this pull request Apr 3, 2024
Since ocaml#10307, dune started allowing libraries to share the same name.
Thus, checking `enabled_if` isn't enough for libraries; dune now needs
to check whether the library is available before tracking it in
ml_sources

Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>
anmonteiro added a commit to anmonteiro/dune that referenced this pull request Apr 4, 2024
Since ocaml#10307, dune started allowing libraries to share the same name.
Thus, checking `enabled_if` isn't enough for libraries; dune now needs
to check whether the library is available before tracking it in
ml_sources

Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>
anmonteiro added a commit to anmonteiro/dune that referenced this pull request Apr 4, 2024
Since ocaml#10307, dune started allowing libraries to share the same name.
Thus, checking `enabled_if` isn't enough for libraries; dune now needs
to check whether the library is available before tracking it in
ml_sources

Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>
anmonteiro added a commit to anmonteiro/dune that referenced this pull request Apr 8, 2024
Since ocaml#10307, dune started allowing libraries to share the same name.
Thus, checking `enabled_if` isn't enough for libraries; dune now needs
to check whether the library is available before tracking it in
ml_sources

Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>
anmonteiro added a commit to anmonteiro/dune that referenced this pull request Apr 12, 2024
Since ocaml#10307, dune started allowing libraries to share the same name.
Thus, checking `enabled_if` isn't enough for libraries; dune now needs
to check whether the library is available before tracking it in
ml_sources

Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>
anmonteiro added a commit to anmonteiro/dune that referenced this pull request Apr 15, 2024
Since ocaml#10307, dune started allowing libraries to share the same name.
Thus, checking `enabled_if` isn't enough for libraries; dune now needs
to check whether the library is available before tracking it in
ml_sources

Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>
anmonteiro added a commit that referenced this pull request Apr 15, 2024
…#10355)

Since #10307, dune started allowing libraries to share the same name.
Thus, checking `enabled_if` isn't enough for libraries; dune now needs
to check whether the library is available before tracking it in
ml_sources

Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>
gridbugs added a commit to gridbugs/opam-repository that referenced this pull request Jun 5, 2024
CHANGES:

### Added

- allow libraries with the same `(name ..)` in projects as long as they don't
  conflict during resolution (via `enabled_if`). (ocaml/dune#10307, @anmonteiro,
  @jchavarri)

- `dune describe pp` now finds the exact module and the stanza it belongs to,
  instead of guessing the name of the preprocessed file. (ocaml/dune#10321, @anmonteiro)

- Print the result of `dune describe pp` with the respective dialect printer.
  (ocaml/dune#10322, @anmonteiro)

- Add new flag `--context` to `dune ocaml-merlin`, which allows to select a
  Dune context when requesting Merlin config. Add `dune describe contexts`
  subcommand. Introduce a field `generate_merlin_rules` for contexts declared
  in the workspace, that allows to optionally produce Merlin rules for other
  contexts besides the one selected for Merlin (ocaml/dune#10324, @jchavarri)

- melange: add include paths for private library `.cmj` files during JS
  emission. (ocaml/dune#10416, @anmonteiro)

- `dune ocaml-merlin`: communicate additional directives `SOURCE_ROOT`,
  `UNIT_NAME` (the actual name with wrapping) and `INDEX` with the paths to the
  index(es). (ocaml/dune#10422, @voodoos)

- Add a new alias `@ocaml-index` that uses the `ocaml-index` binary to generate
  indexes that can be read by tools such as Merlin to provide project-wide
  references search. (ocaml/dune#10422, @voodoos)

- merlin: add optional `(merlin_reader CMD)` construct to `(dialect)` stanza to
  configure a merlin reader (ocaml/dune#8567, @andreypopp)

### Changed

- melange: treat private libraries with `(package ..)` as public libraries,
  fixing an issue where `import` paths were wrongly emitted. (ocaml/dune#10415,
  @anmonteiro)

- install `.glob` files for Coq theories too (ocaml/dune#10602, @ejgallego)

### Fixed

- Don't try to document non-existent libraries in doc-new target (ocaml/dune#10319, fixes
  ocaml/dune#10056, @jonludlam)

- Make `dune-site`'s `load_all` function look for `META` files so that it
  doesn't fail on empty directories in the plugin directory (ocaml/dune#10458, fixes
  ocaml/dune#10457, @shym)

- Fix incorrect warning for libraries defined inside non-existant directories
  using `(subdir ..)` and used by executables using `dune-build-info` (ocaml/dune#10525,
  @rgrinberg)

- Don't try to take build lock when running `coq top --no-build` (ocaml/dune#10547, fixes
  ocaml/dune#7671, @lzy0505)

- Make sure to truncate dune's lock file after locking and unlocking so that
  users cannot observe incorrect pid's (ocaml/dune#10575, @rgrinberg)

- mdx: link mdx binary with `byte_complete`. This fixes `(libraries)` with
  foreign archives on Linux. (ocaml/dune#10586, fixes ocaml/dune#10582, @anmonteiro)
emillon added a commit to emillon/opam-repository that referenced this pull request Jun 17, 2024
CHANGES:

### Added

- allow libraries with the same `(name ..)` in projects as long as they don't
  conflict during resolution (via `enabled_if`). (ocaml/dune#10307, @anmonteiro,
  @jchavarri)

- `dune describe pp` now finds the exact module and the stanza it belongs to,
  instead of guessing the name of the preprocessed file. (ocaml/dune#10321, @anmonteiro)

- Print the result of `dune describe pp` with the respective dialect printer.
  (ocaml/dune#10322, @anmonteiro)

- Add new flag `--context` to `dune ocaml-merlin`, which allows to select a
  Dune context when requesting Merlin config. Add `dune describe contexts`
  subcommand. Introduce a field `generate_merlin_rules` for contexts declared
  in the workspace, that allows to optionally produce Merlin rules for other
  contexts besides the one selected for Merlin (ocaml/dune#10324, @jchavarri)

- melange: add include paths for private library `.cmj` files during JS
  emission. (ocaml/dune#10416, @anmonteiro)

- `dune ocaml-merlin`: communicate additional directives `SOURCE_ROOT`,
  `UNIT_NAME` (the actual name with wrapping) and `INDEX` with the paths to the
  index(es). (ocaml/dune#10422, @voodoos)

- Add a new alias `@ocaml-index` that uses the `ocaml-index` binary to generate
  indexes that can be read by tools such as Merlin to provide project-wide
  references search. (ocaml/dune#10422, @voodoos)

- merlin: add optional `(merlin_reader CMD)` construct to `(dialect)` stanza to
  configure a merlin reader (ocaml/dune#8567, @andreypopp)

### Changed

- melange: treat private libraries with `(package ..)` as public libraries,
  fixing an issue where `import` paths were wrongly emitted. (ocaml/dune#10415,
  @anmonteiro)

- install `.glob` files for Coq theories too (ocaml/dune#10602, @ejgallego)

### Fixed

- Don't try to document non-existent libraries in doc-new target (ocaml/dune#10319, fixes
  ocaml/dune#10056, @jonludlam)

- Make `dune-site`'s `load_all` function look for `META` files so that it
  doesn't fail on empty directories in the plugin directory (ocaml/dune#10458, fixes
  ocaml/dune#10457, @shym)

- Fix incorrect warning for libraries defined inside non-existant directories
  using `(subdir ..)` and used by executables using `dune-build-info` (ocaml/dune#10525,
  @rgrinberg)

- Don't try to take build lock when running `coq top --no-build` (ocaml/dune#10547, fixes
  ocaml/dune#7671, @lzy0505)

- Make sure to truncate dune's lock file after locking and unlocking so that
  users cannot observe incorrect pid's (ocaml/dune#10575, @rgrinberg)

- mdx: link mdx binary with `byte_complete`. This fixes `(libraries)` with
  foreign archives on Linux. (ocaml/dune#10586, fixes ocaml/dune#10582, @anmonteiro)

- virtual libraries: fix an issue where linking an executable involving several
  virtual libries would cause an error. (ocaml/dune#10581, fixes ocaml/dune#10460, @rgrinberg)
avsm pushed a commit to avsm/opam-repository that referenced this pull request Sep 5, 2024
CHANGES:

### Added

- allow libraries with the same `(name ..)` in projects as long as they don't
  conflict during resolution (via `enabled_if`). (ocaml/dune#10307, @anmonteiro,
  @jchavarri)

- `dune describe pp` now finds the exact module and the stanza it belongs to,
  instead of guessing the name of the preprocessed file. (ocaml/dune#10321, @anmonteiro)

- Print the result of `dune describe pp` with the respective dialect printer.
  (ocaml/dune#10322, @anmonteiro)

- Add new flag `--context` to `dune ocaml-merlin`, which allows to select a
  Dune context when requesting Merlin config. Add `dune describe contexts`
  subcommand. Introduce a field `generate_merlin_rules` for contexts declared
  in the workspace, that allows to optionally produce Merlin rules for other
  contexts besides the one selected for Merlin (ocaml/dune#10324, @jchavarri)

- melange: add include paths for private library `.cmj` files during JS
  emission. (ocaml/dune#10416, @anmonteiro)

- `dune ocaml-merlin`: communicate additional directives `SOURCE_ROOT`,
  `UNIT_NAME` (the actual name with wrapping) and `INDEX` with the paths to the
  index(es). (ocaml/dune#10422, @voodoos)

- Add a new alias `@ocaml-index` that uses the `ocaml-index` binary to generate
  indexes that can be read by tools such as Merlin to provide project-wide
  references search. (ocaml/dune#10422, @voodoos)

- merlin: add optional `(merlin_reader CMD)` construct to `(dialect)` stanza to
  configure a merlin reader (ocaml/dune#8567, @andreypopp)

### Changed

- melange: treat private libraries with `(package ..)` as public libraries,
  fixing an issue where `import` paths were wrongly emitted. (ocaml/dune#10415,
  @anmonteiro)

- install `.glob` files for Coq theories too (ocaml/dune#10602, @ejgallego)

### Fixed

- Don't try to document non-existent libraries in doc-new target (ocaml/dune#10319, fixes
  ocaml/dune#10056, @jonludlam)

- Make `dune-site`'s `load_all` function look for `META` files so that it
  doesn't fail on empty directories in the plugin directory (ocaml/dune#10458, fixes
  ocaml/dune#10457, @shym)

- Fix incorrect warning for libraries defined inside non-existant directories
  using `(subdir ..)` and used by executables using `dune-build-info` (ocaml/dune#10525,
  @rgrinberg)

- Don't try to take build lock when running `coq top --no-build` (ocaml/dune#10547, fixes
  ocaml/dune#7671, @lzy0505)

- Make sure to truncate dune's lock file after locking and unlocking so that
  users cannot observe incorrect pid's (ocaml/dune#10575, @rgrinberg)

- mdx: link mdx binary with `byte_complete`. This fixes `(libraries)` with
  foreign archives on Linux. (ocaml/dune#10586, fixes ocaml/dune#10582, @anmonteiro)

- virtual libraries: fix an issue where linking an executable involving several
  virtual libries would cause an error. (ocaml/dune#10581, fixes ocaml/dune#10460, @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.

3 participants