Rework build target handling #4179

dcoutts · 2016-12-19T11:41:45Z

This is a fairly long patch series and this is partly just request for review/comment.

It does a whole lot of refactoring in the handling of command line build targets and related code. This includes rearranging things a bit in the project planning (splitting initial project config steps out) and project orchestration.

One motivation for the reorganisation is to properly support what test, bench, and run need from target handling, since they are slight variations on what is needed for build. Another motivation is doing better detection and reporting of user mistakes in targets. And another motivation is to be able to do most error reporting prior to running the solver. This is important for usability because many user mistakes or misunderstandings will result in solver failure, when really the problem was starting from the wrong place or asking for the wrong thing.

It includes:

all build target
component "filter" targets like: tests, pkgfoo:tests, ./:tests, all:tests
rationalised fully-qualified unambiguous target syntax (for scripts)
stub implementations for the commands (new-) run, test and bench.
rewritten --help text for all the new-* commands.

It has recently been rebased against master and as part of that merge it adds a number of TODOs for places where the master branch is doing something silly. These can be cleaned up as subsequent patches within this series or later.

There are still a few bits to finish, including proper text for a number of error cases that are currently just represented by constructors with 'show'. Also missing is proper testing. There's also still a number of TODOs.

mention-bot · 2016-12-19T11:41:49Z

@dcoutts, thanks for your PR! By analyzing the history of the files in this pull request, we identified @ezyang, @bgamari and @arybczak to be potential reviewers.

ezyang

I like refactors. We should merge refactors.

In general, this patchset adds some more features, but these features are not documented in the user's guide, nor are they tested. It would be really helpful if they were documented at least, esp., since it's not obvious from the code what new things were added.

ezyang · 2016-12-19T14:12:36Z

cabal-install/Distribution/Client/ProjectPlanning.hs

+                        -> Map UnitId [ComponentTarget]
+elaboratePackageTargets elaboratedPlan =
+    Map.mapWithKey $ \pkgid targets ->
+      let Just (InstallPlan.Configured pkg) =


(Intended invariant for this function that all the UnitIds exist?)

Note that this is only an intermediate step during the refactoring. This function gets completely removed in subsequent patches. It's replaced by the combination of availableTargets and resolveTargets which are much more careful about error checking.

ezyang · 2016-12-19T14:18:18Z

cabal-install/Distribution/Client/ProjectPlanning.hs

+        (Just [tgt], TargetActionRepl)    -> elab { elabReplTarget = Just tgt }
+        (Just _,     TargetActionHaddock) -> elab { elabBuildHaddocks = True }
+        (Just _,     TargetActionRepl)    ->
+          error "pruneInstallPlanToTargets: multiple repl targets"


s/pruneInstallPlanToTargets/setRootTargets/

I think that was deliberate. pruneInstallPlanToTargets is the only exported function that uses it.

ezyang · 2016-12-19T14:23:17Z

cabal-install/Distribution/Client/ProjectPlanning.hs

+--
+-- Note that the type parameter is used to help enforce that command
+-- implementations can only select targets that can actually be built (by
+-- forcing them to return the @k@ value for the selected targets).


Is it only used for that? Or is there a legit reason to store a key? (Should we use a GADT instead?)

I think I'm missing what you're getting at exactly. The type that actually gets used in place of the 'k' is (UnitId, ComponentName).

Add that to the comment!

ezyang · 2016-12-19T14:26:53Z

cabal-install/Distribution/Client/ProjectPlanning.hs

+    -- to know about them and have enough info about them to explain why they
+    -- cannot be selected.
+    --
+    -- We refer to these reconstructed missing components as fake targets.


Why not just call them non-buildable targets?

ezyang · 2016-12-19T15:34:12Z

cabal-install/Distribution/Client/ProjectPlanning.hs

+    -- to which this graph node corresponds. For graph nodes that are whole
+    -- packages this makes perfect sense, but for nodes corresponding to
+    -- components this sort-of amounts to taking the cross-product of the set
+    -- of components of a package with itself. Obviously this will give rise


