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

Stack init and solver overhaul #1583

Merged
merged 29 commits into from Jan 8, 2016

Conversation

Projects
None yet
3 participants
@harendra-kumar
Collaborator

harendra-kumar commented Dec 31, 2015

These commits are part of a series of commits to enhance the stack init and solver functionality. The goal is to be able to automatically init any project which has a working cabal file. Similarly the stack solver command should be able to verify and fix the configuration in an optimal manner. Also, to provide a better user experience by providing informative error messages and next steps at every failure. In other words, init should just work and be smooth and painless and always work unless the package is really broken.

See also #1529 and #1533.

The commit messages have more details about the problems and fixes. I will be adding more commits to the branch very soon. I am pushing these out for an early review. The changes are logically self contained, add incremental functionality and should not break any existing functionality.

Please review individual commits in order and see the commit messages for an overview of each commit.

harendra-kumar added some commits Dec 31, 2015

Track the best snapshot by the number of deperrors
Changes in this commit:

1) When figuring out a build plan, track the dependency errors for each
snapshot to figure out which snapshot is the best even if there are
dependency errors.

2) Use the same strategy even when finding out the best set of flags.
Find out the combination of flags producing least number of dependency
errors.

3) Modularise the interfaces to make the code components more reusable
if needed.

This change is supposed to be used for later enhancements to use the
dependency solver on the best matching snapshot in absence of an error
free match.
Track dependency errors for GHC wired in packages
This commit adds ability to track and distinguish build plan dependency errors
related to the packages which are wired in GHC. This interface will be used to
detect whether a snapshot can be used at all for building a given package. If
GHC wired in packages are incompatible we cannot use the snapshot even with
extra dependencies.

This is not being used yet but will be used in future commits.
Improve the build plan selection error messages
Provides more informative but concise error messages during stack init
build plan selection process.
Allow solver to be used with any resolver
Currently stack init --solver cannot be used with a specific resolver.
It just uses the GHC on your path and provides a compiler resolver
corresponding to that GHC and all deps are extra deps.

Instead, we want to be able to use a specific snapshot and only deps not
matching the snapshot should be extra deps. This commit is a step towards
that goal.

1) '--solver' is now a switch which can be used along with a resolver.
    'stack init --resolver lts-2.22 --solver' will now be a valid init
    command.

2)  Snapshot selector now returns the snapshot with least number of
    dependency errors as a partial match. This snapshot is to be used
    by the solver.

    Note that the solver is not yet changed to work on an existing
    snapshot. That will come in a future commit.

3)  We now report an explicit error when the specified resolver does not
    have a compiler compatible with the package dependencies.

4)  Note a behavior change for the 'stack init --resolver' command. Earlier it
    used to write the stack.yaml file even if it did not successully resolve all
    packages. Now it will fail in that case and suggest '--solver' to
    resolve the extra dependencies.

5)  Separate the snapshot selection interface from the snapshot
    dependency check interface.

6) Some error message reporting changes.
Reverse dependency between Init and Config modules
Need to import Stack.Setup in Stack.Solver to be able to use ensureCompiler
which creates a dependency cycle. The cycle can be fixed by making Init
depend on Config instead of the opposite.

This commit only includes the code movement due to the above. There are
no functionality changes.
Ensure, use specified compiler for new/init/solver
This commit introduces the following enahncements:

1) Support --install-ghc for stack new/init/solver commands

2) init or solver will now use the correct compiler specified
in the resolver on command line during init or in stack.yaml when using
the solver command.

Before this change solver used the compiler found on path disregarding
any configuration settings (for stack solver) or the snapshot chosen
(for stack init) or the resolver specified on command line.

Note that even with this change stack init --solver still returns a
compiler resolver with all deps as extra-deps. Using a snapshot resolver
with minimal extra-deps is a matter of a future commit.

Fixes #1529
Solver: Use snapshot instead of compiler resolver
Changes include:

1) For 'init --solver' use the best selected snapshot (having minumum number
of dep errors) with minimum extra deps on top of the snapshot. Also use
the auto selected flags producing minimal snapshot dep errors for solving.

Before this change, solver always used a compiler resolver and
everything as extra-deps.

2) Fix a bug in solver. When determining new extra-deps take version
number into account, not just the name. If the version is different
from the previous or in-snapshot version then its a new dependency.
Solver: retry with relaxed contraints if failed
With a recent commit solver was made to use packages in the snapshot as
contraints when solving. Within those constraints it tries to figure what
external packages are needed to build the package. But this strategy may not
always work. The package dependencies may want to use a package which is
already in the snapshot but requires a version different than in the snapshot.

To take care of this we first try with the snapshot packages as hard
constraints and if that fails we try solving with those packages as soft
constraints i.e. merely preferences.

This commit implements this fallback strategy. With this in place we should be
able to solve and init any package which can be solved by cabal. If any package
is not solved then either the package is broken or we have a bug in stack
solver.

This commit also added useful information in the status logs and suggestions
for next steps or resolutions in case solver fails.
Fix - ignore certain subdirs for stack init
There is code in place to ignore .git .stack-work etc. But it did not work due
to a trailing path separator mismatch in the logic. Fixed.
Solver: perform compiler compatibility check
With recent changes we perform compiler compatibilty check when choosing
snapshots for stack init. We need similar checks for stack solver as well.

This change uses the same code to do compiler compatibility check when using
the solver command. If the resolver specified in the stack.yaml does not have a
compiler which can work with the package we emit an appropriate messages
instead of going ahead and then showing difficult to understand cabal errors.
init/solver improve and share error checking code
1) Share the same error checking code across init and solver commands
2) Use relative paths in error messages to make it easier to understand
3) Massaged some error messages
init/solver handle duplicate cabal package case
When there are duplicate cabal packages in the directory tree or specified in
the config file we should detect the case and provide an appropriate error
message.
Tweak solver error messages
Remove the warnings about not providing a good build plan. Now we either
provide a successful plan or we fail and provide suggestions. We no longer
write an inconsistent stack.yaml.

Report relative filenames in error/status messages.
Solver: expunge unnecessary dependencies and flags
In addition to adding new flags and dependencies solver now also reports
and deletes any dependencies and flags which are no longer needed.
@mgsloan

This comment has been minimized.

mgsloan commented on src/Stack/BuildPlan.hs in a6ff6dd Jan 3, 2016

I think it's fine to ignore them for now. They're expected to mismatch with the snapshot.

@mgsloan

This comment has been minimized.

mgsloan commented on 6b0c4d0 Jan 3, 2016

  1. Note a behavior change for the 'stack init --resolver' command. Earlier it
    used to write the stack.yaml file even if it did not successully resolve all
    packages. Now it will fail in that case and suggest '--solver' to
    resolve the extra dependencies.

Hmm, I think it's useful to be able to generate a stack.yaml despite errors. While this is a little messy, how about adding to the meaning of --force? Now instead of Force overwriting of an existing stack.yaml if it exists it'd be Force writing a stack.yaml, even if a stack.yaml already exists, and even if there is no fully successful configuration.

Also, the description for --resolver should be updated. It still says Use the given resolver, even if not all dependencies are met.

This comment has been minimized.

Owner

harendra-kumar replied Jan 3, 2016

We can do that. I was in fact contemplating using --force but then had second thoughts about the use case. Solver uses --modify-stack-yaml for force overwriting so we may want to normalize that.

I missed the resolver help, will correct that.

This comment has been minimized.

mgsloan replied Jan 3, 2016

Relevant issue: commercialhaskell#1597

I don't really like using --force to mean this, and --modify-stack-yaml seems long. How about deprecating both of them and just having a flag like --modify or --modify-files? Should probably just create a new issue for this.

This comment has been minimized.

Owner

harendra-kumar replied Jan 3, 2016

I agree, both the existing options are a bit ugly. --modify sounds very general and 'files' in --modify-files is also pretty general, how about --update-config?

