[GSoC2018] Add support for multiple public libraries in a single package by fgaz · Pull Request #5526 · haskell/cabal

fgaz · 2018-08-14T15:00:31Z

This is my work for Google Summer of Code 2018.

It adds support for multiple public libraries, as described in #4206.

See https://github.com/fgaz/gsoc/blob/master/2018/final-report.md for a more detailed report. I'll expand a bit this description shortly.

~~There are still a few nonessential things to do and some things to clean, so don't review yet! (see the link above or the checklist below). It should all work though.~~ Go ahead!

Closes #4206

/cc @ezyang @23Skidoo

Things to fix/clean/update before merging:

After merging (better to merge soon and avoid to let it rot) (moved to #5660)

Add a visibility field to sublibraries
Maybe: remove all the bits of code that handle the old syntax (there's lots of them) and do it all in a single spot (hint: grep for packageNameToUnqualComponentName)
Add tests
Update changelog
Update docs
Maybe more source comments
Deprecate old syntax
Update cabal init to take into account sublibraries when populating build-depends

Ericson2314 · 2018-08-14T15:44:16Z

Oh man this overlaps a lot with my old #4265 . I wish I could have gotten you that earlier.

phadej · 2018-08-14T19:24:51Z

    pretty (CBenchName str)  = Disp.text "bench:" <<>> pretty str

 instance Text ComponentName where
-    parse = parseComposite <++ parseSingle


This is used in command line parsers, doesn't it? Can you add a comment, as it's surprising to see Text instance in use. (and without corresponding Parsec instance)

phadej · 2018-08-14T19:27:39Z

             Parse.skipSpaces
             ver <- parse Parse.<++ return anyVersion
             Parse.skipSpaces
-             return (Dependency name ver)


... but editing this Text instance shouldn't affect anything.

Why not? Isn't it used for display? display appears in quite a few places

phadej · 2018-08-14T19:27:53Z

    parsec = do
        name <- lexemeParsec
        ver  <- parsec <|> pure anyVersion
-        return (Dependency name ver)


Note: we need to case on askCabalSpecVersion.

phadej · 2018-08-15T14:03:31Z

Also a dumb question, are all internal sublibraries public with this change? EDIT i.e. can be dependent upon.

fgaz · 2018-08-15T14:22:42Z

@phadej Yup. But I'll add a visibility field shortly. (I just added a todo list to the description)

fgaz · 2018-08-15T14:28:59Z

@Ericson2314 Yeah, I discovered that pr halfway through GSoC (and I actually tried to rebase it), but at that point my work diverged too much from that :-/ (I use Set LibraryName instead of a single library per dep).

I did not manage to separate the dependency-as-constraint from the dependency-dependency as you did, but I really think it should be done: https://github.com/fgaz/gsoc/blob/master/2018/final-report.md#the-dependency-as-constraint-problem

Ericson2314 · 2018-08-15T20:12:11Z

Glad you found it and it was of some use :)

phadej · 2018-08-16T08:07:12Z

@fgaz is there any part which can be separated into own PR and merged already (even it's little or even no use by itself alone)? ComponentName refactor looks like that at a first glance.

fgaz · 2018-08-16T08:53:50Z

@phadej yup, that and the GivenComponent one probably.

Also note that the LibraryName refactor could be much more complete, but I ran into some problems (I probably just forgot something. I'll have to recheck why the tests broke)

fgaz · 2018-08-16T17:31:03Z

Is it ok if I add the visibility field in another pr after the merge, so we merge soon and avoid conflicts?

23Skidoo · 2018-08-16T18:50:16Z

@fgaz Yes, sure.

phadej · 2018-09-05T20:14:08Z

 instance Parsec Dependency where
    parsec = do
        name <- lexemeParsec
+        libs <- option [LMainLibName]


Note: this should be guarded by askCabalSpecVersion

phadej · 2018-09-05T20:23:53Z

23:21 phadej what if there is pkgA:lib; then pkgB depends on pkgA:lib but looks like "ordinary" package outside. These both are required to have recent cabal-version: 3.0 or like that
23:21 phadej but then there's pkgC which depends on pkgB, and has cabal-version: 1.12 and build-type: Custom
23:21 phadej will its Setup.hs be passed --dependency=pkg:component=id ?

or is only direct dependencies are passed. Anyway, build-type: Custom have to we thought out properly, as there we have to speak with old Cabal code.

fgaz · 2018-09-07T13:10:45Z

@phadej Just tested it and only direct deps are passed

phadej · 2018-09-07T13:14:46Z

@fgaz that's good news 👍

…nfigure` In the course of 4466310 (see haskell#5526) the `Setup.hs haddock` CLI was extended to allow component ids to be passed as positional arguments. However, `autoconfUserHooks` which is used in case of `build-type: Configure` wasn't updated accordingly, and consequently this caused `new-haddock` to break for packages using `Configure`. Fixes haskell#5503

@alexbiehl

…nfigure` In the course of 4466310 (see haskell#5526) the `Setup.hs haddock` CLI was extended to allow component ids to be passed as positional arguments. However, `autoconfUserHooks` which is used in case of `build-type: Configure` wasn't updated accordingly, and consequently this caused `new-haddock` to break for packages using `Configure`. cc @alexbiehl Fixes haskell#5503

@alexbiehl

…nfigure` In the course of 4466310 (see #5526) the `Setup.hs haddock` CLI was extended to allow component ids to be passed as positional arguments. However, `autoconfUserHooks` which is used in case of `build-type: Configure` wasn't updated accordingly, and consequently this caused `new-haddock` to break for packages using `Configure`. cc @alexbiehl Fixes #5503 (cherry picked from commit 86dabda)

ezyang · 2018-09-15T20:13:41Z

@fgaz Do you want us to review now? :)

fgaz · 2018-09-16T07:34:49Z

@ezyang Sorry, not yet. Just wait a literal couple of days

"url":"pull/5526", "account":"haskell", "repo":"cabal", "commit": "63f4c37a1b0a267029e5b9e12d58fb0759a22e11", "tag":"linux-7.6.3" }

"url":"pull/5526", "account":"haskell", "repo":"cabal", "commit": "63f4c37a1b0a267029e5b9e12d58fb0759a22e11", "tag":"linux-7.8.4" }

"url":"pull/5526", "account":"haskell", "repo":"cabal", "commit": "63f4c37a1b0a267029e5b9e12d58fb0759a22e11", "tag":"osx-7.8.4" }

"url":"pull/5526", "account":"haskell", "repo":"cabal", "commit": "887eb3352fc6f923e46bc3cb24c0de89292165bb", "tag":"linux-7.10.3" }

"url":"pull/5526", "account":"haskell", "repo":"cabal", "commit": "887eb3352fc6f923e46bc3cb24c0de89292165bb", "tag":"linux-8.0.2" }

fgaz · 2018-10-05T14:49:31Z

@ezyang

Thanks for the review! Good catches!

I'll remove those todos and that undefined and merge then

If default visibility is public there are no objections here.

I think you mean private?

Sublibraries do become public by default with this pr, but only because there's no visibility flag yet, which will default to private

ezyang · 2018-10-05T14:54:06Z

Sublibraries do become public by default with this pr, but only because there's no visibility flag yet, which will default to private

I'm primarily pointing out that changing the default from public to private is a BC-breaking change. If you do it quickly enough no one will notice, but if you don't...

fgaz · 2018-10-05T14:56:42Z

                           brought into scope by mixins: fail:postgresql (Database.PostgreSQL as Database)
-      While filling requirements of build-depends: fail:mylib
+      While filling requirements of     build-depends: fail:mylib
+                                    and build-depends: fail:mylib


@ezyang

Any pointers on fixing the duplicate output? I think it may be some only-main-lib code I didn't delete when adding multilib handling. It does not affect functionality

No, I'd have to investigate.

fgaz · 2018-10-17T07:31:20Z

I'm going to merge this later today unless someone objects.

@ezyang I'll make sure to add the visibility field before the 3.0 release

23Skidoo · 2018-10-17T08:16:10Z

What about the duplicate output on some tests?

edit: ...and the "add a few comments in crucial places" bit?

fgaz · 2018-10-18T08:46:59Z

@23Skidoo I wasn't able to pinpoint the cause, but it seems not to affect functionality. Should I delay the merge until I fix it?

23Skidoo · 2018-11-01T19:19:05Z

Well, it's just that you have that in your "to be fixed before merge" column.

fgaz · 2018-11-02T18:41:02Z

@23Skidoo please don't merge yet. I think I found the issue. I'll merge it myself when it's fixed

The old --dependency could only do --dependency=pkg=cid, but with public sublibraries this will become insufficient. Now there is the option to also specify a component name using --dependency=pkg:component=cid

Before, a 3-tuple was used

This gives us more type safety when dealing with libraries only

Create a new syntax for depending on any library of any package. The syntax is build-depends: pkgname:{pkgname, sublibname} -any where the second `pkgname` specifies a dependency on the main unnamed library. Closes haskell#4206.

Dependency is used in two seemingly related but now incompatible ways: * As a dependency specification ("I want this component from this package, version range xy") * As a constraint ("Package p must be in version range xy") This commit begins to separate the two concepts.

We generated them both the old way (only internal libs) and the new way (arbitrary sublibraries), leading to this: $ cabal new-configure my-package -v [...] Component graph for my-package-0.2.0.0: component exe:my-exe component my-package-0.2.0.0-inplace-my-exe include base-4.11.1.0 include base-4.11.1.0 include word8-0.1.3-968d6922e82a4ae480d10bc205e047a27af7804ae8fb577c4b6eeb868f99ee3b include word8-0.1.3-968d6922e82a4ae480d10bc205e047a27af7804ae8fb577c4b6eeb868f99ee3b [...] ...which didn't have any effect on the build except for some duplicate output, but could lead to problems in the future. Now we only generate them the new way.

This reverts commit 1c4ad06. The previous commit fixed it

fgaz · 2018-11-03T15:50:18Z

It's ready, but appveyor continues to fail with this error:

ghc-pkg.exe: cannot create: C:\Users\appveyor\AppData\Roaming\cabal\store\ghc-8.0.2\package.db already exists

Is that normal/something that happens often?

edit: I don't have a windows machine on which to test it.
edit2: fixed

Ericson2314 · 2018-11-06T00:31:56Z

Congrats!!!

fgaz added this to the 3.0 milestone Aug 14, 2018

fgaz mentioned this pull request Aug 14, 2018

Make --dependency accept a component too [WIP] #5405

Closed

4 tasks

phadej reviewed Aug 14, 2018

View reviewed changes

fgaz mentioned this pull request Aug 16, 2018

[GSoC2018] [FREEZED on 2018-08-14] Add support for multiple public libraries in a single package #5528

Closed

fgaz force-pushed the multiple-libraries-v4 branch from 56a3c0d to b57838f Compare August 29, 2018 20:21

phadej reviewed Sep 5, 2018

View reviewed changes

hvr mentioned this pull request Sep 8, 2018

Allow arguments to be passed to Setup.hs haddock for build-type:configure #5573

Merged

haskell-pushbot pushed a commit to haskell-pushbot/cabal-binaries that referenced this pull request Sep 17, 2018

{"origin":"haskell/cabal/pull/5526",

c380777

"url":"pull/5526", "account":"haskell", "repo":"cabal", "commit": "63f4c37a1b0a267029e5b9e12d58fb0759a22e11", "tag":"linux-7.6.3" }

haskell-pushbot pushed a commit to haskell-pushbot/cabal-binaries that referenced this pull request Sep 17, 2018

{"origin":"haskell/cabal/pull/5526",

a7b7666

"url":"pull/5526", "account":"haskell", "repo":"cabal", "commit": "63f4c37a1b0a267029e5b9e12d58fb0759a22e11", "tag":"linux-7.8.4" }

haskell-pushbot pushed a commit to haskell-pushbot/cabal-binaries that referenced this pull request Sep 17, 2018

{"origin":"haskell/cabal/pull/5526",

32a0596

"url":"pull/5526", "account":"haskell", "repo":"cabal", "commit": "63f4c37a1b0a267029e5b9e12d58fb0759a22e11", "tag":"osx-7.8.4" }

haskell-pushbot pushed a commit to haskell-pushbot/cabal-binaries that referenced this pull request Sep 18, 2018

{"origin":"haskell/cabal/pull/5526",

04843a2

"url":"pull/5526", "account":"haskell", "repo":"cabal", "commit": "887eb3352fc6f923e46bc3cb24c0de89292165bb", "tag":"linux-7.10.3" }

haskell-pushbot pushed a commit to haskell-pushbot/cabal-binaries that referenced this pull request Sep 18, 2018

{"origin":"haskell/cabal/pull/5526",

02a694e

"url":"pull/5526", "account":"haskell", "repo":"cabal", "commit": "887eb3352fc6f923e46bc3cb24c0de89292165bb", "tag":"linux-8.0.2" }

fgaz commented Oct 5, 2018

View reviewed changes

fgaz added 14 commits November 3, 2018 15:46

Use packageNameToUnqualComponentName

dcff38d

Make --dependency accept a component too

bf89715

The old --dependency could only do --dependency=pkg=cid, but with public sublibraries this will become insufficient. Now there is the option to also specify a component name using --dependency=pkg:component=cid

Create the GivenComponent datatype

e28348a

Before, a 3-tuple was used

Make use of the --dependency on component

c15a8d6

Move CLibName and CSubLibName to a separate type

898b284

This gives us more type safety when dealing with libraries only

Multiple libraries (allow depending on sublibs)

e2194d2

Create a new syntax for depending on any library of any package. The syntax is build-depends: pkgname:{pkgname, sublibname} -any where the second `pkgname` specifies a dependency on the main unnamed library. Closes haskell#4206.

Create spec version 3.0 and guard multilibs

4fac7e1

--accept tests for multiple libraries feature

2fcedb4

TEMPORARILY accept test with duplicate output

e623b19

Allow omitting {} on single library dependency

3b71195

Revert "TEMPORARILY accept test with duplicate output"

ed1c6b0

This reverts commit 1c4ad06. The previous commit fixed it

Add comment on multilibs syntax ambiguity

533b7cf

This was referenced Nov 4, 2018

Post-multilibs checklist #5660

Open

Multiple public libraries in a package #4206

Closed

philderbeast mentioned this pull request Nov 6, 2018

Am I limited to working on a single package? nmattia/snack#92

Open

brendanhay mentioned this pull request Oct 10, 2021

Consolidate all amazonka-* libraries into a single amazonka library brendanhay/amazonka#671

Open

mpilgrem mentioned this pull request Jun 11, 2022

Support sub-library dependencies commercialhaskell/stack#5659

Closed

2 tasks

alexfmpe mentioned this pull request Jul 20, 2022

[Proof of concept]: Computing closure of 'interpret' packages obsidiansystems/obelisk#933

Draft

6 tasks

Conversation

fgaz commented Aug 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ericson2314 commented Aug 14, 2018

Uh oh!

phadej Aug 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phadej Aug 14, 2018

Choose a reason for hiding this comment

Uh oh!

fgaz Aug 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phadej Aug 14, 2018

Choose a reason for hiding this comment

Uh oh!

phadej commented Aug 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fgaz commented Aug 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fgaz commented Aug 15, 2018

Uh oh!

Ericson2314 commented Aug 15, 2018

Uh oh!

phadej commented Aug 16, 2018

Uh oh!

fgaz commented Aug 16, 2018

Uh oh!

fgaz commented Aug 16, 2018

Uh oh!

23Skidoo commented Aug 16, 2018

Uh oh!

phadej Sep 5, 2018

Choose a reason for hiding this comment

Uh oh!

phadej commented Sep 5, 2018

Uh oh!

fgaz commented Sep 7, 2018

Uh oh!

phadej commented Sep 7, 2018

Uh oh!

ezyang commented Sep 15, 2018

Uh oh!

fgaz commented Sep 16, 2018

Uh oh!

fgaz commented Oct 5, 2018

Uh oh!

ezyang commented Oct 5, 2018

Uh oh!

fgaz Oct 5, 2018

Choose a reason for hiding this comment

Uh oh!

ezyang Oct 5, 2018

Choose a reason for hiding this comment

Uh oh!

fgaz commented Oct 17, 2018

Uh oh!

23Skidoo commented Oct 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fgaz commented Oct 18, 2018

Uh oh!

23Skidoo commented Nov 1, 2018

Uh oh!

fgaz commented Nov 2, 2018

Uh oh!

fgaz commented Nov 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ericson2314 commented Nov 6, 2018

Uh oh!

Reviewers

Assignees

Labels

fgaz commented Aug 14, 2018 •

edited

Loading

phadej Aug 14, 2018 •

edited

Loading

fgaz Aug 20, 2018 •

edited

Loading

phadej commented Aug 15, 2018 •

edited

Loading

fgaz commented Aug 15, 2018 •

edited

Loading

23Skidoo commented Oct 17, 2018 •

edited

Loading

fgaz commented Nov 3, 2018 •

edited

Loading