-
Notifications
You must be signed in to change notification settings - Fork 413
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
feat: support libraries with the same name in multiple contexts #10307
Conversation
a9306df
to
02b7186
Compare
@rgrinberg @jchavarri let's continue here. This is:
Should be ready for an actual first round of code review now. |
There was a problem hiding this 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!
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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: |
There was a problem hiding this comment.
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 fromLibrary.t
toLib.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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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?
src/dune_rules/scope.ml
Outdated
with | ||
| None -> | ||
Memo.return (With_multiple_results.resolve_result Resolve_result.not_found) | ||
| Some [] -> assert false |
There was a problem hiding this comment.
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?
src/dune_rules/dune_package.ml
Outdated
| Library lib | Hidden_library lib -> | ||
let info = Lib.info lib in | ||
Lib_info.sentinel info | ||
| Deprecated_library_name _ -> assert false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this impossible?
In 86bd245 I removed some changes that were copied over from early work, but are not necessary. The diff in |
There's still something fishy about we're handling
I would imagine that this should work. Not necessarily blocking this PR, but this code still needs tuning. |
src/dune_rules/scope.ml
Outdated
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) |
There was a problem hiding this comment.
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.
test/blackbox-tests/test-cases/lib-collision/lib-collision-private.t
Outdated
Show resolved
Hide resolved
4ec27c2
to
ec2fbd2
Compare
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 |
Removing library IDs from |
Which tests fail? I believe they'd be incorrect. Consider this dune file:
Where we have an external library |
the ones around deprecated_library_name in |
I believe what's failing is the simplest case possible: Since we resolve name -> library_id, and there's no such entry in the map for deprecated libraries, we stopped learning how to resolve |
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>
c6414a4
to
7e992fc
Compare
Thank you @rgrinberg @jchavarri for the guidance and reviews here. I've added a changelog entry and will merge upon a green CI build. |
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>
btw, I ran this change through nix-overlays across all packages (nix-ocaml/nix-overlays#1365) and there are no regressions. |
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>
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>
…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>
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>
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>
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>
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>
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>
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>
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>
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)
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)
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)
this is #10179 + my commits