Solver: Not all unsatisfied constraints are marked as "(user goal)".

[Mokosha]

As an example, see:

https://travis-ci.org/Mokosha/Lambency/jobs/366406973

Note: Documentation fixes for https://docs.haskellstack.org/en/stable/ should target the "stable" branch, not master.

Please include the following checklist in your PR:

• Any changes that could be relevant to users have been recorded in the ChangeLog.md
• The documentation has been updated, if necessary.

Please also shortly describe how you tested your change:

I locally tested the sequence of normal commands for my personal project that were failing on travis.ci:

stack --no-terminal unpack cabal-install-1.24.0.2
stack --no-terminal init cabal-install-1.24.0.2 --solver --ignore-subdirs
stack --no-terminal install
stack --no-terminal --resolver lts-6 init . --force --solver --ignore-subdirs

The solver seems to only have been tested with this version of cabal. It's probably better to try and update the version of cabal that it depends on, but this at least fixes a bug in the current implementation.

[snoyberg]

I haven't worked on the solver command in a long time, but it looks like this is likely to break compatibility with older versions of cabal-install, unless I'm misreading things significantly.

[Mokosha]

I've tested it the current commits with cabal-install versions 1.18.1.0, 1.20.2.0, 1.22.9.0 and 1.24.0.2, the last one being the one that it seems to have been tested with. They all seem to work now.

[snoyberg]

It looks like the code in question was originally added in b7c535f by @harendra-kumar. Harendra: would you mind having a look at this?

[harendra-kumar]

Yeah, I remember writing this code, will take a look tomorrow.

[harendra-kumar]

@Mokosha can you help me understand why you had to make the parsing changes? I see there is a "trying:" line in your travis example output so it should have been parsed by the existing code? I am surely missing something here.

The idea behind this code is to roughly detect the "user packages" that are causing the conflict so that we can try again after omitting those packages from the config and that's why I had put the "user goal" restriction in parsing. Can you help me understand how and why the modified code works as expected?

[Mokosha]

Hey @harendra-kumar, sure.

In the example, cabal failed without having any of the output marked with user goal, so originally I had removed only the && T.isSuffixOf "(user goal)" s. However, this caused problems with other versions of cabal where the package listed on the trying: line was not parseable due to the way it was formatted. After removing the trying: parser and selecting only the next goals from cabal, it seems to generate some stack.yaml even if not the optimal one.

I don't have a good sense of what circumstances would cause cabal to produce the output in the example. By definition, if a package is a dependency of something in the resolver, it should be in the resolver, too (right?). It might have something to do with the dependency via a set flag? (semigroups-0.18.2:+tagged ?)

[harendra-kumar]

@Mokosha thanks for the explanation.

This code is meant for detecting user packages i.e. the packages that you are building or have in your repository or in other words the packages that are not dependencies. When the solver is unable to come up with a good plan, as a last resort, it tries to find which user packages are causing a conflict and then remove those packages from the plan and try again including only the remaining packages in the build plan. When you have only one package in your repo this algo does not make much sense as it will ignore the only package you are building. It is useful when you have many packages and you could do without some of them.

With that in mind, do you think this change is helpful? Can you point to a stack.yaml produced by this? Is the stack.yaml that it produced working correctly to build the package?

[Mokosha]

If the intended goal is to remove local packages from consideration from cabal, then I suppose this change does not help achieve it. However, the previous version of the code didn't seem to do that either. This code is only being used in the top-level solveResolverSpec function that tries to find extra dependencies according to the comment. The current code takes cabal's solution and finds the packages (versions included) outside the resolver that we need to pull either from local packages or hackage. Removing local packages from the plan is only done implicitly provided they are creating conflicts, but overall, the resolution of the full build plan proceeds regardless of whether or not the packages conflict.

In particular, the set of packages sent to cabal as constraints involve all packages (user or otherwise) and their dependencies that are required but not in the current resolver. If this is not the intended use-case, then you have a different bug in the Solver code, but I would argue that if the goal is finding a workable build plan, the current approach is sensible.

To answer your questions:

• Yes, I believe this change is helpful given my current understanding of the code.
• Consider the following some-package.cabal

name: some-package
version: 1.0.0
build-type: Simple
cabal-version: >= 1.10

library
  exposed-modules: Experiments
  build-depends: base, linear == 1.20.5, netwire >= 5
  default-language: Haskell2010

---

module Experiments where data Empty

With the current version of stack, this fails to produce a build-plan because of the problem similar to the example above:

$ stack --no-terminal --resolver lts-6 init . --force --ignore-subdirs --solver

.......


Could not parse cabal-install errors:

>>>> Cabal errors begin
Warning: cannot determine version of /usr/bin/strip :
""
cabal: Could not resolve dependencies:
next goal: tagged (dependency of linear-1.20.5)
rejecting: tagged-0.8.5 (constraint from main config
/private/var/folders/fk/hl4fdy5x33n56y077mtmp2zm0000gn/T/cabal-solver90500/cabal.config
requires ==0.8.4)
trying: tagged-0.8.4
next goal: semigroupoids (dependency of linear-1.20.5)
rejecting: semigroupoids-5.2.2, semigroupoids-5.2.1, semigroupoids-5.2,
semigroupoids-5.1 (constraint from main config
/private/var/folders/fk/hl4fdy5x33n56y077mtmp2zm0000gn/T/cabal-solver90500/cabal.config
requires ==5.0.1)
trying: semigroupoids-5.0.1
rejecting: semigroupoids-5.0.1:+tagged (conflict: tagged==0.8.4,
semigroupoids-5.0.1:tagged => tagged>=0.8.5 && <1)
rejecting: semigroupoids-5.0.1:-tagged (manual flag can only be changed
explicitly)
Dependency tree exhaustively searched.
<<<< Cabal errors end

CallStack (from HasCallStack):
  error, called at src/Stack/Solver.hs:130:25 in stack-1.6.5-7mRq9BU86At6wBLtOimNmq:Stack.Solver

With the current changes, stack first fails when everything is a hard constraint, but then succeeds when it needs to parse the cabal output. It produces the following stack.yaml:

resolver: lts-6.35
packages:
- .
extra-deps:
- netwire-5.0.3
- semigroupoids-5

This proceeds to build fine. Every stack.yaml that I've produced with these changes has also succeeded in its build plan or failed due to an unrelated error (GHC incompatibilities, etc).

[snoyberg]

@Mokosha @harendra-kumar I'm just reviewing open PRs and wanted to check in: any thoughts on next steps here?

[harendra-kumar]

@snoyberg, sorry I missed the previous update by @Mokosha .

@Mokosha

If the intended goal is to remove local packages from consideration from cabal, then I suppose this
change does not help achieve it. However, the previous version of the code didn't seem to do that
either. This code is only being used in the top-level solveResolverSpec function that tries to find
extra dependencies according to the comment.

The intended logic that I implemented long ago was to detect the user packages involved in a conflict and try ignoring one of them as a last resort. It was working perfectly earlier, I have not tested it recently though. But I see the code to achieve that seems to be still there. If you see, parseCabalErrors return Left pkg-list i.e. the list of user packages involved in conflict, which is then returned by solveResolverSpec to checkBundleResolver which then selects one package from the this list and returns it to getWorkingResolverPlan which ignores this package and tries again.

What your change is doing effectively is to somehow make parseCabalErrors succeed so that it does not error out and we are able to proceed for the next step, but it breaks that logic. Equivalently, but without breaking this logic, what you can do is to turn the error produced by parseCabalErrors into a warning so that the next step can proceed even if we are unable to parse the cabal output.

[Mokosha]

but it breaks that logic

It's unclear to me how my change breaks that logic -- it seems to be relaxing the constraints on the parsing, not removing them.

Correct me if I'm wrong, but it seems like your suggestion is to keep the incorrect parsing and instead ignore the failure it produces? Changing errExit e to return $ Right Map.empty seems to allow the solver to succeed but it doesn't produce the correct stack.yaml. If this is the better way to "fix" the problem, then I'm more than happy to implement it (but it seems like the failure to parse the output will remain).

[harendra-kumar]

It's unclear to me how my change breaks that logic -- it seems to be relaxing the constraints on the
parsing, not removing them.

The intended logic is to detect the "user packages" involved in the conflict, not any package. Because we want to omit a user package and then try again. With the relaxed constraints, it is no longer guaranteed to be a user package.

Changing errExit e to return $ Right Map.empty seems to allow the solver to succeed but it doesn't
produce the correct stack.yaml.

We need to understand why the original change proposed in the PR seems to work. As per my (maybe incomplete) understanding of the code, when you parse the cabal output incorrectly it will pass non-user packages to be ignored, which should be as good as returning an empty list unless something funny is going on. Now, I could not spent enough time trying to understand what's going on, can you please dig a little bit and explain why exactly your change is working?

[Mokosha]

Hi -- Sorry for the delay.