I know what you mean, but it would be clearer with an example.

ezyang · 2016-12-19T15:40:59Z

cabal-install/Distribution/Client/ProjectOrchestration.hs

        "The package " ++ display (buildTargetPackage t) ++ " is in the "
     ++ "project but it is not a locally unpacked package, so  "

-reportBuildTargetProblem (BuildTargetOptionalStanzaDisabled _) = undefined
+showTargetProblem _ = undefined


Since you wibbled this line, I couldn't help but notice the pattern match was partial...

Yeah, this bit of the error message formatting remains a TODO

ezyang · 2016-12-19T15:48:17Z

cabal-install/Distribution/Client/CmdRepl.hs

+
+            --TODO: [required eventually] handle no targets case
+            when (Map.null targets) $
+              fail "TODO handle no targets case"


Maybe it is work to write code to nicely explain why the error happened but surely we can do better than "TODO handle no targets case"

Producing nice messages is one of the things remaining to be done.

ezyang · 2016-12-19T15:55:10Z

cabal-install/Distribution/Client/CmdBuild.hs

+     ++ "encounter problems\nplease file issues at "
+     ++ "https://github.com/haskell/cabal/issues and if you\nhave any time "
+     ++ "to get involved and help with testing, fixing bugs etc then\nthat "
+     ++ "is very much appreciated.\n"


Would be nice to not copy paste this text...

Yeah, I should move that to ProjectOrchestration

ezyang · 2016-12-19T16:04:25Z

cabal-install/Distribution/Client/CmdBench.hs

+    buildOutcomes <- runProjectBuildPhase verbosity buildCtx
+    runProjectPostBuildPhase verbosity buildCtx buildOutcomes
+  where
+    verbosity = fromFlagOrDefault normal (configVerbosity configFlags)


A general comment that these stubs are really wordy :/

ezyang · 2016-12-19T16:11:42Z

cabal-install/Distribution/Client/TargetSelector.hs

@@ -222,6 +222,8 @@ data TargetString =
   | TargetString2 String String
   | TargetString3 String String String
   | TargetString4 String String String String
+   | TargetString5 String String String String String
+   | TargetString7 String String String String String String String


Really. You inspired me to have a proper unambigious form for scripts and I realised the existing forms were not necessarily unambigious and were not very consistent or sane. So these are long but they're consistent: each bit gets a qualification, the long forms are:

:pkg:foo:lib:foo :pkg:foo:lib:foo:module:Data.Foo :pkg:foo:exe:foo:file:Main.hs

Count them: 5 and 7 components.

ezyang · 2016-12-19T16:18:12Z

Also, this patchset makes the breaking change, of making cabal new-build no longer imply cabal new-build all, which breaks the PackageTests\Regression\T3436\cabal.test.hs regression test. Changing the invocation of cabal "new-build" [] to cabal "new-build" ["all"] should fix it.

BardurArantsson · 2016-12-24T11:30:37Z

cabal-install/Distribution/Client/ProjectPlanning.hs

-    -- cannot be selected.
+    -- We have a somewhat awkward problem here. We need to know /all/ the
+    -- components from /all/ the packages because these are the things that
+    -- users could refer to. Unfortunately at this stage the elaborated install


"Unfortunately," (missing comma)

BardurArantsson · 2016-12-24T11:31:32Z

cabal-install/Distribution/Client/ProjectPlanning.hs

+    -- We can recover the missing components but it's not exactly elegant. For
+    -- a graph node corresponding to a component we still have the information
+    -- about the package that it came from, and this includes the names of
+    -- /all/ the other components in the package. So in principle this lets us


"So," (missing comma... I think?)

BardurArantsson · 2016-12-24T11:32:21Z

cabal-install/Distribution/Client/ProjectPlanning.hs

+    -- components.
+    --
+    -- Consider for example a package with 3 exe components: foo, bar and baz
+    -- where foo and bar are buildable but baz is not. So the plan contains


", but" (comma)

(I was really hoping to find actual typos and things, but it seems I've become Comma Nazi :) )

BardurArantsson · 2016-12-24T11:32:47Z

cabal-install/Distribution/Client/ProjectPlanning.hs

+    --
+    -- Consider for example a package with 3 exe components: foo, bar and baz
+    -- where foo and bar are buildable but baz is not. So the plan contains
+    -- nodes for the components foo and bar. Now we look at each of these two


"Now, " (comma)? Or do you mean this in the sense of "we used to do X, now we do Y"?

The pruneInstallPlanToTargets function had two main passes. Split the first pass into two passes. This simplifies things a bit and helps clarify the fact that setting the root targets is something we do prior to taking the dep closure.

And push compatSubComponentTargets inside nubComponentTargets. The motivation for this is we'll be exporting nubComponentTargets and using it from the ProjectOrchestration as part of revised target handling.

Previously it took the specification for the targets to build in terms of PackageTarget which is a high-ish level specification of which components to build. It is high level in the sense that it says things like BuildDefaultComponents which still have to be elaborated into the specific components. This elaboration from the higher level intention of what to build into what components to build specifically has to vary between different commands (build, repl, run, test, bench) and it can also fail. These features make it not a good fit to do within plan pruning. Instead it is better to have plan pruning take the lower level specification of what to build in terms of components and to do the conversion of high level to low level separately. That will allow us to do it differently for different commands and deal with failures better. This patch does one step: it changes pruneInstallPlanToTargets from taking [PackageTarget] to [ComponentTarget] and as an intermediate step in refactoring it converts the existing elaboratePackageTargets code to convert from the [PackageTarget] to [ComponentTarget] which is then used by the calling code in ProjectOrchestration. The next step will be to replace the elaboratePackageTargets with something more flexible and that can handle errors better.

This is one major part of a new implementation of translating from high level user intentions of what to build into the detail of what specific components to build. This part collects and summarises information on what component are available as targets. Each available target one has enough status info to determine if we can possibly select the target or if not why not. This information will be used by each command (build, repl, test etc) to select the specific targets that a user intention refers to (and to provide them enough information to be able to produce reasonable error messages). Collecting and summarising this info is unfortunately non-trivial because as part of install plan elaboration we omit components that cannot be built but these are still components that users could try to refer to, so we have to reconstruct the info about the omitted components. It's plausible that we may want to revise the elaborated install plan representation to make this easier.

Previously the resolveUserBuildTargets would return a PackageName which meant we would subsequently select all versions of that package that occur in the project during target selection (resolveAndCheckTargets). Using PackageId means we only pick one version (though in future we may still need to disambiguate multiple instances, or targets from multiple active profiles)

selectTargets would also prune to to the targets, and optionally prune to dependencies only. These aspects are now pulled out into the callers in the various Cmd{Build,Repl,etc} modules. It will make more sense to do it this way once we're doing more explicit error checking in the callers.

Instead of expressing the intention in terms of PackageTargets, it now takes helper functions that select the components to use for a package target or a component target. These helpers are given the set of AvailableTargets and they can do error checking and return failures. This allows the build,configure,haddock,repl implementations to have their own behaviour and error checking and reporting for selecting targets. The full details of error reporting are not covered yet.

The --only-dependencies it is only used in build, so we move the rendering of the errors that arise from that to that build command module so the error message text can be more context specific.

And also reject --only-dependencies for the haddock command.

It looks like CmdHaddock was based on the CmdConfigure template and so copied some bits that don't make sense for it. Change those bits to be the same as for build. In particular for printing the plan, the configure command always behaves as if it is in --dry-run mode, but that's not appropriate for the haddock command. The other change is to follow the standard pattern for the runProjectBuildPhase / runProjectPostBuildPhase and the handling of the --dry-run option, which is handled internally by those and does not need to be handled explicitly in CmdHaddock.

This is slightly tricky stuff to write since it has to be concise, clear, accurate and make sense without too much context. This and the examples should be reviewed with comparison to the user guide.

Rather than having it appear in the [other] section. I've just guessed at where it ought to go. Suggestions welcome.

So far they just build the exe bench and don't do the next steps, but they do at least select the right components to build. Also bring the existing test command into sync with the others.

The way it is currently implemented is incompatible with the new target checking code, since it really selects everything, including things that are not buildable. This leads directly to failre when we check that the things the user has asked to build are in fact buildable.

We already have the SubComponentTarget type so we can use this to better factor the BuildTarget type, and simplify conversions.

dcoutts · 2017-03-07T03:33:21Z

19 new patches, everything from "Change TargetSelector to remove TargetCwdPackage" onwards. So if you were happy with the last lot then you only need to start there. Again, everything is broken out into reviewable chunks.

The first 57 patches are rebased but otherwise not changed.

The main new things are proper integration tests covering all the cases the code checks for, and human readable error messages for these things.

dcoutts · 2017-03-07T03:38:09Z

One thing I've not properly tested is if I have broken the existing test command (or at least it's CLI), so that's a TODO.

