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

Fix misleading error message #390

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

Conversation

gridbugs
Copy link
Collaborator

Fixes #385

@gridbugs
Copy link
Collaborator Author

Note that I'm still not satisfied that I understand why changing the order of the packages causes the diagnostics to change when the solver fails to find a solution. The docs don't mention the order of package names holding any meaning and I haven't dug through the 0install source to find out why this works yet, but it seems to fix the immediate problem.

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.

As far as I know the order of the packages matters in as much as 0install goes through them in priority order when picking up a solution.

So it could be that it wants to pick up the compiler before the non-dune package but it fails so it attributes the failure to the compiler as it cannot pick that one then, instead of the package. But this is just an attempt to explain it and I wish the solver wouldn't need this.

Nice workaround, thanks!

@Leonidas-from-XIV
Copy link
Member

Oh, I noticed that the CI failed, because it seems to have picked a beta compiler. That is a legitimate regression and likely to affect users. So maybe the code needs to be aware of the avoid-versions field.

Rather than demoting packages with avoid-version flag, attempt to run
the solver with these packages excluded, and then re-run the solver
allowing avoid-version packages to be included only if the initial
solve fails.
@gridbugs gridbugs force-pushed the fix-misleading-error-message branch from aa73915 to a3f9c9d Compare May 29, 2023 07:07
@gridbugs
Copy link
Collaborator Author

I've changed it so it now runs the solver with avoid-version packages excluded, then re-runs the solver with them included if the initial attempt to solve fails. @Leonidas-from-XIV let me know if you think this is too much of a hack!

@Leonidas-from-XIV
Copy link
Member

I feel like excluding the avoid-version packages is sort of the nuclear option (and makes the code which I think is already hard to follow through code with lots of strange cases even harder to understand). If I had to explain to someone in a year why we exclude the avoid-version packages, I probably could only point at regression tests saying "it makes them pass" which is not a great outlook for the maintainability of the code.

The thing I wonder about is, why does it even pick a compiler from the avoid-versions set? I can't see the CI failure anymore but I think the thing to investigate is why a beta compiler would pass the condition.

We do a lot of input processing to get the solver to do what we want it to do (already now, the sorting code is basically implementing avoid-versions by hand), but maybe the better solution is to improve the handling in the solver, as the code here is becoming harder and harder to maintain and other users of the solver would need to implement similar logic (though possibly they are not interested in rejecting non-dune packages so maybe this error case is just opam-monorepo specific, but it seems like any kind of rejection of packages than the default one isn't handled particularly well, with the solver often swallowing the actual error that prevents a solution).

@gridbugs
Copy link
Collaborator Author

The thing I wonder about is, why does it even pick a compiler from the avoid-versions set?

Good point. opam-monorepo tries to implement avoid-version by moving versions with that flag to the end of the candidates list (demote_candidates_to_avoid), but this only seems to work on direct dependencies (I don't understand why yet though). This is why we needed to add the ocaml-base-compiler dependency to the solver request to prevent the beta compiler (marked as avoid-version) from being chosen. Further, ocaml-base-compiler needed to appear before other dependencies in the solver request, because most packages depend on ocaml which depends on ocaml-base-compiler, so while solving the first dependency in the request, the solver ends up selecting the beta ocaml-base-compiler transitive dependency.

I think the correct solution here is for opam_0install to become aware of the avoid-version flag and implement the same semantics as opam itself. Then opam-monorepo and dune would both get the benefit and it would mean opam-monorepo no longer has to try to implement avoid-version itself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The <package> Doesn't build with dune error is omitted from the output of opam monorepo lock in some cases
2 participants