Also, we may want to take a step back and rethink the command organization. For example stack config becoming the master command for all config related operations? init and solver could be subcommands of that?

This comment has been minimized.

mgsloan replied Jan 3, 2016

I agree, both the existing options are a bit ugly. --modify sounds very general and 'files' in --modify-files is also pretty general, how about --update-config?

I like --update-config. However, I've changed my mind about this mattering. I figure the flag names don't matter much if the commands are changed to be interactive.

Also, the current flags are decent enough. --force feels fine for new / init, because it's forcing creation of something. Whereas stack solver --force seems weird, since really it's switching between "provide a config" and "set the config".

Also, we may want to take a step back and rethink the command organization. For example stack config becoming the master command for all config related operations? init and solver could be subcommands of that?

While this makes sense, I don't think it's worth changing at this point. Maybe if stack undergoes a major CLI restructuring this can be added in.

This comment has been minimized.

Owner

harendra-kumar replied Jan 4, 2016

I was going on the same line of thinking. --force sounded better for the init use case because it is creating a config so --update-config does not sound appropriate.

So, then I will make init --force create a potentially incomplete config. Maybe we can change the solver flag to --update-config.

This comment has been minimized.

mgsloan replied Jan 5, 2016

Sounds good!

@mgsloan

This comment has been minimized.

Collaborator

mgsloan commented Jan 3, 2016

Wow, this is great stuff, thanks a bunch for working on it!

Along with my commit comments, I noticed the following things that could be enhanced / fixed:

  • If I do stack init in a directory that has a stack.yaml in an ancestor directory, I get an error. Instead, I'd prefer that it only checked the current directory for a stack.yaml. For example, if ~/dir/stack.yaml exists but ~/dir/subdir/stack.yaml doesn't:
mgsloan@computer:~/dir/subdir$ stack init
Stack configuration file stack.yaml exists, use 'stack solver' to fix the existing config file or '--force' to overwrite it.
  • I also split off this issue #1593
  • There's an extra space towards the end of this message - Create pivot points for the solver by specifying some extra dependencies in stack.yaml and then use 'stack solver' to figure out the rest of the dependencies. Also, maybe instead of "Create pivot points for the solver by", say something like "Help the solver out by"
  • When some a local package is also in a snapshot, --solver doesn't work. For example, I get this output when building the latest lens repo. I think what needs to happen is that the local packages need to get filtered out of availablePkgs, used to write out the cabal.config file.
  • With an internal project, for some reason stack init --solver sets a flag like time-locale-compat:-old-locale (in the yaml, old-locale: false). time-locale-compat does not get added to extra-deps, even though it needs to be present in order to work, though this will change in the future. Oddly enough, the build works even if I remove the flag. Unfortunately, I haven't yet come up with a way to reproduce this in something I can publish.
@harendra-kumar

This comment has been minimized.

Collaborator

harendra-kumar commented Jan 3, 2016

If I do stack init in a directory that has a stack.yaml in an ancestor directory, I get an error.

Can't reproduce this, code too looks fine. Maybe you really had a file in the curdir and you thought you didn't?

  1. I am fixing the error message, I agree that it sounds too geeky and could be simpler.

  2. Even if the local and snapshot package versions conflict, the retry should work since it relaxes the snapshot constraints to preferences. Its a good idea to fix it though.

Looking at the solver output for lens, it seems there is a conflict between the base package of the snapshot and the constraints of gloss required by lens-examples. We need a snapshot with the correct base package - in other words we need a different GHC.

The compiler compatibility check that I have put in is pretty naive as it only works for the packages we are building, it cannot check whether the dependencies of the package are compiler compatible. We will have to parse the cabal error messages for that (yucky!) - or cabal itself can provide a distinctive error.

  1. I found a bug with the cabal flags parsing in the solver. I have fixed it, will push the commit soon. Maybe its related to that, try and see if the problem persists with that fix.

harendra-kumar added some commits Jan 3, 2016