ezyang · 2017-03-07T03:41:46Z

Yaaay! Ship it when it's green.

23Skidoo · 2017-03-07T13:44:33Z

Would be nice to have this in 2.0 to make it easier to backport patches, as this is such a massive change.

dcoutts · 2017-03-08T22:00:25Z

@ezyang I don't understand the cause of this failure in NewBuild/T3978. Yes the error message is different, but I cannot see how this is a change I've made. It looks like it's getting past an error message in the solver, and so hitting a different error message in the cli target resolution code. And I didn't change the solver, so I don't get it yet.

If you have a sec, would you mind taking a look?

…to dcoutts-target-handling

ezyang · 2017-03-08T22:33:24Z

In HEAD, the expected output is this:

# cabal new-build
Resolving dependencies...
Error:
    Dependency on unbuildable libraries: p-1.0
    In the stanza 'library'
    In the inplace package 'q-1.0'

This is not an error from the dep solver; it's from ProjectPlanning. If you merge your branch with master and then run the tests, you'll see the complete -v log from the test run which should clarify things, but it does look like with your patch you are now dropping into the Setup script before erroring. This means the unbuildable_external_lib_deps test in ProjectPlanning is not firing.

How does this test work? We do it this way:

            unbuildable_external_lib_deps =
                filter (null . elaborateLibSolverId mapDep) external_lib_dep_sids

checking if elaborateLibSolverId returns any components which matched. Now, I seem to recall that you modified ProjectPlanning to put empty packages in the build plan, even if we weren't going to build anything; could it be that this is thwarting the test?

ezyang · 2017-03-08T23:10:56Z

And indeed, this diff fixes the problem:

Client/ProjectPlanning.hsl/Distribution/Client/ProjectPlanning.hs b/cabal-install/Distribution 
index 03a503d4f..158980a18 100644
--- a/cabal-install/Distribution/Client/ProjectPlanning.hs
+++ b/cabal-install/Distribution/Client/ProjectPlanning.hs
@@ -1429,7 +1429,10 @@ elaborateInstallPlan verbosity platform compiler compilerprogdb pkgConfigDB
       where is_lib (InstallPlan.PreExisting _) = True
             is_lib (InstallPlan.Configured elab) =
                 case elabPkgOrComp elab of
-                    ElabPackage _ -> True
+                    -- If it doesn't have a public library, we should not
+                    -- count it as a valid solver dep.  This will let us
+                    -- catch if an upstream dep is unbuildable.
+                    ElabPackage _ -> PD.hasPublicLib (elabPkgDescription elab)
                     ElabComponent comp -> compSolverName comp == CD.ComponentLib
             is_lib (InstallPlan.Installed _) = unexpectedState

But there is something a bit questionable going on here, so maybe some more refactoring is in order.

Signed-off-by: Edward Z. Yang <ezyang@cs.stanford.edu>

