Skip to content

fix: lib collision public same folder#10

Merged
anmonteiro merged 4 commits intoanmonteiro:anmonteiro/multi-context-libsfrom
jchavarri:jchavarri/multi-context-libs-fix-public-same-folder
Mar 28, 2024
Merged

fix: lib collision public same folder#10
anmonteiro merged 4 commits intoanmonteiro:anmonteiro/multi-context-libsfrom
jchavarri:jchavarri/multi-context-libs-fix-public-same-folder

Conversation

@jchavarri
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
@jchavarri
Copy link
Copy Markdown
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