Track static vs. dynamic dependencies

[bairyn]

This was originally the second of two parts to the ‘Track static vs. dynamic dependencies.’ pull request (#8461), which I've cleaned up and split into two separate PRs.

As in the commit message:

Track build artifacts in installed packages.

    1) Extend the InstalledPackageInfo record with 5 records involving
    configuration flags that affect which build artifacts (dynamic and static
    files) are provided.  (This record corresponds to the ‘.conf’ files in
    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.)
    2) Add a step to the validation phase of the modular resolver that also rules
    out pre-installed package options that would introduce a missing artifact
    condition, so that alternatives if available can be chosen instead, rather than
    producing assignments for a build plan that would fail with ‘files missing’
    (e.g. ‘….hi’ or ‘….dyn_hi’).

Additionally this adds a test that shows what this new step in the validation
phase of the modular resolver can check.

As written in the second commit message:

Add a PackageTest test for missing static files.

    The test builds and installs a package dynamically only, then builds and
    install a package statically that depends on that dynamic package.

    This demonstrates how cabal-install can fail when it ignores which build
    artifacts are provided by a package, e.g. static and/or dynamic files.  Recent
    changes add to the InstalledPackageInfo record so that installed packages are
    more explicit in what is available, so that cabal-install is better able to
    know when it should rebuild from source, when installed packages were not built
    from config flags to build the required artifacts.

(I ran the cabal-testsuite/PackageTests test suite and found ‘432 tests, 6 skipped, 0 unexpected passes, 0 unexpected fails’.)

[bairyn]

(Note: the added test mentions a couple places you can edit the scenario test code to experiment and reproduce configurations for build errors, although without the ‘Fix project-local flags being ignored’ fix, I believe the test would just pass without the extra config flags rather than passing them through properly to see the build errors.)

[bairyn]

I just pushed amendments to this MR that update the remaining tests (along with a rebase), outside PackageTests (isolated changes: bairyn@a0f4763).

[gbaz]

As you've added a new flag, can you add documentation of it to the appropriate place in the manual? (I hope there is such a place!)

[bairyn]

It's added. (And to answer your question, to me it seems more the latter.)

[gbaz]

This will take some care for a full review, but thanks for your work on this! I notice there should be an update to the manual, to document the new flag. (Although -- is there ever a case where we want the flag off, or is this just a safety-for-potential-back-compat thing?) Additionally, as with the other PR, a short changelog entry would be nice.

[bairyn]

I just updated this MR with the following updates:

• Rebased onto upstream, so it's more up-to-date.
• Added changelog entry.
• Updated documentation to add ‘--require-artifacts’.
• Skip the new test on Windows (I noticed the tests I added fail on Windows for some reason; I'll just skip them on Windows for now).

[bairyn]

(Just updated a comment to update the MR: I updated the comment around ‘skipIfWindows’ for the same reason as in #8624.)

MR updates:

• Update comment around ‘skipIfWindows’ in the new test.

[Mikolaj]

Just updated a comment to update the MR

I've invited you to the Triage cabal, which probably lets you do @mergify rebase in a comment in order to rebase the branch. It surely let's you set a merge_me label once 2 reviews are in.

[bairyn]

Thanks Mikolaj.

(I also just rebased this MR's branch so it's up-to-date.)

MR updates:

• Rebased the branch

[Mikolaj]

I think this is fine, but I'm afraid, I'm not able to make any suggestions for improvement (maybe it's just perfect).

[Mikolaj]

@bairyn: we need to cut the 3,10 branch RNS for inclusion in GHC 9.6, but we'd hate to exclude your PR just because we are not able to review it in-depth soon enough. Do you think you could split the PR (again!) into the part that changes Cabal the library (including Cabal-syntax) and the rest?

Then we'd review the Cabal part pronto, merge into the preliminary 3.10 branch that gets included in GHC 9.6 and then, at our leisure, review the PR with the cabal-install part and backport it to 3.10 branch before cabal 3.10 gets released.

Or is it too much to ask?

[gbaz]

Additionally, @grayjay raised a useful question in the call, which is why pkgProfExe is added to the IPI, as it doesn't seem it would be necessary for any particular case. I think I know why pkgDynExe is in the IPI, but I'm not quite sure about that either...?

[grayjay]

I started to review the code, and I have some questions:

• I'm not completely sure I understand the new solver validation pass, but it looks like it checks that the correct artifacts are available along every edge of the dependency graph. Could the logic be simplified to only check that every selected package provides the artifacts specified by the current build flags? For example, if the build used --enable-static, then the solver would check that every package that it chooses is either a source package or an installed package containing static artifacts.
• Why are there two ArtifactSelection inputs to the solver? When would the two values be different?
• Why are there separate values for provided and required artifacts in PInfo? When would they be different (other than when --no-require-artifacts is set)?
• It looks like Dynamic.number is always equal to 3. Would it be better to use a different number between the installed and source versions of Dynamic to check that the correct one was selected?

I also think that this change should have unit tests in https://github.com/haskell/cabal/blob/master/cabal-install/tests/UnitTests/Distribution/Solver/Modular/Solver.hs, including a test for the new error message. If the more complex artifact checking along the edges of the dependency graph is necessary, then it may also make sense to add this to the solver Quickcheck tests in https://github.com/haskell/cabal/blob/master/cabal-install/tests/UnitTests/Distribution/Solver/Modular/QuickCheck.hs.

[bairyn]

Thanks Mikolaj. Yes, I can split the artifact commit into the IPI extension and validation phase parts. (The second piece depends on the first, so the larger PR will have both pieces.)

While I could split it now with the current fields (I already split the commit in a local branch), and it would probably still work, while I was reviewing it and the feedback I thought it might be a bit cleaner and more organized to instead have just 2 new IPI fields, ‘providesStaticArtifacts’ and ‘providesDynamicArtifacts’, and then move the logic that infers the values of these from inside ipiToAS inside the modular resolver to the conversion from LocalBuildInfo in Register.hs. So I thought I'd make that change first.

I should especially have time to work on this PR (fields, adding more tests, responding to the rest of the comments, etc.) this weekend (or, failing that, the next).

[Mikolaj]

@bairyn: thank you again. Given that review of some other crucial PRs lags behind, making this PR's split this weekend should still make it possible to include this PR in GHC 9.6 and, consequently, in cabal 3.10 release. No pressure though, I hope 3.12 release won't be that far away.

[bairyn]

This PR is now split into a smaller PR that has just the first commit:
#8696

This PR currently has all the commits. It now has the following updates:

• Rebased onto upstream, so it's up-to-date.
• Split the artifact tracking into into its 2 pieces (there is a separate PR
  now that can merge just the first piece, and it overlaps with this one).
• Simplify the 5 new IPI fields into just 2 new IPI fields:
  providesStaticArtifacts and providesDynamicArtifacts, and move the
  logic that infers the values of these from inside ipiToAS inside the
  modular resolver to the conversion from LocalBuildInfo in Register.hs.
• Fix Register.generalInstalledPackageInfo's mismatched assignments of the
  IPI fields.

[bairyn]

Thanks for the review, grayjay.

• I'm not completely sure I understand the new solver validation pass, but it looks like it checks that the correct artifacts are available along every edge of the dependency graph. Could the logic be simplified to only check that every selected package provides the artifacts specified by the current build flags? For example, if the build used --enable-static, then the solver would check that every package that it chooses is either a source package or an installed package containing static artifacts.

I suppose I could simplify it to just apply the same artifact requirements for a whole assignment. This would be regardless of dependencies, as currently the implementation assumes that what a package provides and what a package requires are the same.

• Why are there two ArtifactSelection inputs to the solver? When would the two values be different?

• Why are there separate values for provided and required artifacts in PInfo? When would they be different (other than when --no-require-artifacts is set)?

As I said, yes, I could simplify that logic, to use a single value for both what a package requires and what it provides; currently they are assumed to be the same. The existing implementation would permit these to be different, but they are assumed to be the same.

It looks like Dynamic.number is always equal to 3. Would it be better to use a different number between the installed and source versions of Dynamic to check that the correct one was selected?

I could probably add an additional redundant check like this, although the test would include not only how packages are selected but sets up a scenario that reproduces the error that motivated the selection in the first place if the cabal-install doesn't have the fix.

I also think that this change should have unit tests in https://github.com/haskell/cabal/blob/master/cabal-install/tests/UnitTests/Distribution/Solver/Modular/Solver.hs, including a test for the new error message. If the more complex artifact checking along the edges of the dependency graph is necessary, then it may also make sense to add this to the solver Quickcheck tests in https://github.com/haskell/cabal/blob/master/cabal-install/tests/UnitTests/Distribution/Solver/Modular/QuickCheck.hs.

I can also see if I can add cabal-install unit tests.

[bairyn]

(One more change: I mispredicted the PR # of the smaller PR, which I just pushed a fix for.)

MR updates:

• Fixed PR # in the changelog.

[grayjay]

Thanks for splitting the PR!

I'm not completely sure I understand the new solver validation pass, but it looks like it checks that the correct artifacts are available along every edge of the dependency graph. Could the logic be simplified to only check that every selected package provides the artifacts specified by the current build flags? For example, if the build used --enable-static, then the solver would check that every package that it chooses is either a source package or an installed package containing static artifacts.

I suppose I could simplify it to just apply the same artifact requirements for a whole assignment. This would be regardless of dependencies, as currently the implementation assumes that what a package provides and what a package requires are the same.

Yes, I think it would be better to apply the same artifact requirements to all packages in the plan. It would simplify the logic considerably and improve performance. It would also allow all of the logic to be tested by removing cases that aren't supported.

It looks like Dynamic.number is always equal to 3. Would it be better to use a different number between the installed and source versions of Dynamic to check that the correct one was selected?

I could probably add an additional redundant check like this, although the test would include not only how packages are selected but sets up a scenario that reproduces the error that motivated the selection in the first place if the cabal-install doesn't have the fix.

I'm not sure I understand. Maybe I didn't understand the purpose of the line assertOutputContains "Dynamic's number is 3." in the test. What is the purpose of that assertion?

[gbaz]

I raised this again in our cabal meeting and we hope to return to this soon. Sorry for the slowness.

[Kleidukos]

@bairyn I'd like to offer my help to fix the conflicts of your PR, are you interested?

[ulysses4ever]

This looks abandoned at this point, @bairyn?

[gbaz]

This is important work and its our fault it stalled because we couldn't give it sufficient attention. It might need a lot of work to revive, but it would be wrong to drop it on the floor.

[grayjay]

If I understand correctly, this PR is blocked on #8696, and it needs to be updated to address my code review in #8624 (comment) and #8624 (comment). I think we should try to merge it before the formatting changes in #8999.

[andreabedini]

If I understand correctly, this PR is blocked on #8696, and it needs to be updated to address my code review in #8624 (comment) and #8624 (comment). I think we should try to merge it before the formatting changes in #8999.

I feel I have contributed to stalling things because I started working on #6039 which led to the creation of #8967 which I thought a lot about but not really pushed forward. I will try to find the time to push that discussion forward (and perhaps a plan for #6039