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

fix: lib collision public same folder #10

Conversation

jchavarri
Copy link
Collaborator

Solves the problem discussed in ocaml#10307 (comment).

Signed-off-by: Javier Chávarri <javier.chavarri@gmail.com>
@jchavarri jchavarri force-pushed the jchavarri/multi-context-libs-fix-public-same-folder branch from 822b1c1 to f651140 Compare March 26, 2024 11:04
(* we need to check for private name to avoid "multiple rules" errors,
because even for public libraries, the artifacts folder still uses
the private name *)
let name = Library.private_name part.stanza in
Copy link
Owner

@anmonteiro anmonteiro Mar 26, 2024

Choose a reason for hiding this comment

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

I wonder if we should be checking directly what conflicts. In this case, check the obj_dir path equality rather than the private name equality? we have it in group.obj_dir.

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 removed private_name and used obj_dir instead in c1a8628.

imo, as a user the error message is slightly harder to understand because we can't refer to the lib private name anymore, but I don't have a strong opinion. Whatever you and @rgrinberg prefer.

Copy link
Owner

Choose a reason for hiding this comment

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

I do actually prefer the current version than the previous one. This looks way better IMO

jchavarri and others added 3 commits March 27, 2024 10:59
…text-libs-fix-public-same-folder

Signed-off-by: Javier Chávarri <javier.chavarri@gmail.com>
Signed-off-by: Javier Chávarri <javier.chavarri@gmail.com>
Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>
@anmonteiro
Copy link
Owner

OK, so I remembered we actually have a library ID now, which means that we have access to the private name that way. So I restored the error message to the previous version.

@anmonteiro anmonteiro merged commit 5c8f78f into anmonteiro:anmonteiro/multi-context-libs Mar 28, 2024
22 of 23 checks passed
@jchavarri
Copy link
Collaborator Author

OK, so I remembered we actually have a library ID now, which means that we have access to the private name that way. So I restored the error message to the previous version.

Awesome!

@jchavarri jchavarri deleted the jchavarri/multi-context-libs-fix-public-same-folder branch March 29, 2024 07:37
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.

2 participants