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

test: show that universal libraries can't share the public_name #10471

Merged

Conversation

anmonteiro
Copy link
Collaborator

Inspired by a question from @andreypopp.

This is a bit of a sad limitation, because it means enabled_if just proliferates when depending on installed libraries.

@@ -0,0 +1,64 @@
Public libraries using the same `public_name`, in different contexts
Copy link
Member

Choose a reason for hiding this comment

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

Could you describe the test a bit more here? It should be possible to know what it's about without knowing any context.

@jchavarri
Copy link
Collaborator

I talked about this with @andreypopp offline, the problem is that we have been using two contexts in a single opam switch until now. But installing public libs with the same name should be possible if each context was linked to a different opam switch, using st like (context melange (opam (switch x.xx.x))).

@jchavarri
Copy link
Collaborator

I tried using (switch) in the melange context defined in the test so that public libraries get installed in separate switches, but the result is the same. 😕 I guess Dune duplication checks come before actually checking if the public libraries are being installed in separate switches?

because it means enabled_if just proliferates when depending on installed libraries.

I fail to see how the premise of this PR would work. How can we avoid the proliferation of enabled_if without something like #10362? As a consumer of a universal library, how can I use it without having two contexts? In reality, a "universal" library consists of two implementations, so I can't imagine a way in which these two implementations are exposed under a single name. For this, Dune already provides a solution: virtual libraries. But they require to hide the implementation details behind abstract types to make it work. Universal libraries allow you to expose the implementations to consumers, at the cost of defining two build contexts.

@anmonteiro
Copy link
Collaborator Author

I guess Dune duplication checks come before actually checking if the public libraries are being installed in separate switches?

Indeed, I believe they're coming from scope.ml, when creating the db from stanzas.

Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>
Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>
@anmonteiro anmonteiro force-pushed the anmonteiro/test-eif-public-name-collision branch from 2b3c2dd to c9cd9dc Compare May 7, 2024 22:38
@anmonteiro
Copy link
Collaborator Author

@rgrinberg thanks, I made the description a bit more clear.

@@ -0,0 +1,65 @@
Show that public library names can't be defined twice, even in different
contexts
Copy link
Member

Choose a reason for hiding this comment

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

It's worth adding a statement on if this is a bug or an intentional limitation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

to be honest, we're not sure. It depends a bit on the usage, and how they're installed:

  • for Melange libraries that clash, it's not a problem if they have the same public name, as we install melange artifacts under <lib_name>/melange anyway.
  • for other libraries, if we decided to install them in the same context, they would be overwritten.

We're working on an installation RFC with the aim of making a special case for a "melange context" that can be installed in the same prefix as native libraries with the same name.

Copy link
Member

Choose a reason for hiding this comment

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

Okay, well I think this particular issue is easily fixed. We just ignore libraries with enabled_if for the purposes of libname_conflict_map.

@anmonteiro anmonteiro merged commit c2ea2fa into ocaml:main May 18, 2024
1 of 2 checks passed
@anmonteiro anmonteiro deleted the anmonteiro/test-eif-public-name-collision branch May 18, 2024 22:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants