Skip to content

Fix #4155 with pkg:libcomp in build-depends#4265

Closed
Ericson2314 wants to merge 4 commits into
haskell:masterfrom
Ericson2314:build-depends-internal
Closed

Fix #4155 with pkg:libcomp in build-depends#4265
Ericson2314 wants to merge 4 commits into
haskell:masterfrom
Ericson2314:build-depends-internal

Conversation

@Ericson2314
Copy link
Copy Markdown
Collaborator

@Ericson2314 Ericson2314 commented Jan 26, 2017 •
edited
Loading

Working on finishing @ezyang's branch. I also cleaned up the various build-dep related fields in the process. Everything builds but tests fail.

@ezyang note I renamed BuildDependency to LibDependency to match ExeDependency. I suspect when componentization is complete we might be able to remove plain Dependency 😃.

@Ericson2314 Ericson2314 changed the title [WIP] Fix #4206 with pkg:libcomp in build-depends [WIP] Fix #4155 with pkg:libcomp in build-depends Jan 26, 2017
@Ericson2314 Ericson2314 mentioned this pull request Jan 26, 2017
Closed
@Ericson2314 Ericson2314 force-pushed the build-depends-internal branch from bdd5ead to 8646b09 Compare January 26, 2017 02:27
@Ericson2314 Ericson2314 added this to the 2.0 milestone Jan 26, 2017
@Ericson2314 Ericson2314 force-pushed the build-depends-internal branch 3 times, most recently from 15f2ddf to 48051f9 Compare January 26, 2017 20:26
@Ericson2314
Copy link
Copy Markdown
Collaborator Author

Ericson2314 commented Jan 26, 2017

Hmm OK it builds now---fixing cabal-install was way easier than I at first thought. Waiting to see what CI says.

@ezyang
Copy link
Copy Markdown
Contributor

ezyang commented Jan 27, 2017

@Ericson2314 Tell me when you want review.

@Ericson2314
Copy link
Copy Markdown
Collaborator Author

Ericson2314 commented Jan 27, 2017

@ezyang Sure will do. Step one is how can I best run haddock locally? I think that is causing the failure.

@ezyang
Copy link
Copy Markdown
Contributor

ezyang commented Jan 27, 2017

It's pretty annoying. Workaround here: #3535 (comment)

@Ericson2314
Copy link
Copy Markdown
Collaborator Author

Ericson2314 commented Jan 27, 2017 

Distribution/Backpack/ConfiguredComponent.hs:116:21: error:
    parse error on input ‘-- *package* exists, but doesn't have this’

@ezyang that's something that you wrote :). More importantly, it looks fine to me?

@ezyang
Copy link
Copy Markdown
Contributor

ezyang commented Jan 27, 2017

OH. This is a dumb Haddock thing: you're not allowed to have an asterisk at the beginning of a -- because Haddock will try to interpret as a section header. MY BANE.

@Ericson2314 Ericson2314 force-pushed the build-depends-internal branch from 48051f9 to 440d1e3 Compare February 9, 2017 06:08
@Ericson2314
Copy link
Copy Markdown
Collaborator Author

Ericson2314 commented Feb 9, 2017

Sorry to have been slow about this. Hopefully the Haddock error goes away now.

@Ericson2314 Ericson2314 changed the title [WIP] Fix #4155 with pkg:libcomp in build-depends Fix #4155 with pkg:libcomp in build-depends Feb 9, 2017
@Ericson2314
Copy link
Copy Markdown
Collaborator Author

Ericson2314 commented Feb 9, 2017

Removing the WIP in hopes tests pass! :)

@Ericson2314
Copy link
Copy Markdown
Collaborator Author

Ericson2314 commented Feb 9, 2017

Hmm, I do not understand the Travis error.

@Ericson2314 Ericson2314 mentioned this pull request Feb 11, 2017
Merged
@Ericson2314 Ericson2314 force-pushed the build-depends-internal branch 5 times, most recently from e8d4281 to ae6cb1f Compare March 1, 2017 22:33
This was referenced Mar 2, 2017
Closed
Merged
@Ericson2314 Ericson2314 force-pushed the build-depends-internal branch from a811630 to 79174af Compare March 3, 2017 08:01
@Ericson2314 Ericson2314 mentioned this pull request Mar 9, 2017
Merged
@Ericson2314 Ericson2314 force-pushed the build-depends-internal branch from 79174af to c4222ee Compare March 15, 2017 05:47
@Ericson2314 Ericson2314 force-pushed the build-depends-internal branch 2 times, most recently from 2dc6210 to 90130d8 Compare March 18, 2017 01:08
@Ericson2314
Copy link
Copy Markdown
Collaborator Author

