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

On install action, reinstall if test/doc dependencies are not present #4514

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

rjbou
Copy link
Collaborator

@rjbou rjbou commented Jan 28, 2021

fix #4507

@rjbou rjbou added the PR: WIP Not for merge at this stage label Jan 28, 2021
@rjbou rjbou added this to the 2.1.0~beta5 milestone Jan 28, 2021
@rjbou rjbou added this to PR in Progress in Opam 2.1.x via automation Jan 28, 2021
@rjbou
Copy link
Collaborator Author

rjbou commented Jan 28, 2021

Current state : it filters according with-test & with-doc, not if they are combined with other variables as build, post, dev.

@avsm avsm modified the milestones: 2.1.0~beta5, 2.1.1 Feb 19, 2021
@avsm
Copy link
Member

avsm commented Feb 19, 2021

Dev meeting: Pushing this out to 2.1.1, we can make sure the right variables are all accounted for. There is a workaround for opam reinstall right now that we can release note.

Copy link
Member

@AltGr AltGr left a comment

Choose a reason for hiding this comment

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

Hm, this should make the issue less pregnant, but isn't a complete solution: indeed, if you opam install foo --with-doc and doc-deps were missing, it will install them, and then recompile foo (you don't even need to add to reinstall in this case, the dependency change will trigger it already ; just removing from pkg_skip would have been enough) : you get the doc for foo installed as expected. But if the doc dependencies happened to be installed already, you will still get a nothing to do without a recompilation with docs enabled.

We don't really have a way to fix this since we don't record (atm) the flags that were set when foo was initially installed, so the only options would be to always reinstall when --with-x is set (that sounds dangerous ; but it does actually make complete sense in the case of tests ? Maybe after a question like for opam reinstall <not-installed-pkg> ?) ; or just advise to use reinstall instead ?

@@ -1103,6 +1103,29 @@ let install_t t ?ask ?(ignore_conflicts=false) ?(depext_only=false)
if assume_built then OpamPackage.Set.of_list pkg_skip
else Lazy.force t.reinstall %% OpamPackage.Set.of_list pkg_skip
in
let pkg_skip, pkg_reinstall =
if OpamStateConfig.(!r.build_test || !r.build_doc) then
let to_reinstall =
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it be a bit simpler here to just resolve the dependencies in the current context (i.e. following the OpamStateConfig.r.build_* values), and check that everything is installed ?

@rjbou rjbou removed this from the 2.1.1 milestone Feb 26, 2021
@rjbou rjbou added this to In progress in Feature Wish via automation Feb 26, 2021
@rjbou rjbou removed this from PR in Progress in Opam 2.1.x Apr 12, 2021
@rjbou rjbou added this to PR in progress in Opam 2.2.0 via automation Apr 12, 2021
@dra27 dra27 removed this from PR in progress in Opam 2.2.0 Feb 11, 2022
@dra27 dra27 added this to PR in Progress in Opam 2.3 via automation Feb 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: WIP Not for merge at this stage
Projects
Feature Wish
  
In progress
Opam 2.3
  
PR in Progress
Development

Successfully merging this pull request may close these issues.

with-test deps not installed after second call to opam install
3 participants