Fix #2066: buildDepends is just an accessor function, not a field #4383

Ericson2314 · 2017-03-09T05:47:33Z

Prerequisite for #4265

mention-bot · 2017-03-09T05:47:35Z

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

Ericson2314 · 2017-03-09T05:55:14Z

@ezyang this and munge (#4382) have a trivial conflict---I made this off master and submitted it now cause I figure it poses fewer problems (BC, opacity), and can get merged first.

I hope by likewise splitting out more pieces of #4265, I can finally get that one working.

ezyang · 2017-03-09T06:18:23Z

I'd like to see other opinions on the approach taken here, which is to replace the buildDepends computation with a modification of the BuildInfo in each component to incorporate the computed dependency information that otherwise went in buildDepends (whatever that is; I'm not exactly sure what is being computed here.)

Some prior discussion on the subject is here: #3466 (comment) where @kosmikus comments on the fact that allow-older operates by modifying the package description to change the version bounds. In the end we accepted the modification code, because (a) we needed it somehow in the Setup support for allow-older, and (b) it wasn't convenient to modify the solver to do it otherwise. But in the case of buildDepends there is a clear alternative, which is to compute the dep info as an auxiliary data structure and then it pass it around where it is needed.

Ericson2314 · 2017-03-18T02:01:37Z

What's all this "quick brown fix" stuff in the test. We're not a font renderer!

ezyang · 2017-03-18T02:16:32Z

It's testing that we don't deadlock when the test executable emits a lot of output.

Ericson2314 · 2017-04-04T21:39:05Z

@ezyang this is passing except for some appveyor thing which looks unrelated. I agree this is not the most elegant fix, but might be expedient for 2.0, insofar that it enables LibDependency?

23Skidoo · 2017-04-04T22:03:19Z

AppVeyor failure doesn't looks spurious to me. Restarted the AppVeyor build.

Ericson2314 · 2017-04-04T22:07:05Z

@23Skidoo indeed it doesn't look spurious, but I didn't touch that file or anything related either? More spooky CI!

23Skidoo · 2017-04-04T22:09:38Z

Maybe a dependency changed or some CPP'ed code bitrotted.

ezyang · 2017-04-05T02:49:17Z

Build failure looks triggered by release of optparse-applicative-0.13.2.0, which appears to be BC breaking. If you add an upper bound that should solve the problem.

As for whether to merge, I think I'd prefer this to be a package deal with LibDependency changes

Ericson2314 · 2017-04-05T04:28:13Z

Sounds good

Ericson2314 · 2017-04-06T15:33:08Z

@ezyang pcapriotti/optparse-applicative@0.13.1...0.13.2 looks back-compat to me, you sure about that?

ezyang · 2017-04-07T05:44:47Z

Yeah, looks like I'm wrong and the actual problem is a missing Data.Semigroup import...

Ericson2314 · 2017-04-07T16:02:09Z

@ezyang I feel like the right solution would be Distribution.Compat.Prelude, except that's a private module.

ezyang · 2017-04-12T06:10:49Z

What I find really perplexing is that I can't repro your bug. :/

Ericson2314 · 2017-04-13T04:20:43Z

To double check, you tried on 7.4? Yeah I'm confused why master and other PRs don't break with this.

Ericson2314 · 2018-01-30T23:07:55Z

Perhaps waiting for #4265 was not the right approach as I (with @kmicklas's help) are only getting back to it a year later. I'd like to fix #4203 too, but I don't think this digs that hole much deeper. As soon as there as any mechanism to add/override extra constraints, I'd hope it can easily be used for all such things.

Ericson2314 · 2018-01-30T23:10:04Z

Cabal/Distribution/Types/Component.hs

@@ -62,6 +63,14 @@ componentBuildInfo :: Component -> BuildInfo
 componentBuildInfo =
  foldComponent libBuildInfo foreignLibBuildInfo buildInfo testBuildInfo benchmarkBuildInfo

+lensBuildInfo :: (BuildInfo -> BuildInfo) -> (Component -> Component)


We now depend on some Lenz stuff so maybe this can go elsewhere / use type synonym in the type?

Yes, we already have:

cabal/Cabal/Distribution/Types/Component.hs