Correctly parse the flags in cabal solver output
Also, always use the user specified flags as constraint.
Override snapshot with local versions for solver
When a package being built has a version in snapshot as well then the
solver constraints should always use the local version.
@mgsloan

This comment has been minimized.

Collaborator

mgsloan commented Jan 3, 2016

Can't reproduce this, code too looks fine. Maybe you really had a file in the curdir and you thought you didn't?

Ah, I'm not sure how much the parent stack.yaml affects things (other than potentially overriding some non-project options). However, here's a repro where if you have an invalid parent stack.yaml, things don't work: https://gist.github.com/mgsloan/2beeac1531b2ef07c651

@mgsloan

This comment has been minimized.

Collaborator

mgsloan commented Jan 4, 2016

Looking at the solver output for lens, it seems there is a conflict between the base package of the snapshot and the constraints of gloss required by lens-examples. We need a snapshot with the correct base package - in other words we need a different GHC.

Right, that's after it falls back to using preferred versions. When using hard constraints, it yields rejecting: lens-4.13.1 (global constraint requires ==4.13). In other words, it's writing down the lens constraint from the snapshot in the cabal config, but that mismatches with our local version.

I found this issue by running stack init on all the repos cloned by diagrams-ci (I had to modify the build-universe.sh script to just clone the repos and do nothing else).

I ran into a couple miscellaneous issue while looking into this: If I delete the version: field in the cabal file, I get this error:

cabal: Error parsing config file /tmp/cabal-solver11856/cabal.config:910:
Parse of field 'constraint' failed (expected a package name followed by a
constraint, which is either a version range, 'installed', 'source' or flags):
lens==

The compiler compatibility check that I have put in is pretty naive as it only works for the packages we are building, it cannot check whether the dependencies of the package are compiler compatible. We will have to parse the cabal error messages for that (yucky!) - or cabal itself can provide a distinctive error.

How about always using hard constraints for wired in packages?

  1. I found a bug with the cabal flags parsing in the solver. I have fixed it, will push the commit soon. Maybe its related to that, try and see if the problem persists with that fix.

Cool, I do get different behavior - more flags now (text:-integer-simple in addition to time-locale-compat:-old-locale). They don't seem to be necessary, as I can delete them. So, I'm still not sure what's going on there.

@harendra-kumar

This comment has been minimized.

Collaborator

harendra-kumar commented Jan 4, 2016

here's a repro where if you have an invalid parent stack.yaml

This warrants a separate issue. Perhaps we should avoid loading project config for init. Currently it uses withConfigAndLock which parses existing stack.yaml. We don't need to parse it for init, we only need to check the existence of it.

@mgsloan

This comment has been minimized.

Collaborator

mgsloan commented Jan 5, 2016

This warrants a separate issue. Perhaps we should avoid loading project config for init. Currently it uses withConfigAndLock which parses existing stack.yaml. We don't need to parse it for init, we only need to check the existence of it.

I've opened #1604 . Yeah, I think we should be able to avoid using withConfigAndLock

I am sorry, I never compiled with GHC 7.8.4. I have a fix for the CI build failure, should I rebase and squash with the last commit or just push another commit? Let me know if there is anything else to be taken care of as well. Many thanks for spending time on reviewing, testing and providing useful feedback.

No problem, that's what CI is for! Pushing another commit is fine, we have lots of "Fix build on GHC 7.8" commits..

Many thanks for spending time on implementing this! The next steps you've described sound good.

harendra-kumar added some commits Jan 5, 2016

Fix GHC 7.8.4 compilation
Instead of using mapM_ on a Map moved the assert deeper where it can be applied
on a single Map entry.
Solver: Update resolver when updating config
When solver is called with --resolver and resolver changes from what is there
in stack.yaml already then show changed resolver in output and also update it
in config when called with --modify-stack-yaml

Also, reformatted the output so as to distinguish regular messages a bit from
yaml formatted messages.
Solver: change option name --modify-stack-yaml
The solver option --modify-stack-yaml is changed to --update-config
Take flags into account to compute extra deps
Solver issues fixed with this commit are:

When solver returns flag changes for packages which are in snapshot, we update
the flags in stack.yaml but we do not put those packages in extra dependencies.
When we run stack build it complains about it.

We are not comparing the snapshot package flags (we compare only the version)
when determining extra dependencies. If the snapshot flags are different than
those the solver arrived at then we need to put those packages in extra
dependencies. Otherwise the plan will not build because the status of flags can
change dependencies.
@harendra-kumar

This comment has been minimized.

Collaborator

harendra-kumar commented Jan 5, 2016

It now gives a workable plan for stack itself using lts-2.22 and solver. Can you try it again now and see if you encounter any issues?

@harendra-kumar

This comment has been minimized.

Collaborator

harendra-kumar commented Jan 5, 2016

We will have to update the documentation as well. I guess I can do that via a separate PR?

@mgsloan

This comment has been minimized.

Collaborator

mgsloan commented Jan 6, 2016

Looks good! Feel free to do documentation in this PR, or a different one. I suppose I'd prefer including it with this one, but it's no big deal.

I'm still running into the issue where there are hard constraints which disagree with local package versions. Could you please try this repro?

git clone https://github.com/mgsloan/diagrams-ci
cd diagrams-ci
git checkout stack-repro-for-1583
./build-universe.sh    # Doesn't actuall build, due to my one commit on the repo
cd build-tmp
stack init --solver

