Implement build-tool-depends #4104

Ericson2314 · 2016-11-12T23:20:47Z

Implements much of #3708

Note the the exe component name is currently mandatory, unlike as specified in the original issue.

mention-bot · 2016-11-12T23:20:49Z

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

ezyang

Commit 1 review.

ezyang · 2016-11-12T23:37:38Z

Cabal/Distribution/Package.hs

 -- | Describes a legacy `build-tools`-style dependency on an executable
 --
 -- It is "legacy" because we do not know what the build-tool referred to. It
 -- could refer to a pkg-config executable (PkgconfigName), or an internal
 -- executable (UnqualComponentName). Thus the name is stringly typed.
--
+--g


ezyang · 2016-11-12T23:38:22Z

Cabal/Distribution/Package.hs

+                     PackageName
+                     UnqualComponentName -- name of executable component of package
+                     VersionRange
+                     deriving (Generic, Read, Show, Eq, Typeable, Data)


It is possible that you may want a helper function that takes an ExeDependency and gives a ComponentName (since the UnqualComponentName is only "implicitly" qualified.) I don't know if you'd actually use it.

Sure, doesn't hurt to add it now. Good hint on unqual vs qual for anyone working with the code base.

ezyang · 2016-11-12T23:39:38Z

Cabal/Distribution/Package/TextClass.hs

+
+  parse = do name <- parse
+             _ <- Parse.char ':'
+             exe <- parse


OK, so right now you are forced to specify the executable name. That's fine (although our users might complain :)

Besides laziness, I figured a blanket depend-on-all-the-exes dependency might complicate making the solver use deps on specific components. Then again we may need a blanket package dep anyways.

ezyang

Commit 3 (commit 2 was obvious, no problem.)

ezyang · 2016-11-12T23:44:13Z

Cabal/Distribution/FooBar.hs

+-- that we know how to desugar.
+--
+-- This should almost always be used instead of just accessing the
+-- `toolDepends` field directly.


Ok, if that's true there better be a comment on toolDepends telling people about it, or rename that field to something to discourage people from using it directoy.

I added some longer documentation to the relevant fields in the top commit. I'll also grep at the end to make sure nothing is lurking.

I could sed the field names too, but probably easiest to do that last.

ezyang · 2016-11-12T23:45:18Z

Cabal/Distribution/FooBar.hs

+--
+-- This is a tiny function, but used in a number of places. Note that the
+-- version bounds and components of the package are unchecked. This is because
+-- we sanitize exe deps so that the matching name implies these other


Where does this sanitizing happen.

PackageDescription.Check does sanitation happen in places besides that?

I think the overall codebase doesn't use "sanitization" as the term here. Furthermore, not all checks in PackageDescription.Check are necessarily run all the time: a package can still be built even if some of the checks fail. It is better to say that there is some invariant that is expected to hold; in that case the invariant description should be at the data type definition.

ezyang · 2016-11-12T23:45:47Z

Cabal/Distribution/PackageDescription/Check.hs

@@ -524,13 +525,42 @@ checkFields pkg =
        ++ "for example 'tested-with: GHC==6.10.4, GHC==6.12.3' and not "
        ++ "'tested-with: GHC==6.10.4 && ==6.12.3'."

-  , check (not (null buildDependsRangeOnInternalLibrary)) $
+  , check (not (null depInternalLibraryWithExtraVersion)) $


I didn't read this code carefully. Good tests would tickle all of these cases.

Whew! A lot of copy pasta but tested everything.

ezyang · 2016-11-12T23:47:29Z

Looking good. Obviously you still need to do the cabal-install bits.

Ericson2314 · 2016-11-16T15:37:54Z

How should I write the (presumably system) tests for those tests? I see the cabal files in PackageTests, but not sure where the code verifying stdout / stderr is.

23Skidoo · 2016-11-16T19:50:12Z

You may want to wait until #4095 is merged.

Ericson2314 · 2016-11-16T21:41:45Z

Hmm good idea

