fix: redirect deprecated libraries resolution to the public lib DB #10354

anmonteiro · 2024-04-01T23:31:50Z

addresses this comment

anmonteiro · 2024-04-01T23:33:32Z

test/blackbox-tests/test-cases/lib-collision/lib-collision-public-same-public-name.t

@@ -23,12 +23,12 @@ different folders.
 Without any consumers of the libraries

  $ dune build


this error becomes better: we now use the loc for the (public_name ..) field, which is the actual conflict.

jchavarri

I'm afraid I don't fully understand what the change in redirection to parent is fixing (besides the better locations in errors mentioned). But otherwise lgtm.

src/dune_rules/lib.ml

jchavarri · 2024-04-02T08:55:40Z

src/dune_rules/scope.ml

@@ -85,13 +85,13 @@ module DB = struct
      else Lib.DB.Resolve_result.not_found
    | Found lib -> Memo.return (Lib.DB.Resolve_result.found lib)
    | Deprecated_library_name lib ->
-      Memo.return (Lib.DB.Resolve_result.redirect_in_the_same_db lib)
+      Memo.return (Lib.DB.Resolve_result.redirect parent lib)


Does this change unlocks new possibilities? I can see that the behavior exercised in features.t test suite isn't modified. Maybe there's some case with some deprecation of a library into a lib only available in the public lib db that would be affected by this change?

I believe this change is just saving us 1 redirect, because the new public name will exist in the public libs DB. We'd redirect anyway at resolution time (following 2 redirects, now just 1)

src/dune_rules/lib.ml

anmonteiro · 2024-04-02T20:27:25Z

Isn't this constructor also used for redirecting public names to private names?

I think this might be true. We may have to specify whether we're redirecting to a name or a library ID.

rgrinberg · 2024-04-02T20:29:34Z

Just to make sure we're on the same page, I think we need two constructors:

Redirect of Lib_name.t * DB.t used for implementing (deprecated_library_stanza ..). The target db here should be the public db
Redirect of Lib_id.t used for implementing public to private redirects.

anmonteiro · 2024-04-02T20:31:30Z

@rgrinberg I need to dig deeper into the redirect business, but we may have that already? Lib.DB.Resolve_result.redirect takes a DB and a Lib_id.t

EDIT: 🤦 except I changed that in this PR. I see what you mean.

anmonteiro · 2024-04-02T21:43:57Z

OK I added a new Redirect_to_lib_id constructor that handles the redirection for local stanzas.

I also added visibility in Lib_id to get the "best loc" for a stanza. This way we make sure to keep showing the correct loc for the public name clash case in tests.

src/dune_rules/lib.mli

rgrinberg · 2024-04-03T10:10:33Z

src/dune_rules/lib_id.ml

@@ -18,7 +23,7 @@ module Local = struct
      | x -> x
    ;;


I missed this from before, but this seems wrong. How is the comparison function ignoring the other elements?

My thought was that, since this refers to Local library IDs, the loc would be more than enough to distinguish different lib_ids.

The only field we're not comparing is enabled_if. We could, but I couldn't come up with a compare function for Blang.t -- there's only Blang.equal.

My thought was that, since this refers to Local library IDs, the loc would be more than enough to distinguish different lib_ids.

In that case, I would drop the enabled_if from the id as well.

rgrinberg · 2024-04-03T10:11:12Z

src/dune_rules/lib_id.ml

@@ -1,9 +1,14 @@
 open Import

+type visibility =


Could you split this change into a separate PR? I think adding not strictly necessary fields to Lib_id isn't something we should do lightly.

rgrinberg · 2024-04-03T10:12:34Z

src/dune_rules/scope.ml

@@ -85,13 +85,13 @@ module DB = struct
      else Lib.DB.Resolve_result.not_found
    | Found lib -> Memo.return (Lib.DB.Resolve_result.found lib)
    | Deprecated_library_name lib ->
-      Memo.return (Lib.DB.Resolve_result.redirect_in_the_same_db lib)
+      Memo.return (Lib.DB.Resolve_result.redirect parent lib)


Is parent always the public library database here?

yes. I'll move these functions more closely together and make that explicit in a comment

it adds a bit of whitespace diff noise, but I did the above in 3e73083

rgrinberg · 2024-04-04T14:01:23Z

src/dune_rules/scope.ml

+  let create_db_from_stanzas =
+    (* Here, [parent] is always the public_libs DB. Check the call to
+       [create_db_from_stanzas] below. *)
+    let resolve_found_or_redirect ~parent fr =


How about just rename ~parent to ~public_db then?

Signed-off-by: Antonio Nuno Monteiro <[email protected]>