Line 41 in 736c6f4

instance L.HasBuildInfo Component where

phadej · 2018-01-31T08:25:00Z

Cabal/Distribution/PackageDescription/Configuration.hs

+      redoBD bd_lens = bd_lens $ \bi -> bi { targetBuildDepends = fromDepMap depMap }
+
+      lensLibBD :: (BuildInfo -> BuildInfo) -> (Library -> Library)
+      lensLibBD f = \l -> l { libBuildInfo = f $ libBuildInfo l }


we have that lens. it's over L.buildInfo. We also have targetBuildDepends (which can apply directly on Library):

cabal/Cabal/Distribution/Types/BuildInfo/Lens.hs

Line 182 in 736c6f4

targetBuildDepends :: Lens' a [Dependency]

BTW, lensLibBD is not Lens, it's Setter ;)

Ericson2314 · 2018-01-31T23:08:36Z

I can't figure out why the parsing tests would fail for one GHC but not another. But maybe its just spurious? 8.2.2 certainly worked fine locally.

Ericson2314 · 2018-01-31T23:25:13Z

Ah, its cause I need to rebase

- Post flag-solving, replace buildDepends with solved dependencies. - In most cases, just collect the build-depends of *enabled* components. B4ut in a few, we have no enable spec on hand and must collect them all

Eventually, configuring will be rewritten so extra constraints do note pollute the checks. When that happens this commit should be reverted.

ezyang · 2018-02-01T04:00:47Z

Cabal/Distribution/PackageDescription/Check.hs

@@ -1369,7 +1369,7 @@ checkCabalVersion pkg =
      _                   -> False

    versionRangeExpressions =
