Prevent dependency on private library #5848

fgaz · 2019-01-16T16:24:24Z

This will make Cabal error out when depending on a private library.

It does not give detailed info on which sublibrary is the culprit because that would require changing the dependencySatisfiable and finalizePD return types, and that touches A LOT of the codebase. It can be done later.

/cc @ezyang @phadej

TODO:

on older ghcs treat all internal libraries as private and all main libraries as public, instead of "everything is private"
- even on older older ghcs (<8.2)

Closes #5806

fgaz · 2019-01-16T17:18:06Z

Oops, of course on ghc <8.8 all libraries are private 🤦‍♂️

fgaz · 2019-02-03T15:17:34Z

Hmm, this probably requires to add visibility to Basic, so we can turn it into LibraryVisibilityPublic if it's a LMainLibName

fgaz · 2019-02-04T17:05:48Z

This is now complete and reviewable

(Sorry for the mass ping)

phadej

It would be nice if we had a test that depending on a private lib fails as expected.

Adding a positive variant is possible only when GHC-8.8 is out, please make an issue about that.

hvr

I'm not so sure anymore we should merge this; I'm still discussing this with @fgaz

phadej · 2019-03-03T18:29:35Z

@hvr why

I'm not so sure anymore we should merge this;

?

fgaz · 2019-03-04T13:49:29Z

@phadej

Concerns about the narrow ghc compatibility. As it is, this patch only allows multilibs on GHC<8.8 and doesn't provide an override. If it did (for example when using --exact-config), then cabal-install could make multilibs work even on older GHCs by getting the visibility info from an IPI-external source.

Also this makes cabal-install fail after build plan construction, which is not pleasant, but the only way to solve that is to make the solver sublibrary-aware.

23Skidoo · 2019-05-06T14:33:04Z

@fgaz @hvr Do you still want this in 3.0?

phadej · 2019-05-06T16:08:07Z

This has priority: high.

But in fact, i'm leaning towards disabling parsing of visibility field (i.e. essentially removing the multiple public libraries feature from public usage), if it won't be finalized in some way.

This is because I simply don't understand all the implications of the choices, and want to error on the safe side.

23Skidoo · 2019-05-06T18:31:42Z

I'm fine with postponing it until 3.2.

fgaz · 2019-05-07T08:36:23Z

I think we can, and in fact should merge this, even if we end up disabling visibility for 3.0, simply because we can relax the constraint later, but the opposite is difficult. As it is, the rest of multilibs is already there, unconstrained.

Now, should we, in another pr, disable it? well...

When this pr is merged, the Cabal part will be complete
- The only missing piece I can think of is the error reporting I mentioned in the OP
cabal-install is not multilibs-aware, but IMO this is not so bad. Changing the visibility of a library
can be treated like changing the visibility of a module for now, like I suggest in this blog post draft
(see the warnings)
The BC problem cannot be solved at the Cabal level, but we can provide an override which can be used by future cabal-install releases
- This can just be exact-config
  - Easy to implement at the Cabal level, it's just an extra condition
    here
  - I think this requires at least a basic no-deps-on-private-libs check in cabal-install, since we always pass exact-config
- Or it can be an entirely new flag (--allow-depending-on-private-libs-yes-im-checking-it-externally)
  - Easy-ish to add to Cabal, there should just be a lot of boilerplate since we have to pass it down to dependencySatisfiable (like we do with exact-config)
  - Requires no modifications to cabal-install 3.0
  - Probably the safest option
- Or we can add the override in later Cabal versions. A minor version, even (if we use a new flag)

TL;DR: +1 for merging, -1 for disabling multilibs, I'm for adding a new flag to allow future cabal-installs to override visibility and expand BC

23Skidoo · 2019-05-07T11:20:04Z

OK, I'll try to review this PR this week. Sorry for being slow.

fgaz · 2019-05-07T17:05:58Z

np

Added a negative package test

hvr · 2019-05-07T17:30:21Z

but we can provide an override which can be used by future cabal-install releases

please do! :-)

fgaz · 2019-05-10T08:17:24Z

The test is failing for ghc <=8.0.*

Somehow for those versions sublibraries always appear as public. I'm investigating, but I don't see any clues in the ghc changelog

edit: 8.2 included Cabal 2.0 (the first version with internal libraries and backpack), so it may be that.
edit2: I could just disable public libs for ghc <8.2

ghc-pkg<8.8 cannot read the visibility field, so all libraries were reported as private, even the main (unnamed) one. This commit forces the visibility of main libraries to be "public". This commit can be reverted once we stop supporting GHC<8.8, at the condition that we keep marking main libraries as public when registering them.

Related to the previous two commits (preventing dependency on private libraries and marking main libraries as public)

fgaz · 2019-06-08T19:43:25Z

OK, this should fix the older ghcs. I also added the --allow-depending-on-private-libs flag which can be used by cabal-install later.

Ping @23Skidoo

Cabal/Distribution/Simple/Configure.hs

This is intended to be used by tools like cabal-install so they can add multilibs-compatibility even to older ghcs

fgaz · 2019-06-21T10:03:32Z

The travis failures are unrelated to this (I did not even modify ResponseFiles.hs).

@23Skidoo can we merge?

23Skidoo · 2019-06-25T02:41:03Z

Cabal/Distribution/Simple/Setup.hs

