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

refactor: treat deprecated lib case explicitly #10231

Closed

Conversation

jchavarri
Copy link
Collaborator

This change will help with enabling multi-context libraries in #10179.

The reason is that with the changes in that PR, the checks in create_db_from_stanzas change a bit, with two redirects being a potentially valid situation (e.g. if there are two public libraries with the same name defined in different contexts).

Having a way to explicitly recognize the deprecated library case allows to keep the behavior of that feature untouched while unlocking multi-context public libraries.

Signed-off-by: Javier Chávarri <javier.chavarri@gmail.com>
@jchavarri jchavarri requested a review from rgrinberg March 7, 2024 18:24
@anmonteiro anmonteiro self-requested a review March 7, 2024 21:14
@rgrinberg
Copy link
Member

I'd keep it in #10231 . Off the top of my head, I don't see how this is going to help to implement enabled_if for libraries.

@jchavarri
Copy link
Collaborator Author

The main reason is that to enable multi-context libs with the same name, we need to allow Redirect with duplicated names to "go through" in the database to support the case with public libs.

Essentially, the following line needs to change to remove the Lib_name.equal change and instead add both to the (newly added) list of libs:

if Lib_name.equal lib1 lib2 then Ok v1 else Error (loc1, loc2)

But if we do that change without introducing the new variant, we will break the deprecated libs use case.

@jchavarri
Copy link
Collaborator Author

I'd keep it in #10231 .

10231 is this PR. Which one are you referring to?

Signed-off-by: Javier Chávarri <javier.chavarri@gmail.com>
@rgrinberg
Copy link
Member

I meant #10179.

It could be very well the case that this PR is useful, but it's impossible to tell without seeing the final use of it. On its own, it does not yet offer any value (not in readability, performance, etc)

@jchavarri jchavarri closed this Mar 8, 2024
@jchavarri jchavarri deleted the lib-treat-deprecated-explicitly branch March 8, 2024 08:32
jchavarri added a commit to jchavarri/dune that referenced this pull request Mar 8, 2024
brings the changes from ocaml#10231

Signed-off-by: Javier Chávarri <javier.chavarri@gmail.com>
@jchavarri
Copy link
Collaborator Author

jchavarri commented Mar 8, 2024

ftr applied the changes in jchavarri@845ac40.

anmonteiro pushed a commit to anmonteiro/dune that referenced this pull request Mar 26, 2024
brings the changes from ocaml#10231

Signed-off-by: Javier Chávarri <javier.chavarri@gmail.com>
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

2 participants