ezyang · 2017-03-09T02:02:40Z

Two windows failures are legit. One is easy to fix but the other I'm going to have to debug properly.

ezyang · 2017-03-09T06:59:25Z

Despite being unable to repro on my Windows 10 box, I'm fairly confident that the failing test case in "Target selectors" has to do with case-insensitivity on Windows/Mac OS X. Essentially, what I think is happening is when we pass Q, we perform a test to see if a file or directory with this name exists. And in fact, there IS one that exists: the lower case directory q for the package, which matches because the file system is case insensitive.

Experimentally, the only way I can get the "display" case of a file is to do something like listDirectory; canonicalizePath does not seem to give me back the display case of the path. And in any case, if we're on a case insensitive filesystem and someone gives us a file name, we should probably also be case insensitive.

I think there are two ways to deal:

Get information about whether or not the typed case matches the display case, and use that to order our preferences so that we behave as if the file system was case sensitive, and only fall back on case insensitive information as last resort.
Rewrite the test case so that this ambiguity is not present, punting the problem. Cabal on Windows will continue to act weirdly if you accidentally pick up a local directory.

CC @dcoutts

Ericson2314 · 2017-03-09T17:08:01Z

I would prefer syntax like pkg:test:* to pkg:tests. Other things (solving constraints, wildcard build deps) would use that syntax and I'd like the consistency.

23Skidoo · 2017-03-09T17:16:49Z

That'd conflict with shell's pattern expansion:

$ echo pkg:tests:*
zsh: no matches found: pkg:tests:*

Maybe pkg:test:all is an acceptable alternative, given that you can do new-build all.

Ericson2314 · 2017-03-09T17:46:11Z

@23Skidoo I suppose if we want to reserve all across the board I like that. Quoting "*" is pretty annoying, I'll admit.

Ericson2314 · 2017-03-09T18:34:05Z

@grayjay you were using any, right?

ezyang · 2017-03-10T00:32:18Z

I don't see why we can't support the asterisk syntax...

Signed-off-by: Edward Z. Yang <ezyang@cs.stanford.edu>

grayjay · 2017-03-10T05:20:21Z

@Ericson2314 Qualified constraints use any, but it's not too late to change it.

dcoutts · 2017-03-14T10:37:17Z

@Ericson2314 @grayjay I'm totally open to adding/adjusting the target syntax (provided it's coherent). Having multiple aliases for the same thing is also ok (though if it introduces more ambiguity then it has knock-on implications).

dcoutts · 2017-03-14T10:42:09Z

@ezyang thanks for the review, fixing things and merging. I see you added a workaround for the windows case insensitivity thing. So that remains as a TODO for me.

Ericson2314 · 2017-03-14T17:19:57Z

I just want to be consistent. I like * because it wouldn't parse as a name anyways, but reserving any across the board is also consistent. any everywhere, * everywhere, or both is fine with me! :)

Rework build target handling

ezyang approved these changes Dec 19, 2016

View reviewed changes

BardurArantsson reviewed Dec 24, 2016

View reviewed changes

dcoutts force-pushed the target-handling branch 2 times, most recently from 3e1eea4 to 7a3ae03 Compare December 29, 2016 17:31

ezyang mentioned this pull request Jan 11, 2017

[RFC] Add 'cabal new-path' command #4220

Closed

ezyang mentioned this pull request Jan 18, 2017

WIP: new-test #4244

Merged

dmwit mentioned this pull request Jan 26, 2017

Target handling #4264

Closed

dcoutts added 21 commits February 27, 2017 23:39

Tidy-up and adjust comments in pruneInstallPlanPass1

49c1af5

Refactoring: make nubComponentTargets top level

5fbf262

And push compatSubComponentTargets inside nubComponentTargets. The motivation for this is we'll be exporting nubComponentTargets and using it from the ProjectOrchestration as part of revised target handling.

Drop now unused PackageTarget and elaboratePackageTargets

5db46c6

