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

grayjay · 2019-05-13T02:36:47Z

This commit tracks dependencies on sub-libraries by extending the functionality
for tracking executables that was added in
e86f838. It also starts adding support for
library visibility, though it currently only works for source packages. There
is a TODO for handling installed packages.

Fixes #6038.

/cc @fgaz @kosmikus

Please include the following checklist in your PR:

Patches conform to the coding conventions.
Any changes that could be relevant to users have been recorded in the changelog.
The documentation has been updated, if necessary.
If the change is docs-only, [ci skip] is used to avoid triggering the build bots.

Please also shortly describe how you tested your change. Bonus points for added tests!

grayjay · 2019-05-14T05:10:31Z

I just realized that this PR probably doesn't handle visibility correctly. It only reads the top-level visibility field, but the field can also appear in conditionals.

Handling conditional visibility would probably require something similar to the "component on/off" flags in #4087 (comment), though that would be a significant change.

Another option would be to handle visibility similarly to the way buildable is handled after #5337. At the start of dependency solving, we could check whether a component is private with the current environment and flag constraints, which would allow the solver to reject most dependencies on private libraries. A visibility field controlled by an automatic flag could still cause a build failure at the end of solving.

phadej · 2019-05-14T08:02:49Z

Is there a use case for visibility controlled by an automatic flag? If there aren’t, then cabal check could reject such definitions?

…

On 14 May 2019, at 8.10, Kristen Kozak ***@***.***> wrote: I just realized that this PR probably doesn't handle visibility correctly. It only reads the top-level visibility field, but the field can also appear in conditionals. Handling conditional visibility would probably require something similar to the "component on/off" flags in #4087 (comment), though that would be a significant change. Another option would be to handle visibility similarly to the way buildable is handled after #5337. At the start of dependency solving, we could check whether a component is private with the current environment and flag constraints, which would allow the solver to reject most dependencies on private libraries. A visibility field controlled by an automatic flag could still cause a build failure at the end of solving. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

23Skidoo · 2019-05-14T12:13:03Z

If there aren’t, then cabal check could reject such definitions?

I think it'd be totally fine to do this, we can always relax this restriction later.

grayjay · 2019-05-16T08:26:20Z

I like the idea of disallowing visibility controlled by automatic flags, if it would be easy to reverse later. I could update this PR to try to determine whether libraries are visible at the start of solving, as I described above, which should always be correct when automatic flags aren't allowed.

23Skidoo · 2019-05-16T11:59:07Z

@grayjay SGTM.

grayjay · 2019-11-21T09:53:43Z

I finally updated this PR to determine whether each library is visible in the current environment at the start of solving. The solver checks the visibility of the main library and sub-libraries, though I'm not sure if it is possible for the main library to be private.

cc @fgaz

grayjay · 2019-12-09T09:35:03Z

I'm planning to merge this next weekend if no one objects.

phadej · 2019-12-09T09:41:50Z

I was going to (try to) stabilize master for 3.2 release this week. How high is the risk that this introduces regressions?

phadej · 2019-12-09T09:54:34Z

Yet, this doesn't change anything in Cabal the library, so as far as CI is happy, I'm happy too.

grayjay · 2019-12-09T09:56:38Z

Yes, it only affects the solver. I think there is a small risk of a regression in cabal-install, especially since this PR only contains unit tests, and, as far I know, there isn't a positive integration test for depending on sublibraries yet.

phadej · 2019-12-09T09:58:43Z

Wouldn't a positive test be a copy of cabal-testsuite/PackageTests/MultipleLibraries with a visibility toggled?

grayjay · 2019-12-09T10:01:41Z

I'm not sure. It looks like there some complexity with the GHC support for sublibraries in the discussion in #5848.

phadej · 2019-12-09T10:06:03Z

We can skip that test for GHC <8.8, I'll try this today as well.

EDIT: #6409

phadej · 2019-12-10T22:11:32Z