I guess I'm confused about the distinction between "user packages" and just any package that's sent as a constraint to cabal. 95% of the time, the "next goal" according to the output will be a user package as that's the constraint that cabal is trying to solve. In certain scenarios, like the one provided in the examples, the constraint is not a "user package". The packages that we choose to omit from the ones listed as constraint flags to cabal shouldn't be restricted to just "user packages" as there are others (e.g. dependencies) that we might want to omit to let cabal provide a better solution to the overall goal of building the main package.

which should be as good as returning an empty list unless something funny is going on

It isn't, as you can see in the examples. There are situations where certain packages are incompatible, like in the case of netwire-5 and linear-1.20.5 in the lts-6 resolver. netwire-5 is not in the resolver, and its dependency on semigroupoids creates a conflict with what exists in linear-1.20.5 and the resolver. semigroupoids is not a "user package" -- it's a dependency. There exists a workable solution for semigroupoids (e.g. semigroupoids-5 that gets added to stack.yaml with this change) found by cabal once we remove the proper constraint flags from the cabal input.

Does that help? I might not be explaining this using the right terminology...

[harendra-kumar]

The packages that we choose to omit from the ones listed as constraint flags to cabal shouldn't be
restricted to just "user packages"

The idea as I designed it was this - when we have more than one user packages under the tree and we cannot find a working build plan when we include all of them, because their dependencies conflict, then as a last ditch effort we try omitting one package from the conflicting ones and see if we can find a plan. If we find a plan then that particular package is removed from the list of packages in the stack.yaml. One of the usecases was when some packages that we are trying to init have several other sub-packages/cabal files merely for testing and not allowing stack init to succeed.

Now, if we want to apply this sort of omitting of packages to dependencies then I am not even sure what it means. I am not even sure what exactly is making it work for you, that's why I asked you to dig further. My guess was that you are just sending a non-user package, which is as good as sending an empty list because we are not going to omit it as it is not a user package. You need to figure out what's going on.

[Mokosha]

OK -- I may have a misunderstanding of what the goals of the --solver flag are. I was under the impression that the --solver flag would try to match as many constraints as possible given the current resolver. In doing so, it would omit all packages that it encountered that cause problems in cabal: i.e. if a package exists in the resolver, but is incompatible with some user constraints, we would remove the package from the list of constraints to cabal in order to add a possibly more compatible version to the extra-deps field of the stack.yaml.

If this isn't the case, then I may have misunderstood the goal of the --solver flag, and this change is inappropriate. If we're not meant to relax the constraints from any of the packages in the resolver then there might be a bug where we try to remove the only existing user package -- perhaps in this case the correct answer is to simply produce an incompatible stack.yaml, although changing the error call to a warning seems like the incorrect place to do that (it shouldn't be getting there to begin with).

If my understanding of the intent of the --solver command is correct, then there are situations (for a single user package) that are causing problems due to incorrect parsing of the cabal output. These situations are outlined in the provided examples. We can, indeed, return the empty list instead of throwing an error to have stack "succeed" in the case where parsing fails, but the fundamental problem of parsing the cabal output will not omit the packages from the resolver that it could. In turn, the produced stack.yaml will not contain the necessary extra-deps that it needs to build the package.

Rereading the thread, I now understand that the original intent of the code was to support directories that contain multiple user packages. In these cases, if conflicts arise (regardless of whether or not they are marked as user goal: in the cabal output), they will show up as the next goal: that cabal is solving for, so I don't believe this change breaks anything, although proving that is difficult (this change has worked with all of my test cases).

[harendra-kumar]

@Mokosha omitting of packages is an optional feature, disabled by default and enabled by a command line flag. It can omit one or even all packages, in the latter case a stack.yaml with no packages enabled is generated.

The problem here is that we generate a list of conflicting packages irrespective of the status of omit-packages flag. If omit-packages is not enabled we do not need even execute this code, but we do. You can possibly pass a flag down to the solver to enable or disable this behavior based on the omit flag.

I am not sure whether your code actually breaks the existing logic, but it will include more cases and may also include non-user packages in the output. I have not looked at how that case is handled in the upper level callers, do we just ignore non-user packages (in which case it would be harmless) or not.

So your options are:

• pass a flag and invoke this logic only when omit-packages is enabled
• when there is an error ignore the error, turn it into a warning. This may result in a non-working stack.yaml when omit-packages is specified. Your current code may do the same, in case it avoids the error but does not return a user-package to ignore.

[Mokosha]

I unfortunately don't have time to work on this, so I'm closing this PR in favor of the tracking bug #4319 .