Update docs for resolveTargets and in export list

929f056

Rename some of the TargetProblem constructors

a6913df

Move rendering of CannotPruneDependencies to CmdBuild

259a88c

The --only-dependencies it is only used in build, so we move the rendering of the errors that arise from that to that build command module so the error message text can be more context specific.

Add in checks or TODOs for null/multiple target cases

c82d086

And also reject --only-dependencies for the haddock command.

Rearange Cmd{Build,Haddock,Repl} import lists for consistency

67795df

Update the --help text for all the new-* commands

d414b1b

This is slightly tricky stuff to write since it has to be concise, clear, accurate and make sense without too much context. This and the examples should be reviewed with comparison to the user guide.

Add reconfigure into a section in the --help output

9e03a50

Rather than having it appear in the [other] section. I've just guessed at where it ought to go. Suggestions welcome.

Put the new-* commands in their own section in --help

b07d0c9

Add initial templates for new run & bench commands

f2a8bdf

So far they just build the exe bench and don't do the next steps, but they do at least select the right components to build. Also bring the existing test command into sync with the others.

Simplfy the representation of BuildTarget using SubComponentTarget

07bb0ca

We already have the SubComponentTarget type so we can use this to better factor the BuildTarget type, and simplify conversions.

dcoutts force-pushed the target-handling branch from 68cd5b2 to 1022c14 Compare March 7, 2017 03:26

Merge branch 'target-handling' of https://github.com/dcoutts/cabal in…

55daa5b

…to dcoutts-target-handling

ezyang added 3 commits March 8, 2017 15:21

Don't say a package provides a library if it has no public library.

1d33008

Signed-off-by: Edward Z. Yang <ezyang@cs.stanford.edu>

Fix new-test.

4ff8550

Signed-off-by: Edward Z. Yang <ezyang@cs.stanford.edu>

Fix some Windows test errors.

32a152f

Signed-off-by: Edward Z. Yang <ezyang@cs.stanford.edu>

Work around for case-insensitivity bug.

6334d73

Signed-off-by: Edward Z. Yang <ezyang@cs.stanford.edu>

ezyang force-pushed the target-handling branch from 492eb2f to 6334d73 Compare March 10, 2017 04:01

ezyang merged commit 56070e4 into haskell:master Mar 10, 2017

hvr mentioned this pull request Mar 15, 2017

[nix-local-build] new-run, new-test, new-bench, new-exec #3638

Closed

ezyang added a commit to ezyang/cabal that referenced this pull request Mar 21, 2017

Merge pull request haskell#4179 from dcoutts/target-handling

b2ee927

Rework build target handling

grayjay mentioned this pull request Jan 31, 2018

'cabal new-build test:test-suite' doesn't force tests to be enabled. #3923

Open

qrilka mentioned this pull request Nov 18, 2020

stack repl subpackage compile time cwd is different from stack build subpackage one commercialhaskell/stack#5421

Closed

Rework build target handling #4179

Rework build target handling #4179

Conversation

dcoutts commented Dec 19, 2016 • edited Loading

mention-bot commented Dec 19, 2016

ezyang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ezyang commented Dec 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcoutts commented Mar 7, 2017

dcoutts commented Mar 7, 2017

ezyang commented Mar 7, 2017

23Skidoo commented Mar 7, 2017

dcoutts commented Mar 8, 2017

ezyang commented Mar 8, 2017

ezyang commented Mar 8, 2017

ezyang commented Mar 9, 2017

ezyang commented Mar 9, 2017

Ericson2314 commented Mar 9, 2017

23Skidoo commented Mar 9, 2017

Ericson2314 commented Mar 9, 2017

Ericson2314 commented Mar 9, 2017

ezyang commented Mar 10, 2017

grayjay commented Mar 10, 2017

dcoutts commented Mar 14, 2017

dcoutts commented Mar 14, 2017

Ericson2314 commented Mar 14, 2017

dcoutts commented Dec 19, 2016 •

edited

Loading