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

Make "opam install --check" check if all dependencies are installed recursively #6122

Merged
merged 7 commits into from
Aug 21, 2024

Conversation

kit-ty-kate
Copy link
Member

@kit-ty-kate kit-ty-kate commented Jul 29, 2024

Fixes #6097
Queued on top of #6121
Queued on #6123

@kit-ty-kate kit-ty-kate added KIND: BUG PR: QUEUED Pending pull request, waiting for other work to be merged or closed labels Jul 29, 2024
@kit-ty-kate kit-ty-kate added this to the 2.3.0~alpha milestone Jul 29, 2024
@rjbou rjbou self-requested a review August 2, 2024 14:48
Copy link
Collaborator

@rjbou rjbou left a comment

Choose a reason for hiding this comment

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

lgtm on the idea and its proper code.
I've added a test for basic cases for install check, like that we are sure that these changes don't change them. I don't know if it should be in this PR or #6121.

Comment on lines +52 to +194
### opam install --fake a.3
The following actions will be faked:
=== upgrade 1 package
- upgrade a 1 to 3
=== install 1 package
- install b 1 [required by a]

<><> Processing actions <><><><><><><><><><><><><><><><><><><><><><><><><><><><>
Faking installation of b.1
Faking installation of a.3
Done.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Out of curiosity, why faking the install ?

Copy link
Member Author

Choose a reason for hiding this comment

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

no other reason other than the force of habit and just in case it makes it faster

src/client/opamClient.ml Outdated Show resolved Hide resolved
@kit-ty-kate kit-ty-kate removed the PR: QUEUED Pending pull request, waiting for other work to be merged or closed label Aug 5, 2024
@kit-ty-kate kit-ty-kate requested a review from rjbou August 5, 2024 13:34
@kit-ty-kate
Copy link
Member Author

The benchmark failure is real and is caused by an infinit loop. I'll have a look

@kit-ty-kate kit-ty-kate marked this pull request as draft August 8, 2024 20:38
@kit-ty-kate kit-ty-kate marked this pull request as ready for review August 17, 2024 18:16
master_changes.md Outdated Show resolved Hide resolved
src/client/opamClient.ml Outdated Show resolved Hide resolved
Copy link
Collaborator

@rjbou rjbou left a comment

Choose a reason for hiding this comment

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

I've added some test from an old branch that I have, and some format nitpicking. Feel free to squash if it's ok for you.
Otherwise, lgtm!

src/client/opamClient.mli Outdated Show resolved Hide resolved

let assume_built_restrictions ?available_packages t atoms =
let missing = check_installed ~build:false ~post:false t atoms in
let missing =
check_installed ~build:false ~post:false ~recursive:false t atoms
Copy link
Collaborator

Choose a reason for hiding this comment

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

Assume built does not check recursively, is it to not change the known behaviour?

Copy link
Member Author

Choose a reason for hiding this comment

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

no, the previous behaviour of this function did not check recursively so the behaviour of assume-built hasn't changed either

Copy link
Collaborator

@rjbou rjbou left a comment

Choose a reason for hiding this comment

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

Thanks!

@rjbou rjbou merged commit 0c4ace3 into ocaml:master Aug 21, 2024
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

opam install --check does not check whether dependencies are installed recursively
2 participants