Ericson2314 · 2016-11-17T16:15:27Z

Hmm, so InternalLibrary4 explicitly tests that version bounds can trigger the use of an external library instead of internal one. This is in direct conflict with the buildDependsRangeOnInternalLibrary test this patch expands---it says version bounds don't matter because the internal library is always used. I don't really care which semantics we go with, except that we should be consistent for all 3 of "build-tools", "build-depends", and "tool-depends".

ezyang · 2016-11-17T17:00:50Z

@Ericson2314 We got rid of this feature in af24cef. Internal library should always be used.

Ericson2314 · 2016-11-26T00:36:21Z

~~Ok things left:~~ edit see new list in comment below

cabal-install's init
cabal-install's tests
Real name for Distribution.FooBar :D

ezyang

It's helpful if the commit message mentions the overall code motion that occurred; especially here were some things got deleted, and other things got added.

ezyang · 2016-11-26T07:39:24Z

Lookin' good!

ezyang · 2016-11-26T07:37:24Z

Cabal/Distribution/Backpack/ComponentsGraph.hs

-         , let toolname = mkUnqualComponentName name
-         , toolname `elem` map exeName (executables pkg_descr) ]
+         | (ExeDependency _ toolname _)
+            <- getAllInternalToolDependencies pkg_descr bi ]


ezyang · 2016-11-26T07:38:02Z

Cabal/Distribution/Types/BuildInfo.hs

+        -- precisely specify an executable in a package.
+        --
+        -- Unless use are very sure what you are doing, use the function in
+        -- `Distribution.FooBar` rather than accessing this field directly.


But... which function/!

ezyang · 2016-11-28T21:29:23Z

NB: this shouldn't be merged until "Ok things left:" list is done.

hvr · 2016-11-28T21:36:56Z

@ezyang what's the first todo item about? if it's about cabal init I don't think it's essential for this PR

ezyang · 2016-11-28T21:48:00Z

Looking at the source code, it doesn't look like the new-build code has been adjusted for tool-depends yet. But maybe no changes were needed; I don't know if it works or not; the tests would be able to say so.

Modeled after the extern_build_tools test

…ild-tools` `build-tools` is described in terms of `build-tool-depends`, just as it is implemented in terms of it.

Ericson2314 · 2017-01-06T22:20:14Z

Ok, docs are done also all that remains is cabal init, and making the PATH more fine-grained a la #4104 (comment) . OK to do those in future PRs?

ezyang · 2017-01-06T22:35:12Z

Yep. Anything in particular you want reviewed before merging?

hvr · 2017-01-07T08:16:47Z

let's get this merged soon before the PR starts to "conflict-rot" :-)

This is a bug from `first build-tool-depends` PR: haskell#4104

This is a bug from `first build-tool-depends` PR: haskell#4104 Also, remove TODO about factoring things out. The exact set of deps filter here is no longer so globally relevant, and `Distribution.Simple.BuildToolDepends` exposes better filters which are.

ezyang · 2017-01-13T12:52:00Z

@Ericson2314 It would be great if we could have a changelog entry too!

phadej · 2017-01-13T13:41:04Z

I can't wait till this gets to hvr's ppa repository so I can try it out wild!

hvr · 2017-01-14T14:33:28Z

@phadej ...I got the hint ;-)

Ericson2314 · 2017-01-14T19:16:25Z

@ezyang OK I will do that in the PR that https://github.com/Ericson2314/cabal/tree/build-tool-depends-glob turns into.

ezyang · 2017-01-16T03:37:55Z

Thanks!

This is a bug from `first build-tool-depends` PR: haskell#4104 Also, remove TODO about factoring things out. The exact set of deps filter here is no longer so globally relevant, and `Distribution.Simple.BuildToolDepends` exposes better filters which are.

ezyang reviewed Nov 12, 2016

View reviewed changes

Ericson2314 force-pushed the tool-depends branch 4 times, most recently from f6d7e02 to 3269f83 Compare November 16, 2016 13:54

Ericson2314 force-pushed the tool-depends branch 2 times, most recently from b1487b4 to fd11fbc Compare November 16, 2016 16:29

Ericson2314 mentioned this pull request Nov 17, 2016

Drop version check when resolving package names. #4021

Merged

Ericson2314 force-pushed the tool-depends branch from fd11fbc to e298eee Compare November 18, 2016 23:44

Ericson2314 force-pushed the tool-depends branch from e298eee to 30b4fa8 Compare November 26, 2016 00:11

Ericson2314 force-pushed the tool-depends branch from 2419889 to e11029c Compare November 26, 2016 00:40

ezyang reviewed Nov 26, 2016

View reviewed changes

Ericson2314 force-pushed the tool-depends branch 4 times, most recently from 30e62d9 to 0b62af1 Compare December 6, 2016 16:19

Ericson2314 and others added 2 commits January 6, 2017 16:50

Add end-to-end cabal-install test for "build-tool-depends"

9db90bf

Modeled after the extern_build_tools test

Document build-tool-depends in the manual, and rewrite that for `bu…