I'm milestoning this for 3.2. Let's get this in. If I introduce conflicts I can do the merging.

fgaz · 2019-12-10T23:09:03Z

Sorry, I've been busy lately. I'll take a look before the weekend

grayjay · 2019-12-11T06:23:20Z

@phadej @fgaz Thanks for looking into the integration testing. I'm fine with merging this before or after the release.

fgaz

Looks good! Thank you!

fgaz · 2019-12-15T11:20:07Z

cabal-install/Distribution/Solver/Modular/IndexConversion.hs

 where
+  -- TODO: Handle sub-libraries and visibility.


What does this entail?

I forgot to look into handling installed packages. It could be tricky because of the hack where the solver treats installed internal libraries as packages with munged names:

cabal/cabal-install/Distribution/Solver/Modular/IndexConversion.hs

Lines 106 to 131 in 8f42d3f

-- Note [Index conversion with internal libraries]

-- ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

-- Something very interesting happens when we have internal libraries

-- in our index. In this case, we maybe have p-0.1, which itself

-- depends on the internal library p-internal ALSO from p-0.1.

-- Here's the danger:

--

-- - If we treat both of these packages as having PN "p",

-- then the solver will try to pick one or the other,

-- but never both.

--

-- - If we drop the internal packages, now p-0.1 has a

-- dangling dependency on an "installed" package we know

-- nothing about. Oops.

--

-- An expedient hack is to put p-internal into cabal-install's

-- index as a MUNGED package name, so that it doesn't conflict

-- with anyone else (except other instances of itself). But

-- yet, we ought NOT to say that PNs in the solver are munged

-- package names, because they're not; for source packages,

-- we really will never see munged package names.

--

-- The tension here is that the installed package index is actually

-- per library, but the solver is per package. We need to smooth

-- it over, and munging the package names is a pretty good way to

-- do it.

I would need to figure out how to distinguish between inter- and intra-package dependencies when enforcing visibility.

fgaz · 2019-12-15T11:23:10Z

cabal-install/tests/UnitTests/Distribution/Solver/Modular/Solver.hs

              db = [ Right $ exAv "A" 1 [ExAny "B"]
                   , Right $ exAvNoLibrary "B" 2 `withExe` exe
                   , Right $ exAv "B" 1 [] `withExe` exe ]
          in runTest $ mkTest db "choose version of build-depends dependency that has a library" ["A"] $
             solverSuccess [("A", 1), ("B", 1)]
        ]
+    , testGroup "sub-library dependencies" [


This is great :)

fgaz · 2019-12-15T11:30:18Z

Almost forgot: would it be possible to give a better error when using ghc<8.8 and depending on a public library?
edit: it probably should be done in another pr. let's merge this first

grayjay · 2019-12-17T08:09:57Z

@fgaz Thanks for the review!

Almost forgot: would it be possible to give a better error when using ghc<8.8 and depending on a public library?

How should the solver handle ghc<8.8? Should it disallow inter-package dependencies on sublibraries?

fgaz · 2019-12-17T12:54:01Z

There are two options:

disallow inter-package dependencies on sublibraries, since they will fail Cabal's checks
override Cabal's checks with the --allow-depending-on-private-libs flag (see Prevent dependency on private library #5848 (review) and later comments), so that cabal-install can enable the multilibs functionality even on older ghcs

I think it was agreed to go with the second
/cc @hvr @ezyang

grayjay · 2019-12-18T07:49:58Z

@fgaz Does the second option (override with --allow-depending-on-private-libs) mean that the solver doesn't need to give a different error for ghc<8.8?

fgaz · 2019-12-18T08:22:42Z

Yes

phadej · 2020-05-14T14:57:13Z

@grayjay I noticed you worked on this recently. Do you think we can get this merged in the beginning of June 2020?

I optimistally milestone this for 3.4. But if the above deadline is too tight, feel free to de-milestone.