they're used Signed-off-by: Antonio Nuno Monteiro <[email protected]>

Signed-off-by: Antonio Nuno Monteiro <[email protected]>

anmonteiro requested review from rgrinberg and jchavarri April 1, 2024 23:31

anmonteiro commented Apr 1, 2024

View reviewed changes

anmonteiro force-pushed the anmonteiro/deprecated-name-redirect-to-public branch from 7947efa to dc6ced7 Compare April 2, 2024 00:39

jchavarri approved these changes Apr 2, 2024

View reviewed changes

rgrinberg reviewed Apr 2, 2024

View reviewed changes

src/dune_rules/lib.ml Outdated Show resolved Hide resolved

anmonteiro force-pushed the anmonteiro/deprecated-name-redirect-to-public branch from dc6ced7 to 3b1fea4 Compare April 2, 2024 21:42

rgrinberg reviewed Apr 3, 2024

View reviewed changes

src/dune_rules/lib.mli Outdated Show resolved Hide resolved

rgrinberg reviewed Apr 3, 2024

View reviewed changes

src/dune_rules/lib.mli Outdated Show resolved Hide resolved

rgrinberg reviewed Apr 3, 2024

View reviewed changes

anmonteiro force-pushed the anmonteiro/deprecated-name-redirect-to-public branch from ec08819 to 6395ddd Compare April 3, 2024 21:32

rgrinberg approved these changes Apr 4, 2024

View reviewed changes

anmonteiro added 6 commits April 4, 2024 11:19

fix: redirect deprecated libraries resolution to the public lib DB

2cf8562

Signed-off-by: Antonio Nuno Monteiro <[email protected]>

add a new Redirect_to_lib_id constructor

886f060

Signed-off-by: Antonio Nuno Monteiro <[email protected]>

refactor: rename Redirect_to_lib_id to Redirect_by_id, Redirect_by_name

a93206b

Signed-off-by: Antonio Nuno Monteiro <[email protected]>

refactor: in scope.ml, move helper functions to the lexical scope where

1813f20

they're used Signed-off-by: Antonio Nuno Monteiro <[email protected]>

revert visibility change

0aae01a

Signed-off-by: Antonio Nuno Monteiro <[email protected]>

refactor: rename parent to public_libs

6303565

Signed-off-by: Antonio Nuno Monteiro <[email protected]>

anmonteiro force-pushed the anmonteiro/deprecated-name-redirect-to-public branch from 613e494 to 6303565 Compare April 4, 2024 18:21

anmonteiro merged commit c6a0a69 into ocaml:main Apr 4, 2024
26 of 27 checks passed

anmonteiro deleted the anmonteiro/deprecated-name-redirect-to-public branch April 4, 2024 18:41

This was referenced Apr 4, 2024

refactor: remove enabled_if from Lib_id.t #10388

Merged

fix: show the relevant loc for public library name collisions #10389

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: redirect deprecated libraries resolution to the public lib DB #10354

fix: redirect deprecated libraries resolution to the public lib DB #10354

anmonteiro commented Apr 1, 2024 •

edited

Loading

anmonteiro Apr 1, 2024

jchavarri left a comment

jchavarri Apr 2, 2024

anmonteiro Apr 2, 2024

anmonteiro commented Apr 2, 2024

rgrinberg commented Apr 2, 2024 •

edited

Loading

anmonteiro commented Apr 2, 2024 •

edited

Loading

anmonteiro commented Apr 2, 2024

rgrinberg Apr 3, 2024

anmonteiro Apr 3, 2024

anmonteiro Apr 3, 2024

rgrinberg Apr 4, 2024

rgrinberg Apr 3, 2024

rgrinberg Apr 3, 2024

anmonteiro Apr 3, 2024

anmonteiro Apr 3, 2024

rgrinberg Apr 4, 2024

		@@ -23,12 +23,12 @@ different folders.
		Without any consumers of the libraries

		$ dune build

fix: redirect deprecated libraries resolution to the public lib DB #10354

fix: redirect deprecated libraries resolution to the public lib DB #10354

Conversation

anmonteiro commented Apr 1, 2024 • edited Loading

Choose a reason for hiding this comment

jchavarri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anmonteiro commented Apr 2, 2024

rgrinberg commented Apr 2, 2024 • edited Loading

anmonteiro commented Apr 2, 2024 • edited Loading

anmonteiro commented Apr 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anmonteiro commented Apr 1, 2024 •

edited

Loading

rgrinberg commented Apr 2, 2024 •

edited

Loading

anmonteiro commented Apr 2, 2024 •

edited

Loading