[WIP] Add glob deps #4241

Ericson2314 · 2017-01-17T02:40:02Z

This is really really WIP, but I wanted to ask whether my top commit, adding an extra field to exe deps in the solver, will help route that information to ProjectPlanning for the plan in #4104 (comment), or whether I'm barking up the wrong tree.

#4217

mention-bot · 2017-01-17T02:40:04Z

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

grayjay · 2017-01-17T06:52:50Z

There is a related issue for refactoring the dependency solver to solve for components instead of packages: #4087

Ericson2314 · 2017-01-17T15:11:21Z

@grayjay thanks! Yeah, I view this role as a bridge towards that---metadata ferried along by the solver for now, but actually inspected by it in the future.

edit

At a minimum, the solver should keep track of dependencies between packages' components.

Oh great, we are totally on the same page! :)

23Skidoo · 2017-01-17T15:27:33Z

CI failure is due to -Werror:

Distribution/Simple/Build.hs:33:1: Warning:
    The import of ‘Distribution.Types.ExeDependency’ is redundant
      except perhaps to import instances from ‘Distribution.Types.ExeDependency’
    To import instances alone, use: import Distribution.Types.ExeDependency()

Ericson2314 · 2017-01-17T15:28:41Z

@23Skidoo thanks! Though heh isn't quite ready to be merged anyways :)

ezyang · 2017-01-19T17:41:49Z

The top commit seems fine to me. What happens if I depend on multiple executables from the same package; does it show up twice?

Ericson2314 · 2017-01-19T17:58:16Z

@ezyang hmm probably? I haven't looked at the solving code in much detail---I mainly just changed the definition of Dep and then dealt with the fallout as ghc fed it to me. Perhaps it should take a list of exes so I can combine them somewhere?

@grayjay maybe this is anticipating per-componenting solving more than I thought! :/

ezyang · 2017-01-19T18:23:19Z

@Ericson2314 My suggestion is to take a look at the output you get after the change. (Hard to get this output? Add some logging!)

Having thought about this more, it may not be necessary to propagate executable dependencies from the solver; ProjectPlanning can recompute them from scratch. Take a look at how we handle exe_deps0 in elaborateSolverToComponents: the first thing we do run elaborateExeSolverId/elaborateExePath on them, which essentially looks up the package that defined the executable(s) and then adds them as deps. With the information you have from the package description, it would be a simple matter to filter this list down to the true dependencies.

(BTW, I think this code is probably in need of some refactoring!)

Ericson2314 · 2017-01-20T17:39:45Z

I added this to the 2.0 milestone, because without it build-tool-depends isn't so well implemented.

Ericson2314 · 2017-01-20T23:41:53Z

Ok, did things just in ProjectPlanning as @ezyang described (I still have my old commit in another branch for future work :)).

All that's left here is https://github.com/haskell/cabal/pull/4241/files#diff-a6de9cdd16372d3e6c4ec16d4d1703a1R575, I need to route the expanded wildcard back to cabal somehow. Does this mean adding flags to Cabal?

CC @hvr: the wildcard stuff was your idea so hopefully you have an idea here? :)

ezyang

If this isn't WIP anymore, the commits look fine but we need TESTS.

ezyang · 2017-01-22T20:36:14Z

Cabal/Distribution/Simple/Configure.hs

+          -- Second, we collect any build-tools entry we don't know how to
+          -- desugar. We'll never have no idea how to build them, so we just
+          -- hope they are already on the PATH.
+          let unknownBuildTools =


I just realized that I may have communicated the situation here a little poorly. It is true that Setup was modified to pick up unknown build-tools from PATH, but in fact, all the testing code we have does not seem to actually exercise this case, as it is always assumed the build-tools refers to an internal dep. So a test here would be appreciated.

Ericson2314 · 2017-01-22T23:10:02Z

@ezyang Yes, definitely tests, but I do need the route the expanded wildcard back to Cabal too, right?

ezyang · 2017-01-22T23:43:39Z

I guess so (although it's not altogether clear what that looks like; I don't think Cabal has any way of conveniently finding out what the wildcard will glob).

Ericson2314 · 2017-01-23T16:22:48Z

I think the bug-fix portion of this should at least make 2.0 if not wildcards, so I'm going to factor those apart. (And get some tests!)

I'd like to instead change the solver to route the used components through (baby steps towards per-component solving), rather than reconstruct it post-hoc from the Package Description, but this gets the job done for now.

They look like pkg:* <optional version bound> and denote a dependency on every executable in the package. In the case of internal package dependencies, it is easy to expand the wildcard for internal dependencies, but not possible in Cabal alone for external ones, as the exact set of exe deps depends on the version resolved in cabal-install's build plan.

ezyang · 2017-02-03T22:48:52Z

Hello @Ericson2314, what would you like to be done on this PR specifically?

Ericson2314 · 2017-02-04T00:20:43Z