This commit tracks dependencies on sub-libraries by extending the functionality for tracking executables that was added in e86f838. It also starts adding support for library visibility, though it currently only works for source packages. There is a TODO for handling installed packages. This commit handles visibility similarly to the way that the buildable field is handled currently. It only checks whether a component is made private by the current environment and flag constraints at the start of dependency solving. This means that the solver can treat a component as visible when the visibility is controlled by an automatic flag, and the build can fail later, depending on the value that is chosen for the flag. Fixes haskell#6038.

This commit also refactors the Dependencies type so that it can represent any combination of dependencies, buildability, and visibility.

grayjay · 2020-05-17T08:35:47Z

@phadej I was only rebasing the branch. I think that the only thing blocking this PR was the integration test, but I see that you added tests in #6409. Thanks for adding them! This PR still has a TODO for handling installed packages, but I would rather do that in a separate PR.

grayjay · 2020-05-26T01:50:26Z

This was merged in #6836.

grayjay mentioned this pull request May 13, 2019

Make the cabal-install solver multilibs-aware #6039

Open

grayjay added the cabal-install: solver label May 13, 2019

grayjay force-pushed the issue-6038 branch from 1efd4d3 to 2695c0f Compare May 13, 2019 03:39

grayjay force-pushed the issue-6038 branch 2 times, most recently from e0c83bb to d22f35a Compare November 21, 2019 09:42

grayjay force-pushed the issue-6038 branch from d22f35a to a9f4ad8 Compare November 22, 2019 08:31

phadej added this to the 3.2 milestone Dec 10, 2019

fgaz approved these changes Dec 15, 2019

View reviewed changes

grayjay force-pushed the issue-6038 branch from a9f4ad8 to 5cb8cdb Compare December 17, 2019 07:45

grayjay force-pushed the issue-6038 branch 3 times, most recently from 836fd15 to 8146c52 Compare January 25, 2020 23:14

grayjay force-pushed the issue-6038 branch from 8146c52 to 559453c Compare February 1, 2020 19:20

grayjay force-pushed the issue-6038 branch from 559453c to 9b3257d Compare February 16, 2020 01:21

grayjay force-pushed the issue-6038 branch from 9b3257d to 242abec Compare March 1, 2020 07:59

grayjay force-pushed the issue-6038 branch from 242abec to 78f446e Compare March 23, 2020 03:19

grayjay force-pushed the issue-6038 branch from 78f446e to bcb5d65 Compare April 6, 2020 07:00

grayjay force-pushed the issue-6038 branch from bcb5d65 to 4b47409 Compare April 18, 2020 05:45

fgaz mentioned this pull request May 6, 2020

Post-multilibs checklist #5660

Open

31 tasks

grayjay force-pushed the issue-6038 branch from 4b47409 to abfc451 Compare May 9, 2020 22:49

phadej modified the milestones: 3.2.0.0, 3.4.0.0 May 14, 2020

fgaz mentioned this pull request May 15, 2020

Resolve #6409: Add public multilib test(s) #6802

Merged

grayjay added 5 commits May 17, 2020 00:40

Update comment about extractCondition.

3838247

Solver DSL: Support sub-libraries and library visibility field.

7ef6a8f

This commit also refactors the Dependencies type so that it can represent any combination of dependencies, buildability, and visibility.

