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

Depext filter enhancement: resolve package variables #5455

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

Conversation

rjbou
Copy link
Collaborator

@rjbou rjbou commented Feb 28, 2023

depexts: filter can now handle package variables. They are stored and resolved on the go & gradually. On install, packages appearing in solution are considered as installed for resolving. See depexts.test for examples.

@rjbou rjbou added PR: QUEUED Pending pull request, waiting for other work to be merged or closed PR: WAITING FOR REVIEW labels Feb 28, 2023
@rjbou rjbou added this to the 2.2.0~alpha milestone Feb 28, 2023
@rjbou rjbou requested review from AltGr and dra27 February 28, 2023 12:45
@rjbou rjbou added this to PR in progress in Opam 2.2.0 via automation Feb 28, 2023
@rjbou rjbou moved this from PR in progress to PR to review for 2.2.0~alpha in Opam 2.2.0 Feb 28, 2023
tests/reftests/depexts.test Outdated Show resolved Hide resolved
In order to be able to resolve undefined variables at several stages
(once switch_state loaded, on install, etc.)
They are lately used for resolving at install stage, for availability
checkup, etc. permitting to evaluate on filter install status of a
package or its version.
@kit-ty-kate kit-ty-kate removed the PR: QUEUED Pending pull request, waiting for other work to be merged or closed label Mar 15, 2023
Comment on lines +104 to +109
### OPAMVAR_os_family=dummy-success::0 opam install conf-os --show
The following actions would be performed:
=== install 2 packages
- install conf-os 1
- install conf-os1 1 [required by conf-os]
[WARNING] These additional system packages are required, but not available on your system: os1-depext
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure to understand this test result. Why is this just a warning if the system package isn't available?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It should be a simple message indeed, or an error, not a warning.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In fact, here it's with --show, it errors at install step.

Comment on lines +110 to +116
### OPAMVAR_os_family=dummy-success::0 opam install conf-os
The following actions will be performed:
=== install 2 packages
- install conf-os 1
- install conf-os1 1 [required by conf-os]
[WARNING] These additional system packages are required, but not available on your system: os1-depext
[ERROR] Some packages have missing dependencies after resolution:
Copy link
Member

Choose a reason for hiding this comment

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

Same sort of question than above here: why are we displaying the error so late?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We check depext availability when we try to install once we have the solution. We resolve last filters (here conf-os1:installed) with an environment that contains packages to install as installed.

- install conf-os1 1 [required by conf-os]
[WARNING] These additional system packages are required, but not available on your system: os1-depext
[ERROR] Some packages have missing dependencies after resolution:
- conf-os.1: os1-depext
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't also say something about os2-depext ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As we resolve with the solution, conf-os1:installed resolves to true while conf-os2 resolves to false, that's why we display only os1-depext.
We can recheck availability at this stage, and print a message for os2-depext.

@kit-ty-kate
Copy link
Member

I'm rather unconvinced by the solution and I'm still confused as to why this is necessary for Windows support.
Here is the summary of the problems i can see after thinking about it some more:

  • Proper support requires recursive solve (do one solve, check if it installs unavailable depexts, do another solve without this package, …)
  • The current way of doing (one solve, check if it's actually possible quite late in the process) feels quite jenky and doesn't really help users understand issues
  • It makes it quite hard to understand what a package is doing and unpredictable

Could describe what the problem it's trying to solve is?

@kit-ty-kate kit-ty-kate moved this from PR to review for 2.2.0~alpha to PR to update for 2.2.0~alpha in Opam 2.2.0 Apr 17, 2023
@kit-ty-kate kit-ty-kate added PR: WIP Not for merge at this stage and removed PR: WAITING FOR REVIEW labels Apr 17, 2023
@rjbou rjbou moved this from PR to update for 2.2.0~alpha to After ~alpha; before release in Opam 2.2.0 May 8, 2023
@rjbou rjbou removed this from the 2.2.0~alpha milestone Jun 7, 2023
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
No open projects
Opam 2.2.0
  
To keep on hand
Development

Successfully merging this pull request may close these issues.

None yet

3 participants