Ericson2314 commented Mar 18, 2017

Locally tested, this is getting pretty close to being working, though I can probably shrink the diff a bit (gratuitous changes to PackageIndex for example).

@Ericson2314 Ericson2314 force-pushed the build-depends-internal branch from a19efc8 to a1bd5ca Compare April 5, 2017 16:14
@Ericson2314 Ericson2314 force-pushed the build-depends-internal branch from a1bd5ca to 2709dd8 Compare April 27, 2017 19:47
@23Skidoo 23Skidoo modified the milestones: 2.2, 2.0 May 5, 2017
@23Skidoo
Copy link
Copy Markdown
Member

23Skidoo commented May 5, 2017

Remilestoned.

@Ericson2314
Copy link
Copy Markdown
Collaborator Author

Ericson2314 commented May 6, 2017

I won't be back for a few more days, but how should we accommodate the syntax landed in 2.0? Warning? Deprecation?

@ezyang
Copy link
Copy Markdown
Contributor

ezyang commented May 7, 2017

I'm not sure. We may need to support it permanently.

@Ericson2314
Copy link
Copy Markdown
Collaborator Author

Ericson2314 commented May 17, 2017 •
edited
Loading

Oh when I said "deprecate", I meant ban it for min version > 2.0, but support it for min version = 2---deem it legacy syntax but not actually break existing packages.

I've love to release 2.0.1 doublethinking this into a bugfix, but I'm not holding out hope for such devilry :).

@Ericson2314
Copy link
Copy Markdown
Collaborator Author

Ericson2314 commented Jan 26, 2018 •
edited
Loading

Copying old Travis links for comparison (cause I find them hard to find after the fact)

Unfortunately the downstream travises I was most interested in are already gone.

@kmicklas kmicklas force-pushed the build-depends-internal branch from 2709dd8 to 9e0ed42 Compare January 26, 2018 22:53
@kmicklas
Copy link
Copy Markdown
Collaborator

kmicklas commented Jan 26, 2018

@Ericson2314 and I rebased this on master. We will try to finish it before it bitrots this time :)

@Ericson2314
Copy link
Copy Markdown
Collaborator Author

Ericson2314 commented Jan 27, 2018 •
edited
Loading

Is it possible to get this into 2.1? We'd like to ban legacy-style sub library dependencies as soon as possible, so that they are are only allowed with cabal-version >= 2.0 projects.

If so, it would probably be a good idea to break this up a bit, e.g. land the #2066 immediately.

@ezyang
Copy link
Copy Markdown
Contributor

ezyang commented Jan 27, 2018

You have no objections from me, please use your discretion.

@Ericson2314 Ericson2314 removed this from the 2.2 milestone Feb 12, 2018
@Ericson2314
Copy link
Copy Markdown
Collaborator Author

Ericson2314 commented Feb 12, 2018

This will depend on refactors that @phadej doesn't want for 2.2, so changed milestone.

Ericson2314 and others added 4 commits March 6, 2018 18:46
@Ericson2314
Inline mkConfiguredComponent
063dab7
@ezyang @Ericson2314
Distinguish between internal and external libraries in build-depends
554468f 
Fixes haskell#4155.

We create a new `LibDependency` just used for parsing `build-depends`
entries for now, but I hope it has a bright future in the brave new
per-component world.

Already in 'Cabal', this type will be used instead of 'Dependency' in
most cases, implemented in the following commits of this PR. Used in:

 - Condition Trees
 - Querying the PackageIndex

-----

Not sure about which type should have the (not)ThisPackageVersion function
Also need to update some comments. Everything builds, however.
@Ericson2314
Update tests
aa5a4fa 
In a number of cases the build process is now interleved differently,
but this seems harmless. It's probably just the new lib-dep identifiers
causing a different (valid) topological sort to be used.
@Ericson2314
Allow more erroneous warnings for now.
3c2e70e 
Eventually, configuring will be rewritten so extra constraints do note pollute
the checks. When that happens this commit should be reverted.
This was referenced Aug 13, 2018
Open
Merged
@phadej
Copy link
Copy Markdown
Collaborator

phadej commented Nov 28, 2019

Does @ezyang or @Ericson2314 plan to come back to this?

@phadej
Copy link
Copy Markdown
Collaborator

phadej commented Jul 20, 2020

I assume no.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

blocked: action-required

Projects

None yet

Milestone

Considered for 3.4

Development

Successfully merging this pull request may close these issues.

5 participants

@Ericson2314 @ezyang @23Skidoo @kmicklas @phadej