Extend the InstalledPackageInfo record with 2 fields for artifacts. #8696

bairyn · 2023-01-24T03:44:04Z

(Split from PR #8624; there is more discussion there.)

This PR adds 2 new IPI fields that would allow the modular resolver to detect installed package options that would be missing required artifacts, to avoid build plans that would cause missing file errors.

This is the Cabal part of PR #8624 outside cabal-install, and it adds 2 new fields to the IPI, updating the syntax.

These 2 fields involve configuration flags that affect which build artifacts (dynamic and static files) are provided. (This record corresponds to the ‘.conf’ files in the package-db directories, and Cabal-syntax provides an interface to this record. Cabal-syntax is used as a dependency by ‘ghc-pkg’ and ‘cabal-install’, and old Cabal-syntax implementations may produce an ‘Unknown field’ warning when used with new ‘.conf’ files.) Add these IPI build artifact fields to enable tracking build artifacts in installed packages. The modular resolver could then use these new fields to filter out installed package options missing required build artifacts that would lead to a failing build plan.

bairyn · 2023-01-24T04:06:40Z

It looks like I mispredicted the next PR number when I opened this PR, so I just changed it in the changelog.d from 8692 to 8696.

MR updates:

Fixed PR # in the changelog.

Mikolaj

Thank you very much. We've just cut 3.10 yesterday, but if this is reviewed quickly and if GHC has not included 3.10 into GHC codebase yet, perhaps we could squeeze this in.

Mikolaj · 2023-01-24T19:36:33Z

Ben seems to be busy, so I'm annoying other maintainers with the express review request. :)

bgamari

This needs more commentary. What problem is being solved? What is the design of the solution? How does that design address the problem?

bgamari · 2023-01-24T22:08:09Z

Cabal-syntax/src/Distribution/Types/InstalledPackageInfo.hs

-        pkgRoot           :: Maybe FilePath
+        pkgRoot           :: Maybe FilePath,
+        -- Artifacts included in this package:
+        providesStaticArtifacts  :: Bool,


These need more documentation. What do we mean by "artifacts" here? How do these relate to GHC's notion of ways? Why are "static" and "dynamic" special (e.g. what about profiling)?

Ben: this is discussed in the prior two PRs -- #8624 and #8461

Initially it had added five fields, not just two. However, the other part of the split PR only now makes use of these two, so the scope was scaled back for simplicity. (Unfortunately, this isn't in the history of the linked PR due to it being force-pushed). I think extending the logic to profiling (and teaching the solver about that) would also be useful, but its out of scope for the current round of work, so having those fields added without then being used opened the way to some bikeshedding and confusion which this hopefully avoids.

@ben: do you mean documentation in the code included in GHC so that GHC devs don't need to rummage in cabal PRs? That makes sense. Or at least ready links to the aformentioned PRs from the code comments. Or both.

Based on common practice in GHC, I suspect the answer is "document it right there in the code." I think it's a best practice in any case. :)

(Wrong @ben fyi)

Oh. I'm sorry, @bgamari.

And I'm sorry @ben. :)

@chreekat hit the nail on the head. Yes, the documentation should be next to the implementation otherwise future us has little chance of finding it.

Fair enough. I should have been more specific when asking @bairyn for the artificial commit, but I lacked foresight. Let's wait with resuming the 3.10 semi-freeze and publishing a new dogfooding tag till today's devs meeting. I know @bairyn is busy, so that probably means the PRs have to wait until GHC 9.8. Tough luck.

ulysses4ever · 2023-01-25T00:57:37Z

Would it be possible to have a test for this?

gbaz · 2023-01-25T04:40:26Z

#8624 has some good tests (that pass) in it. Again, this is just a fragment that was split for technical reasons (so we can get the changes necessary for the Cabal library out in time for ghc and then merge into cabal-install the bigger changes without the pressure of an immediate branch)

Mikolaj · 2023-01-25T06:11:54Z

@ulysses4ever: And it has a minimal test (results change) of the (non-)introduced (non-)functionality, namely a parsing test results adjustment.

ulysses4ever · 2023-01-25T07:44:45Z

@Mikolaj I read the PR because I wanted to approve it. But then I decided it's above my head a bit. What could have helped me is the things that Ben mentioned (documentation and motivation), and a proper test demonstrating the new functionally. What didn't helped: the changes in the test suite that have been made.

@gbaz If there are tests on the other PR, why not move them here? I will understand if the tests require changes from other PR. But then the current PR is hardly an independent PR imo. That's okay, I just wanted a test so that I understand it (better). No big deal.

Mikolaj · 2023-01-25T08:53:19Z

But then the current PR is hardly an independent PR imo.

Yes, you got it right. This PR is completely out of whack and I requested it only due the exceptional circumstances: the difficulty of reviewing and improving the main PR and the need to cut 3.10 without blocking the main PR until GHC incorporates Cabal next time, which should normally be in GHC 9.8.

grayjay · 2023-01-26T06:37:28Z

Cabal/src/Distribution/Simple/Register.hs

+          statics = libDefaults || any ($ lbi) [withVanillaLib]
+          dynamics = any ($ lbi) [withSharedLib]


These uses of any can be simplified now that the lists have length one.

Mikolaj · 2023-01-26T19:08:21Z

@bairyn: so it seems we've made time for a non-hurried review and design discussion process for this PR. It's changing the GHC/cabal interface and, unfortunately, cabal and GHC have to be backward compatible with all new and old versions of this interface for ages, so we need to discuss and turn it upside down until it sticks. But there is a significant chance for it to land already in cabal 3.10 and GHC 9.6, if we do this right.

bgamari · 2023-01-26T21:39:17Z

@bairyn, I hope you will forgive my rather unspecific request above and thank your patience in pushing this forward. It was based in some skepticism about the approach which I found hard to articulate at the time. However, after further reflection, I think the problem is that I'm simply skeptical of giving static and dynamic special treatment in Cabal. Afterall, they are simply two GHC "ways" of many.

I think a more uniform solution would both reduce the amount of special-casing necessary in Cabal and allow us to solve the problem in its full generality. Specifically, instead of adding two boolean fields, why not add a single field encoding the set of ways in which the package was built. For instance:

data Way =
    Way { wayDynamic :: Bool  -- ^ static or dynamic?
        , wayProfiled :: Bool -- ^ profiling enabled?
        }

data InstalledPackageInfo =
    InstalledPackageInfo
        { ...
        , pkgBuiltWays :: [Way]
        }

This allows us to solve the static/dynamic problem you set out to address, as well as the same problem as it manifests with profiled objects. Moreover, it's easily extensible if/when new ways are introduced.

Does this sound like a sensible way forward?

bairyn · 2023-11-15T23:04:58Z

@bgamari: Thanks for the feedback, and while extending with the special case of 2 ‘ways’ or mutually compatible building modes in particular I think would work, I think I like your approach better. (When I first made the MR, I was unfamiliar with GHC ways.)

I'll probably work on a new PR to replace this one, that takes that approach instead. (And then for the PR for Track Static vs Dynamic dependencies, which depends on this, I can add a new PR that checks IPI Way compatibility, to see if it needs to rule out pre-installed IPI options that are incompatible and would lead to build errors, so that it can pick, if available, source packages instead.)

bairyn mentioned this pull request Jan 24, 2023

Track static vs. dynamic dependencies #8624

Open

bairyn force-pushed the fix-dynamic-deps-2-arts-ipi branch from 80bc4ca to 111a46a Compare January 24, 2023 04:04

Mikolaj requested a review from bgamari January 24, 2023 08:17

Mikolaj approved these changes Jan 24, 2023

View reviewed changes

Mikolaj requested review from grayjay and gbaz January 24, 2023 19:35

gbaz approved these changes Jan 24, 2023

View reviewed changes

bgamari requested changes Jan 24, 2023

View reviewed changes

Mikolaj removed the request for review from grayjay January 25, 2023 09:51

grayjay reviewed Jan 26, 2023

View reviewed changes

andreabedini mentioned this pull request May 23, 2023

InstalledPackageInfo mega issue #8967

Open

ulysses4ever mentioned this pull request Jun 16, 2023

Store hie files #9019

Merged

4 tasks

andreabedini added the re: installed-package-info label Aug 31, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend the InstalledPackageInfo record with 2 fields for artifacts. #8696

Extend the InstalledPackageInfo record with 2 fields for artifacts. #8696

bairyn commented Jan 24, 2023 •

edited by gbaz

Loading

bairyn commented Jan 24, 2023

Mikolaj left a comment

Mikolaj commented Jan 24, 2023

bgamari left a comment

bgamari Jan 24, 2023

gbaz Jan 24, 2023 •

edited

Loading

Mikolaj Jan 25, 2023

chreekat Jan 25, 2023

Mikolaj Jan 25, 2023

Mikolaj Jan 25, 2023

bgamari Jan 26, 2023

Mikolaj Jan 26, 2023 •

edited

Loading

ulysses4ever commented Jan 25, 2023

gbaz commented Jan 25, 2023

Mikolaj commented Jan 25, 2023

ulysses4ever commented Jan 25, 2023

Mikolaj commented Jan 25, 2023

grayjay Jan 26, 2023

Mikolaj commented Jan 26, 2023

bgamari commented Jan 26, 2023 •

edited

Loading

bairyn commented Nov 15, 2023

		statics = libDefaults \|\| any ($ lbi) [withVanillaLib]
		dynamics = any ($ lbi) [withSharedLib]

Extend the InstalledPackageInfo record with 2 fields for artifacts. #8696

Are you sure you want to change the base?

Extend the InstalledPackageInfo record with 2 fields for artifacts. #8696

Conversation

bairyn commented Jan 24, 2023 • edited by gbaz Loading

bairyn commented Jan 24, 2023

Mikolaj left a comment

Choose a reason for hiding this comment

Mikolaj commented Jan 24, 2023

bgamari left a comment

Choose a reason for hiding this comment

bgamari Jan 24, 2023

Choose a reason for hiding this comment

gbaz Jan 24, 2023 • edited Loading

Choose a reason for hiding this comment

Mikolaj Jan 25, 2023

Choose a reason for hiding this comment

chreekat Jan 25, 2023

Choose a reason for hiding this comment

Mikolaj Jan 25, 2023

Choose a reason for hiding this comment

Mikolaj Jan 25, 2023

Choose a reason for hiding this comment

bgamari Jan 26, 2023

Choose a reason for hiding this comment

Mikolaj Jan 26, 2023 • edited Loading

Choose a reason for hiding this comment

ulysses4ever commented Jan 25, 2023

gbaz commented Jan 25, 2023

Mikolaj commented Jan 25, 2023

ulysses4ever commented Jan 25, 2023

Mikolaj commented Jan 25, 2023

grayjay Jan 26, 2023

Choose a reason for hiding this comment

Mikolaj commented Jan 26, 2023

bgamari commented Jan 26, 2023 • edited Loading

bairyn commented Nov 15, 2023

bairyn commented Jan 24, 2023 •

edited by gbaz

Loading

gbaz Jan 24, 2023 •

edited

Loading

Mikolaj Jan 26, 2023 •

edited

Loading

bgamari commented Jan 26, 2023 •

edited

Loading