@ezyang It's in limbo until someone has a good idea for routing the glob expansion back to Cabal. Not a priority for 2.0 I'd think unless @hvr differs (it's back-compat with explicit :* syntax).

ezyang · 2017-02-04T00:29:14Z

OK. Maybe this presents the case for extending the database to also include executables, or maybe teaching Cabal how to interact with the Nix store in some way.

gbaz · 2021-08-12T02:17:58Z

#4217 looks like it was resolved without this pr. can this be closed?

jneira · 2022-05-11T19:53:26Z

closing optimistically after no feedback for the previous comment, thanks all!

23Skidoo requested a review from kosmikus January 17, 2017 15:30

Ericson2314 force-pushed the build-tool-depends-glob branch 5 times, most recently from bd0b3de to 484d159 Compare January 18, 2017 21:04

Ericson2314 added this to the 2.0 milestone Jan 20, 2017

Ericson2314 force-pushed the build-tool-depends-glob branch from 484d159 to 42938e3 Compare January 20, 2017 23:26

ezyang added the blocked: action-required label Jan 22, 2017

ezyang requested changes Jan 22, 2017

View reviewed changes

Ericson2314 mentioned this pull request Jan 23, 2017

Unknown build-tools are looked up on the PATH -- fix and test #4260

Merged

Ericson2314 removed this from the 2.0 milestone Jan 23, 2017

Ericson2314 added 2 commits January 24, 2017 11:05

Filter build-tool-resolving components to what's needed

bbd5803

I'd like to instead change the solver to route the used components through (baby steps towards per-component solving), rather than reconstruct it post-hoc from the Package Description, but this gets the job done for now.

Ericson2314 changed the title ~~[WIP] Fix #4217 and add glob deps~~ [WIP] Add glob deps Jan 24, 2017

Ericson2314 mentioned this pull request Jan 24, 2017

Fix #4217 #4262

Merged

Ericson2314 force-pushed the build-tool-depends-glob branch from 42938e3 to b9cfe19 Compare January 25, 2017 01:21

Mistuke mentioned this pull request Feb 24, 2017

Update GHC to 8.0.2, pull in upstream Arch scripts msys2/MINGW-packages#2204

Merged

jneira closed this May 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Add glob deps #4241

[WIP] Add glob deps #4241

Ericson2314 commented Jan 17, 2017 •

edited by ezyang

Loading

mention-bot commented Jan 17, 2017

grayjay commented Jan 17, 2017

Ericson2314 commented Jan 17, 2017 •

edited

Loading

23Skidoo commented Jan 17, 2017

Ericson2314 commented Jan 17, 2017

ezyang commented Jan 19, 2017

Ericson2314 commented Jan 19, 2017 •

edited

Loading

ezyang commented Jan 19, 2017

Ericson2314 commented Jan 20, 2017

Ericson2314 commented Jan 20, 2017 •

edited

Loading

ezyang left a comment

ezyang Jan 22, 2017

Ericson2314 Jan 22, 2017

Ericson2314 commented Jan 22, 2017

ezyang commented Jan 22, 2017

Ericson2314 commented Jan 23, 2017 •

edited

Loading

ezyang commented Feb 3, 2017

Ericson2314 commented Feb 4, 2017

ezyang commented Feb 4, 2017 •

edited

Loading

gbaz commented Aug 12, 2021

jneira commented May 11, 2022

[WIP] Add glob deps #4241

[WIP] Add glob deps #4241

Conversation

Ericson2314 commented Jan 17, 2017 • edited by ezyang Loading

mention-bot commented Jan 17, 2017

grayjay commented Jan 17, 2017

Ericson2314 commented Jan 17, 2017 • edited Loading

23Skidoo commented Jan 17, 2017

Ericson2314 commented Jan 17, 2017

ezyang commented Jan 19, 2017

Ericson2314 commented Jan 19, 2017 • edited Loading

ezyang commented Jan 19, 2017

Ericson2314 commented Jan 20, 2017

Ericson2314 commented Jan 20, 2017 • edited Loading

ezyang left a comment

Choose a reason for hiding this comment

ezyang Jan 22, 2017

Choose a reason for hiding this comment

Ericson2314 Jan 22, 2017

Choose a reason for hiding this comment

Ericson2314 commented Jan 22, 2017

ezyang commented Jan 22, 2017

Ericson2314 commented Jan 23, 2017 • edited Loading

ezyang commented Feb 3, 2017

Ericson2314 commented Feb 4, 2017

ezyang commented Feb 4, 2017 • edited Loading

gbaz commented Aug 12, 2021

jneira commented May 11, 2022

Ericson2314 commented Jan 17, 2017 •

edited by ezyang

Loading

Ericson2314 commented Jan 17, 2017 •

edited

Loading

Ericson2314 commented Jan 19, 2017 •

edited

Loading

Ericson2314 commented Jan 20, 2017 •

edited

Loading

Ericson2314 commented Jan 23, 2017 •

edited

Loading

ezyang commented Feb 4, 2017 •

edited

Loading