a6cee38

…ild-tools` `build-tools` is described in terms of `build-tool-depends`, just as it is implemented in terms of it.

Ericson2314 force-pushed the tool-depends branch from acfb6aa to a6cee38 Compare January 6, 2017 21:52

Ericson2314 mentioned this pull request Jan 6, 2017

Only build needed executables in a package #4217

Closed

Ericson2314 merged commit 5e4377b into haskell:master Jan 7, 2017

Ericson2314 deleted the tool-depends branch January 7, 2017 16:09

Ericson2314 changed the title ~~[WIP] implement tool-depends~~ implement tool-depends Jan 9, 2017

Ericson2314 changed the title ~~implement tool-depends~~ Implement tool-depends Jan 9, 2017

Ericson2314 added a commit to Ericson2314/cabal that referenced this pull request Jan 11, 2017

Cabal should look for unknown build-tools entries on the PATH too

83d9483

This is a bug from `first build-tool-depends` PR: haskell#4104

Ericson2314 mentioned this pull request Jan 17, 2017

[WIP] Add glob deps #4241

Closed

Ericson2314 changed the title ~~Implement tool-depends~~ Implement build-tool-depends Jan 24, 2017

hvr mentioned this pull request Jul 31, 2017

Support for setup-tool-depends #4648

Open

Ericson2314 mentioned this pull request Jul 5, 2018

Support for run-tool-depends #5411

Open

Implement build-tool-depends #4104

Implement build-tool-depends #4104

Conversation

Ericson2314 commented Nov 12, 2016 • edited Loading

mention-bot commented Nov 12, 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

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

Ericson2314 Nov 26, 2016 • edited Loading

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 Nov 12, 2016

Ericson2314 commented Nov 16, 2016

23Skidoo commented Nov 16, 2016

Ericson2314 commented Nov 16, 2016

Ericson2314 commented Nov 17, 2016 • edited Loading

ezyang commented Nov 17, 2016

Ericson2314 commented Nov 26, 2016 • edited Loading

ezyang left a comment • edited Loading

Choose a reason for hiding this comment

ezyang commented Nov 26, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ezyang commented Nov 28, 2016

hvr commented Nov 28, 2016

ezyang commented Nov 28, 2016

Ericson2314 commented Jan 6, 2017 • edited Loading

ezyang commented Jan 6, 2017

hvr commented Jan 7, 2017

ezyang commented Jan 13, 2017

phadej commented Jan 13, 2017

hvr commented Jan 14, 2017

Ericson2314 commented Jan 14, 2017

ezyang commented Jan 16, 2017

Ericson2314 commented Nov 12, 2016 •

edited

Loading

Ericson2314 Nov 26, 2016 •

edited

Loading

Ericson2314 commented Nov 17, 2016 •

edited

Loading

Ericson2314 commented Nov 26, 2016 •

edited

Loading

ezyang left a comment •

edited

Loading

Ericson2314 commented Jan 6, 2017 •

edited

Loading