-        [ dep | dep@(Dependency _ vr) <- buildDepends pkg
+        [ dep | dep@(Dependency _ vr) <- allBuildDepends pkg


This makes my BC senses tingle. What's the BC story for custom setups that call buildDepends. Can we preserve the old semantics under the old name?

https://github.com/23Skidoo/all-custom-setups says buildDepends it isn't really used anywhere, pandoc-1.6 is from 2010

~/all-custom-setups$ git grep -e buildDepends origin/all-custom-setups origin/all-custom-setups:open-pandoc-1.5.1.1/Setup.hs: let highlightingSupport = any isHighlightingKate $ buildDepends pkg origin/all-custom-setups:pandoc-1.5.0.1/Setup.hs: let highlightingSupport = any isHighlightingKate $ buildDepends pkg origin/all-custom-setups:pandoc-1.5.1.1/Setup.hs: let highlightingSupport = any isHighlightingKate $ buildDepends pkg origin/all-custom-setups:pandoc-1.5.1/Setup.hs: let highlightingSupport = any isHighlightingKate $ buildDepends pkg origin/all-custom-setups:pandoc-1.5/Setup.hs: let highlightingSupport = any isHighlightingKate $ buildDepends pkg origin/all-custom-setups:pandoc-1.6.0.1/Setup.hs: let highlightingSupport = any isHighlightingKate $ buildDepends pkg origin/all-custom-setups:pandoc-1.6/Setup.hs: let highlightingSupport = any isHighlightingKate $ buildDepends pkg ogre@bigtest:~/all-custom-setups$ git grep -e buildDepends origin/all-custom-setups origin/all-custom-setups:open-pandoc-1.5.1.1/Setup.hs: let highlightingSupport = any isHighlightingKate $ buildDepends pkg origin/all-custom-setups:pandoc-1.5.0.1/Setup.hs: let highlightingSupport = any isHighlightingKate $ buildDepends pkg origin/all-custom-setups:pandoc-1.5.1.1/Setup.hs: let highlightingSupport = any isHighlightingKate $ buildDepends pkg origin/all-custom-setups:pandoc-1.5.1/Setup.hs: let highlightingSupport = any isHighlightingKate $ buildDepends pkg origin/all-custom-setups:pandoc-1.5/Setup.hs: let highlightingSupport = any isHighlightingKate $ buildDepends pkg origin/all-custom-setups:pandoc-1.6.0.1/Setup.hs: let highlightingSupport = any isHighlightingKate $ buildDepends pkg origin/all-custom-setups:pandoc-1.6/Setup.hs: let highlightingSupport = any isHighlightingKate $ buildDepends pkg

Yeah per the big comment at https://github.com/haskell/cabal/pull/4383/files#diff-d02dbf8d6911c6afb8c8cb25ebc6ea9cL127, anything that uses it seems bound to be incorrect anyways.

ezyang · 2018-02-01T04:05:13Z

Cabal/Distribution/Types/PackageDescription.hs

@@ -398,6 +389,13 @@ enabledBuildInfos pkg enabled =
 -- * Utils
 -- ------------------------------------------------------------

+allBuildDepends :: PackageDescription -> [Dependency]


Need docs, same with the other function below.

FYI, in other PR I made allBuildInfo return all BuildInfo, also non-buildable. I don't know if they should be filtered out here.

enabledBuildDepends below does that.

ezyang · 2018-02-01T04:06:01Z

Cabal/Distribution/PackageDescription/Configuration.hs

+    flattenTst (n, t) = testFillInDefaults $ (fst $ ignoreConditions t)
+      { testName = n }
+    flattenBm (n, t) = benchFillInDefaults $ (fst $ ignoreConditions t)
+      { benchmarkName = n }


What happened in this diff?

I removed the accumulation of deps and just did the flattening of components. Because of the removal of https://github.com/haskell/cabal/pull/4383/files#diff-12c8f4f2a1b901f8cbc37a619afafcb7L520, we no longer need that accumulation.

ezyang · 2018-02-01T04:06:42Z

Cabal/Distribution/PackageDescription/Configuration.hs

+    (PDNull, x) -> x  -- actually this should not happen, but let's be liberal
+    where
+      redoBD :: L.HasBuildInfo a => a -> a
+      redoBD = set L.targetBuildDepends $ fromDepMap depMap


What does this do?

It's been a while, but pretty sure this is the problematic bit via your choice a year ago. The difference between the new version and the old version is now we updated the components' targetBuildDepends with the depMaps of each target, whereas before those just updated the top-level build depends at https://github.com/haskell/cabal/pull/4383/files#diff-12c8f4f2a1b901f8cbc37a619afafcb7L450.

ezyang · 2018-02-01T04:07:52Z

Well, I guess it looks plausible; we'll see how the tests go.

Are you planning to fix the spurious warnings? What's the game plan there?

Ericson2314 · 2018-02-01T16:16:47Z

@ezyang happy to fix #4203 and the spurious warnings next if that's what you prefer. I don't think it should be too hard?

Forgot to do this in #4383 [skip ci]

…ends" This reverts commit ea75854, reversing changes made to 602dfdc. See #5119 for the reason for reverting this.

Revert "Merge pull request #4383 from Ericson2314/no-legacy-build-dep…

Ericson2314 force-pushed the no-legacy-build-depends branch 2 times, most recently from d540a69 to fc7657a Compare March 15, 2017 05:47

Ericson2314 force-pushed the no-legacy-build-depends branch from fc7657a to 26b1593 Compare March 18, 2017 00:06

Ericson2314 force-pushed the no-legacy-build-depends branch from 26b1593 to 36eb3ef Compare April 4, 2017 15:03

Ericson2314 closed this Apr 5, 2017

Ericson2314 deleted the no-legacy-build-depends branch April 5, 2017 16:31

Ericson2314 reopened this Jan 30, 2018

Ericson2314 force-pushed the no-legacy-build-depends branch from 36eb3ef to bbc583a Compare January 30, 2018 23:00

Ericson2314 commented Jan 30, 2018

View reviewed changes

phadej reviewed Jan 31, 2018

View reviewed changes

Ericson2314 force-pushed the no-legacy-build-depends branch 2 times, most recently from d9cd6dc to a36ef41 Compare January 31, 2018 19:12

Ericson2314 and others added 4 commits January 31, 2018 18:26

Fix haskell#2066: buildDepends is just an accessor function, not a field

6154796

- Post flag-solving, replace buildDepends with solved dependencies. - In most cases, just collect the build-depends of *enabled* components. B4ut in a few, we have no enable spec on hand and must collect them all

Fix Cabal parser tests now that buildDepends is no longer a field

e3ebcb2

Fix Cabal check tests, changes resulted in more alphabetization

0d41fbd

Allow erroneous warning for now.

73713ef

Eventually, configuring will be rewritten so extra constraints do note pollute the checks. When that happens this commit should be reverted.

Ericson2314 force-pushed the no-legacy-build-depends branch from a36ef41 to 73713ef Compare January 31, 2018 23:38

ezyang reviewed Feb 1, 2018

View reviewed changes

Ericson2314 mentioned this pull request Feb 1, 2018

Don't implement allow-newer/allow-older by modifying PackageDescription #4203

Open

Add docs for allBuildDepends and enabledBuildDepends

3af7f3e

Ericson2314 merged commit ea75854 into haskell:master Feb 2, 2018

Ericson2314 added a commit that referenced this pull request Feb 2, 2018

Update Cabal/changelog with buildDepends removal

540b257

Forgot to do this in #4383 [skip ci]

Ericson2314 mentioned this pull request Feb 2, 2018

Update Cabal/changelog with buildDepends removal #5099

Merged

4 tasks

Ericson2314 added a commit that referenced this pull request Feb 2, 2018

Update Cabal/changelog with buildDepends removal

e4f872e

Forgot to do this in #4383 [skip ci]

Ericson2314 added a commit that referenced this pull request Feb 2, 2018

Update Cabal/changelog with buildDepends removal

99fc71e

Forgot to do this in #4383 [skip ci]

Ericson2314 added a commit that referenced this pull request Feb 2, 2018

Update Cabal/changelog with buildDepends removal

5dfcbdf

Forgot to do this in #4383 [skip ci]

This was referenced Feb 2, 2018

Cabal release for GHC 8.4 #5075

Closed

Spurious warning: The package has an extraneous version range for a dependency on an internal library #5119

Open

23Skidoo added a commit that referenced this pull request Feb 14, 2018

Revert "Merge pull request #4383 from Ericson2314/no-legacy-build-dep…

b43cb7f

…ends" This reverts commit ea75854, reversing changes made to 602dfdc. See #5119 for the reason for reverting this.

23Skidoo added a commit that referenced this pull request Feb 14, 2018

Revert "Merge pull request #4383 from Ericson2314/no-legacy-build-dep…

d6831f7

…ends" This reverts commit ea75854, reversing changes made to 602dfdc. See #5119 for the reason for reverting this.

23Skidoo added a commit that referenced this pull request Feb 15, 2018

Merge pull request #5148 from haskell/2.2-revert-4383

aeebf3e

Revert "Merge pull request #4383 from Ericson2314/no-legacy-build-dep…

Fix #2066: buildDepends is just an accessor function, not a field #4383

Fix #2066: buildDepends is just an accessor function, not a field #4383

Conversation

Ericson2314 commented Mar 9, 2017

mention-bot commented Mar 9, 2017

Ericson2314 commented Mar 9, 2017

ezyang commented Mar 9, 2017

Ericson2314 commented Mar 18, 2017

ezyang commented Mar 18, 2017

Ericson2314 commented Apr 4, 2017 • edited Loading

23Skidoo commented Apr 4, 2017

Ericson2314 commented Apr 4, 2017

23Skidoo commented Apr 4, 2017

ezyang commented Apr 5, 2017

Ericson2314 commented Apr 5, 2017

Ericson2314 commented Apr 6, 2017

ezyang commented Apr 7, 2017

Ericson2314 commented Apr 7, 2017

ezyang commented Apr 12, 2017

Ericson2314 commented Apr 13, 2017

Ericson2314 commented Jan 30, 2018

Choose a reason for hiding this comment

phadej Jan 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phadej Jan 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ericson2314 commented Jan 31, 2018

Ericson2314 commented Jan 31, 2018

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

phadej Feb 1, 2018 • 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 Feb 1, 2018

Ericson2314 commented Feb 1, 2018

Ericson2314 commented Apr 4, 2017 •

edited

Loading

phadej Jan 31, 2018 •

edited

Loading

phadej Jan 31, 2018 •

edited

Loading

phadej Feb 1, 2018 •

edited

Loading