-
Notifications
You must be signed in to change notification settings - Fork 396
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
pkg: dedup package names in dependencies #10544
Conversation
cbf6435
to
1a86d73
Compare
2338a8d
to
b8f14c0
Compare
else x :: ret_reversed, Package_name.Set.add seen x) | ||
in | ||
List.rev ret_reversed | ||
;; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could potentially make sense to have a
val uniq : equal:('a -> 'a -> bool) -> 'a list -> 'a t
style function in Stdune.List
, but this is fine too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I just noticed that List.sort_uniq
already exists. It will obviously reshuffle the order, but might be ok for the usecase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that we aren't allowed to re-order the dependencies as specified by the user. The order has a semantic meaning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the case of libraries
yes, but do OPAM dependencies have a semantic ordering?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should preserve the order of dependencies in lockfiles if for no other reason than to simplify the mental model for the correspondence between lockfiles and the opam files they are generated from. If some fields are copied but reordered then it's more work to understand how the opam file maps to the lockfile.
Could you submit the failure in a separate commit? In this PR or a separate one. EDIT: didn't realize #10543 existed |
Fixes a bug where depending on a package with a constraint involving a conjunction would lead to the package appearing in the "depends" lockfile field multiple times. Signed-off-by: Stephen Sherratt <stephen@sherra.tt>
b8f14c0
to
00892e1
Compare
Fixes a bug where depending on a package with a constraint involving a conjunction would lead to the package appearing in the "depends" lockfile field multiple times. Signed-off-by: Stephen Sherratt <stephen@sherra.tt>
Fixes a bug where depending on a package with a constraint involving a conjunction would lead to the package appearing in the "depends" lockfile field multiple times.
Fixes #10542