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 variants #1999

Merged
merged 3 commits into from
Apr 3, 2019
Merged

Fix variants #1999

merged 3 commits into from
Apr 3, 2019

Conversation

TheLortex
Copy link
Collaborator

I found a bug in the variants implementation.
If you have several scopes with the same implementation library inside, the same implementation is considered multiple times for variant selection, which creates a conflict.
Such case happens when there are multiple .opam in a project directory.

This is the minimal fix which uses a Set to eliminate duplicates. The proper fix would be to change Variant.Map.Multi.t to be a map from variant to lib_info sets. Is this enough or should I make the proper fix ?

@rgrinberg
Copy link
Member

I think the definition of uniqueness introduced by Lib_info.Set.t is quite unintuitive. For example, 2 different libraries defined in the same dir are the same? That's too confusing.

If you want to use this hack properly, I suggest you simply use List.sort_uniq. But first add this function to stdune and make sure it uses dune's convention of Ordering.t.

I will have another look to see what a valid fix would look like.

@TheLortex
Copy link
Collaborator Author

Oops you're right it's confusing, I actually meant to define uniqueness on name + path.

@TheLortex TheLortex requested a review from a user April 1, 2019 12:33
@TheLortex
Copy link
Collaborator Author

ListLabels.sort_uniq only exists after 4.03 and List.sort_uniq seems to be hidden..

TheLortex and others added 3 commits April 3, 2019 17:03
Signed-off-by: Lucas Pluvinage <lucas.pluvinage@gmail.com>
…s define the same library.

Signed-off-by: Lucas Pluvinage <lucas.pluvinage@gmail.com>
Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>
Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>
@rgrinberg rgrinberg merged commit 9f6daf1 into ocaml:master Apr 3, 2019
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