Add solver unit tests for sub-library dependencies (issue haskell#6039).

91f8b30

Update test output with solver error for private dependencies.

60324d6

grayjay force-pushed the issue-6038 branch from abfc451 to 60324d6 Compare May 17, 2020 08:32

phadej mentioned this pull request May 18, 2020

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

Closed

fgaz mentioned this pull request May 22, 2020

Solver: Support dependencies on sub-libraries (issue #6039) (3rd iteration) #6836

Merged

grayjay closed this May 26, 2020

grayjay deleted the issue-6038 branch May 26, 2020 01:50

phadej mentioned this pull request Jul 10, 2020

Update changelog for 3.4 #6950

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

grayjay commented May 13, 2019 •

edited

Loading

grayjay commented May 14, 2019

phadej commented May 14, 2019 via email

23Skidoo commented May 14, 2019

grayjay commented May 16, 2019

23Skidoo commented May 16, 2019

grayjay commented Nov 21, 2019

grayjay commented Dec 9, 2019

phadej commented Dec 9, 2019 •

edited

Loading

phadej commented Dec 9, 2019

grayjay commented Dec 9, 2019

phadej commented Dec 9, 2019

grayjay commented Dec 9, 2019

phadej commented Dec 9, 2019 •

edited

Loading

phadej commented Dec 10, 2019

fgaz commented Dec 10, 2019

grayjay commented Dec 11, 2019

fgaz left a comment

fgaz Dec 15, 2019

grayjay Dec 17, 2019

fgaz Dec 15, 2019

fgaz commented Dec 15, 2019 •

edited

Loading

grayjay commented Dec 17, 2019

fgaz commented Dec 17, 2019 •

edited

Loading

grayjay commented Dec 18, 2019

fgaz commented Dec 18, 2019

phadej commented May 14, 2020

grayjay commented May 17, 2020

grayjay commented May 26, 2020

	-- Note [Index conversion with internal libraries]
	-- ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
	-- Something very interesting happens when we have internal libraries
	-- in our index. In this case, we maybe have p-0.1, which itself
	-- depends on the internal library p-internal ALSO from p-0.1.
	-- Here's the danger:
	--
	-- - If we treat both of these packages as having PN "p",
	-- then the solver will try to pick one or the other,
	-- but never both.
	--
	-- - If we drop the internal packages, now p-0.1 has a
	-- dangling dependency on an "installed" package we know
	-- nothing about. Oops.
	--
	-- An expedient hack is to put p-internal into cabal-install's
	-- index as a MUNGED package name, so that it doesn't conflict
	-- with anyone else (except other instances of itself). But
	-- yet, we ought NOT to say that PNs in the solver are munged
	-- package names, because they're not; for source packages,
	-- we really will never see munged package names.
	--
	-- The tension here is that the installed package index is actually
	-- per library, but the solver is per package. We need to smooth
	-- it over, and munging the package names is a pretty good way to
	-- do it.

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

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

Conversation

grayjay commented May 13, 2019 • edited Loading

grayjay commented May 14, 2019

phadej commented May 14, 2019 via email

23Skidoo commented May 14, 2019

grayjay commented May 16, 2019

23Skidoo commented May 16, 2019

grayjay commented Nov 21, 2019

grayjay commented Dec 9, 2019

phadej commented Dec 9, 2019 • edited Loading

phadej commented Dec 9, 2019

grayjay commented Dec 9, 2019

phadej commented Dec 9, 2019

grayjay commented Dec 9, 2019

phadej commented Dec 9, 2019 • edited Loading

phadej commented Dec 10, 2019

fgaz commented Dec 10, 2019

grayjay commented Dec 11, 2019

fgaz left a comment

Choose a reason for hiding this comment

fgaz Dec 15, 2019

Choose a reason for hiding this comment

grayjay Dec 17, 2019

Choose a reason for hiding this comment

fgaz Dec 15, 2019

Choose a reason for hiding this comment

fgaz commented Dec 15, 2019 • edited Loading

grayjay commented Dec 17, 2019

fgaz commented Dec 17, 2019 • edited Loading

grayjay commented Dec 18, 2019

fgaz commented Dec 18, 2019

phadej commented May 14, 2020

grayjay commented May 17, 2020

grayjay commented May 26, 2020

grayjay commented May 13, 2019 •

edited

Loading

phadej commented Dec 9, 2019 •

edited

Loading

phadej commented Dec 9, 2019 •

edited

Loading

fgaz commented Dec 15, 2019 •

edited

Loading

fgaz commented Dec 17, 2019 •

edited

Loading