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.

Sign up for GitHub

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

Jump to bottom

Ignore pinned packages when building dev-repo conflicts #398

Merged
merged 6 commits into from
Nov 7, 2023
Merged

Ignore pinned packages when building dev-repo conflicts #398

merged 6 commits into from
Nov 7, 2023

Conversation

samoht
Copy link
Collaborator

@samoht samoht commented Sep 22, 2023

This allows to pin a single package for multi-repository. In that case Opam-monorepo will always choose the pinned repository, so adding conflicts is just confusing the solver.

@samoht samoht force-pushed the do-not-add-conflicts-for-pinned-packages branch from b3fdcbd to 2b75025 Compare September 22, 2023 14:36
Leonidas-from-XIV
Leonidas-from-XIV reviewed Oct 2, 2023
View reviewed changes
Copy link
Member

@Leonidas-from-XIV Leonidas-from-XIV left a comment

Choose a reason for hiding this comment

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

Could you describe the change better? The code doesn't make me go "aha, so this is because of pinned repos", would be nice to have a better description of the change and why it fixes the problem.

test/lib/test_solve.ml Outdated Show resolved Hide resolved
test/lib/test_solve.ml Outdated Show resolved Hide resolved
@samoht samoht force-pushed the do-not-add-conflicts-for-pinned-packages branch from e06a063 to a0c00df Compare October 27, 2023 17:24
@samoht
Copy link
Collaborator Author

samoht commented Oct 27, 2023

I've pushed a new version: a mix between @reynir's #353 and my patch.

We now have a way to detect if a package is locally pinned, present in the pin-depends, or in a local opam file. This is largely untested for now, any help to push this forward is appreciated.

Leonidas-from-XIV
Leonidas-from-XIV requested changes Nov 3, 2023
View reviewed changes
Copy link
Member

@Leonidas-from-XIV Leonidas-from-XIV left a comment

Choose a reason for hiding this comment

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

A few small changes would make it better but the overall form seems ok - let's give it a chance!

lib/duniverse.ml Outdated Show resolved Hide resolved
lib/duniverse.ml Outdated Show resolved Hide resolved
lib/duniverse.ml Outdated Show resolved Hide resolved
lib/duniverse.ml Outdated Show resolved Hide resolved
lib/opam.ml Outdated Show resolved Hide resolved
lib/opam_solve.ml Outdated Show resolved Hide resolved
test/lib/test_duniverse.ml Show resolved Hide resolved
@samoht
Copy link
Collaborator Author

samoht commented Nov 7, 2023

Thanks, I've pushed fixes to address your review. Not totally sure what tests you wanted me to add, so I've just added a very simple one.

@Leonidas-from-XIV Leonidas-from-XIV merged commit a84f4f1 into tarides:main Nov 7, 2023
3 checks passed
@Leonidas-from-XIV
Copy link
Member

Leonidas-from-XIV commented Nov 7, 2023 •
edited
Loading

Looks good, thanks! Any test is better than no test and a starting point for more tests :)

@samoht samoht deleted the do-not-add-conflicts-for-pinned-packages branch November 7, 2023 23:07
@reynir reynir mentioned this pull request Nov 8, 2023
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@reynir reynir reynir left review comments

@Leonidas-from-XIV Leonidas-from-XIV Leonidas-from-XIV approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@samoht @Leonidas-from-XIV @reynir