-
Notifications
You must be signed in to change notification settings - Fork 711
Solver: Enforce dependencies on executables (fixes #4781). #4884
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
Conversation
Fixing the TODO caused the solver to report conflicting constraints in the opposite order in some cases, but the change didn't seem to have a negative impact on the clarity of the error messages. Here is an example of the change from running "cabal install --dry-run nerf --index-state=2017-11-04T18:36:30Z -v3": [_19] trying: template-haskell-2.11.1.0/installed-2.1... (dependency of tagged) [_20] next goal: pretty (dependency of template-haskell) -[_20] rejecting: pretty-1.1.3.3/installed-1.1... (conflict: pretty => deepseq==1.4.2.0/installed-1.4..., text => deepseq>=1.1.0.0 && <1.4) +[_20] rejecting: pretty-1.1.3.3/installed-1.1... (conflict: text => deepseq>=1.1.0.0 && <1.4, pretty => deepseq==1.4.2.0/installed-1.4...) [_20] rejecting: pretty-1.1.3.5, pretty-1.1.3.4, pretty-1.1.3.3, pretty-1.1.3.2, pretty-1.1.3.1, pretty-1.1.2.1, pretty-1.1.2.0, pretty-1.1.1.3, pretty-1.1.1.2, pretty-1.1.1.1, pretty-1.1.1.0, pretty-1.1.0.0, pretty-1.0.1.2, pretty-1.0.1.1, pretty-1.0.1.0, pretty-1.0.0.0 (conflict: template-haskell => pretty==1.1.3.3/installed-1.1...) [_20] fail (backjumping, conflict set: pretty, template-haskell, text)
This commit splits the 'Conflicting' constructor of 'FailReason' into five different constructors, for different types of dependency conflicts. This change makes it easier to print different error messages for different conflicts, though it doesn't change any of the error messages yet. The refactoring caused a small change to the error message for conflicts caused by self-dependencies.
…pendencies. This commit changes the field of type 'IsExe' in the 'Dep' data type to type 'Maybe UnqualComponentName'. It also adds the executable name to error messages that previously just contained "(exe)".
This commit adds two checks to the validation phase of the solver: 1. It checks that each newly chosen package instance contains all executables that are required from that package so far. 2. It checks that each new build tool dependency that refers to a previously chosen package can be satisfied by the executables in that package. This commit also fixes a TODO related to solver log messages. Previously, it was possible for the log to associate an incorrect executable name with a dependency.
The pre-new-build code path sets the solver parameter 'solveExecutables' to false, which causes the solver to ignore all build-tools and build-tool-depends dependencies. This commit adds a test for ignoring each of the two types of dependencies.
This all sounds absolutely great, but can you please add a short note to the changelog? |
Thanks. I updated the changelog and added a few more tests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR looks good to me, so unless @kosmikus objects I'm going to merge it tomorrow.
Merged. Thanks, @grayjay! |
Thanks for the review! |
Please include the following checklist in your PR:
[ci skip]
is used to avoid triggering the build bots.Please also shortly describe how you tested your change. Bonus points for added tests!
Main commits:
Solver: Use more specific failure reasons for dependency conflicts.
This commit splits the
Conflicting
constructor ofFailReason
into fivedifferent constructors, for different types of dependency conflicts. This
change makes it easier to print different error messages for different
conflicts, though it doesn't change any of the error messages yet.
The refactoring caused a small change to the error message for conflicts caused
by self-dependencies.
Solver: Store names of required executables for build-tool-depends dependencies.
This commit changes the field of type
IsExe
in theDep
data type totype
Maybe UnqualComponentName
. It also adds the executable name to errormessages that previously just contained "(exe)".
Solver: Enforce dependencies on executables (fixes #4781).
This commit adds two checks to the validation phase of the solver:
It checks that each newly chosen package instance contains all executables
that are required from that package so far.
It checks that each new build tool dependency that refers to a previously
chosen package can be satisfied by the executables in that package.
This commit also fixes a TODO related to solver log messages. Previously, it was
possible for the log to associate an incorrect executable name with a
dependency.
Add tests for build-tool-depends dependencies using the solver DSL.
Test that the solver ignores build tool dependencies in legacy mode.
The pre-new-build code path sets the solver parameter
solveExecutables
tofalse, which causes the solver to ignore all build-tools and build-tool-depends
dependencies. This commit adds a test for ignoring each of the two types of
dependencies.
I compared the performance on this branch (cbf73ac) with master (26426fc) to see the effect of the extra validation steps. I wrote a cabal file with one library build-depends field to test the solver with new-build. (Only new-build handles build tool dependencies.) Then I ran
cabal new-build --dry-run --index-state=2017-11-04T18:36:30Z --no-count-conflicts
, with two sets of dependencies in the build-depends field. I tried to choose one set of dependencies that caused a long run time and another that created a deep search tree. I turned off conflict counting to prevent the solver from finishing too quickly in the first case. I also compared the -v3 log from master and the branch to ensure that the change didn't have any effect on conflicts, since I only wanted to measure the overhead of checking for executables. Each measurement is the average of three trials.It looks like this change didn't have much effect in either case.
/cc @kosmikus