@@ -703,6 +707,13 @@ configureOptions showOrParseArgs =
         configUseResponseFiles
         (\v flags -> flags { configUseResponseFiles = v })
         (boolOpt' ([], ["disable-response-files"]) ([], []))
+
+      ,option "" ["allow-depending-on-private-libs"]


This should be added to filterConfigureFlags as well, though since we don't set it anywhere, it's not a blocker for merging.

23Skidoo · 2019-06-25T02:41:38Z

Merged, thanks!

fgaz added the Cabal: other label Jan 16, 2019

fgaz added this to the 3.0 milestone Jan 16, 2019

fgaz mentioned this pull request Jan 24, 2019

Release for GHC 8.8 #5819

Closed

fgaz added the priority: high 🔥 label Jan 24, 2019

fgaz mentioned this pull request Feb 2, 2019

Post-multilibs checklist #5660

Open

31 tasks

fgaz added the blocked: action-required label Feb 2, 2019

fgaz force-pushed the prevent-dep-on-private-lib branch from af482b4 to 6d5ab50 Compare February 3, 2019 14:38

fgaz force-pushed the prevent-dep-on-private-lib branch 2 times, most recently from 2d0bfac to 86daec5 Compare February 4, 2019 17:01

fgaz requested review from phadej, ezyang, 23Skidoo and hvr February 4, 2019 17:02

fgaz removed the blocked: action-required label Feb 4, 2019

phadej reviewed Feb 21, 2019

View reviewed changes

hvr reviewed Mar 3, 2019

View reviewed changes

Prevent dependency on private library

cf27290

fgaz force-pushed the prevent-dep-on-private-lib branch from 86daec5 to fcbe835 Compare May 7, 2019 16:58

grayjay mentioned this pull request May 10, 2019

Make the cabal-install solver multilibs-aware #6039

Open

fgaz and others added 3 commits June 8, 2019 21:39

--accept some tests

c7b927c

Related to the previous two commits (preventing dependency on private libraries and marking main libraries as public)

Add a negative test for multilibs

f14d4a2

fgaz force-pushed the prevent-dep-on-private-lib branch from f88a8ad to 88ecdd7 Compare June 8, 2019 19:39

fgaz commented Jun 8, 2019

View reviewed changes

Cabal/Distribution/Simple/Configure.hs Outdated Show resolved Hide resolved

fgaz mentioned this pull request Jun 10, 2019

RFC: library visibility enforcement #5806

Closed

Add a flag to allow depending on private libraries

3e31042

This is intended to be used by tools like cabal-install so they can add multilibs-compatibility even to older ghcs

fgaz force-pushed the prevent-dep-on-private-lib branch from 88ecdd7 to 3e31042 Compare June 17, 2019 15:18

23Skidoo reviewed Jun 25, 2019

View reviewed changes

23Skidoo merged commit 9756b4c into haskell:master Jun 25, 2019

grayjay mentioned this pull request Dec 9, 2019

Solver: Support dependencies on sub-libraries (issue #6039). #6047

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent dependency on private library #5848

Prevent dependency on private library #5848

fgaz commented Jan 16, 2019 •

edited

Loading

fgaz commented Jan 16, 2019 •

edited

Loading

fgaz commented Feb 3, 2019

fgaz commented Feb 4, 2019

phadej left a comment •

edited

Loading

hvr left a comment

phadej commented Mar 3, 2019

fgaz commented Mar 4, 2019

23Skidoo commented May 6, 2019

phadej commented May 6, 2019 •

edited

Loading

23Skidoo commented May 6, 2019

fgaz commented May 7, 2019 •

edited

Loading

23Skidoo commented May 7, 2019

fgaz commented May 7, 2019

hvr commented May 7, 2019

fgaz commented May 10, 2019 •

edited

Loading

fgaz commented Jun 8, 2019

fgaz commented Jun 21, 2019

23Skidoo Jun 25, 2019

23Skidoo commented Jun 25, 2019

Prevent dependency on private library #5848

Prevent dependency on private library #5848

Conversation

fgaz commented Jan 16, 2019 • edited Loading

fgaz commented Jan 16, 2019 • edited Loading

fgaz commented Feb 3, 2019

fgaz commented Feb 4, 2019

phadej left a comment • edited Loading

Choose a reason for hiding this comment

hvr left a comment

Choose a reason for hiding this comment

phadej commented Mar 3, 2019

fgaz commented Mar 4, 2019

23Skidoo commented May 6, 2019

phadej commented May 6, 2019 • edited Loading

23Skidoo commented May 6, 2019

fgaz commented May 7, 2019 • edited Loading

23Skidoo commented May 7, 2019

fgaz commented May 7, 2019

hvr commented May 7, 2019

fgaz commented May 10, 2019 • edited Loading

fgaz commented Jun 8, 2019

fgaz commented Jun 21, 2019

23Skidoo Jun 25, 2019

Choose a reason for hiding this comment

23Skidoo commented Jun 25, 2019

fgaz commented Jan 16, 2019 •

edited

Loading

fgaz commented Jan 16, 2019 •

edited

Loading

phadej left a comment •

edited

Loading

phadej commented May 6, 2019 •

edited

Loading

fgaz commented May 7, 2019 •

edited

Loading

fgaz commented May 10, 2019 •

edited

Loading