(NOTE: I haven't actually tried these steps, but this is what I did. There may be typos)

@mgsloan

This comment has been minimized.

Collaborator

mgsloan commented Jan 6, 2016

Also with that repro, I get all the local packages having entries in flags, if I stack init --solver --force:

flags:
  diagrams-contrib: {}
  diagrams-lib: {}
  ...

It'd be best to leave these out!

@harendra-kumar

This comment has been minimized.

Collaborator

harendra-kumar commented Jan 6, 2016

I will try the diagrams-ci test. For documentation, I was only worried about merge conflicts if this PR stays longer. I will assess how many changes are needed and then we can decide to postpone or not.

I thought I filtered out the empty flags but might have missed something, will check.

@harendra-kumar

This comment has been minimized.

Collaborator

harendra-kumar commented Jan 6, 2016

The empty flags were getting through only in case of --force I fixed that. Another nice-to-have is to not write the flags if they are unchanged from defaults. Currently we are writing the default values of manual flags.

The solver errors are due to conflicting requirements of source packages in the diagrams suite. Here is the output I am getting:

cabal: Could not resolve dependencies:
trying: active-0.2.0.8 (user goal)
trying: diagrams-input-0.1 (user goal)
next goal: diagrams-lib (user goal)
rejecting: diagrams-lib-1.3.0.8, 1.3.0.7, 1.3.0.6, 1.3.0.5, 1.3.0.4, 1.3.0.3,
1.3.0.2, 1.3.0.1 (global constraint requires ==1.3)
trying: diagrams-lib-1.3
next goal: linear (dependency of active-0.2.0.8)
rejecting: linear-1.19.1.3, 1.20.3, 1.20.2, 1.20.1, 1.20, 1.19.1.2, 1.19.1.1,
1.19.1, 1.19 (conflict: diagrams-input => linear>=1.11.3 && <1.19)
rejecting: linear-1.18.3, 1.18.1.1, 1.18.1, 1.18.0.1, 1.18, 1.17.1, 1.17,
1.16.4, 1.16.2, 1.16.1, 1.16, 1.15.5, 1.15.4, 1.15.3, 1.15.2, 1.15.1,
1.15.0.1, 1.15, 1.14.0.1, 1.14 (conflict: diagrams-lib => linear>=1.20.1 &&
<1.21)
rejecting: linear-1.13, 1.12.1, 1.11.3, 1.11.2, 1.11.1, 1.11, 1.10.1.2,
1.10.1.1, 1.10.1, 1.10, 1.9.1, 1.9.0.1, 1.9, 1.8.1, 1.8, 1.7, 1.6, 1.4,
1.3.1.1, 1.3.1, 1.3, 1.2, 1.1.4, 1.1.2, 1.1.1, 1.0.1, 0.9.2, 0.9.1, 0.9, 0.8,
0.7, 0.6.1, 0.6, 0.5, 0.4.2.2, 0.4.2.1, 0.4.1, 0.2.0.2, 0.2 (conflict: active
=> linear>=1.14 && <1.21)
Dependency tree exhaustively searched.

Here source packages active, diagrams-input and diagrams-lib have a conflict on linear. Among themselves the dependencies are diagrams-input->diagrams-lib->active. So we can perhaps remove diagrams-input and anything that depends on that from our source packages and it should work. It indeed worked when I removed it.

So maybe a future enhancement can be to automatically suggest which user packages can be removed to get a working plan.

harendra-kumar added some commits Jan 6, 2016

Do not write empty flags with stack init --force
Filter out any empty flag values so that we do not end up having lots of
package flags set to empty values in stack.yaml.
Show flags along with dep errors in debug mode
The build plan checks show dependency errors about packages when a particular
build plan does not match. But flags can affect dependencies and are therefore
needed to verify the errors. This commit adds code to print the flags as well
in debug mode. Avoid in normal mode for the sake of brevity of info.
Solver: report missing packages
When only selected packages are added in stack.yaml solver will now print a
message about which packages are available under the current dir subtree which
are not found in the config.
init: Cleanup default flag values before writing
The resolver build plan may return source package flags set to their
default values. If so remove any flags set to default values before
writing them to stack.yaml.
@harendra-kumar

This comment has been minimized.

Collaborator

harendra-kumar commented Jan 6, 2016

A very clean stack.yaml is generated now. I have added logic to remove flags with default values as well.

I might push one more commit to report compiler incompatibility errors for each individual source package rather than once for the whole set. That way the user can easily identify exactly which package has problems.

Lay down corrective actions at each solver failure
Specify clear steps to help out the user when a 'stack init' or solver fails.
Make the help output more clear, concise and complete.

@mgsloan mgsloan merged commit 885a354 into commercialhaskell:master Jan 8, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mgsloan

This comment has been minimized.

Collaborator

mgsloan commented Jan 8, 2016

This is a great set of changes, thanks for all your excellent work!

I fixed a warning that cropped up due to now reporting more warnings.

@mgsloan

This comment has been minimized.

Collaborator

mgsloan commented Jan 8, 2016

RE the diagrams-haddock thing, you're probably right, I was just getting confused by the wording of cabal-install's output.

@harendra-kumar

This comment has been minimized.

Collaborator

harendra-kumar commented Jan 8, 2016

Great. Thanks! I will try to fix the user guide in the next couple of days.

@borsboom

This comment has been minimized.

Contributor

borsboom commented Jan 8, 2016

Looks like there's a syntax error in the haddocks:

src/Stack/Solver.hs:306:25:
    parse error on input ‘-- ^ src package constraints’
     (src/Stack/Types/Internal.hs:56)
    HasSupportsUnicode (src/Stack/Types/Internal.hs:59)
     (src/Stack/Types/Internal.hs:62)
    Sticky (src/Stack/Types/Internal.hs:65)
    HasSticky (src/Stack/Types/Internal.hs:69)
     (src/Stack/Types/Internal.hs:72)
@harendra-kumar

This comment has been minimized.

Collaborator

harendra-kumar commented Jan 8, 2016

Can we include haddock in CI?

mgsloan added a commit that referenced this pull request Jan 8, 2016

@mgsloan

This comment has been minimized.

Collaborator

mgsloan commented Jan 8, 2016

Sure! Added in 92da37a

@mgsloan

This comment has been minimized.

Collaborator

mgsloan commented Jan 8, 2016

Unfortunately, it doesn't work: #1623

@harendra-kumar

This comment has been minimized.

Collaborator

harendra-kumar commented Jan 8, 2016

I see you already fixed the haddock issues. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment