Track static vs. dynamic dependencies.

[bairyn]

Hello, and thanks for creating and maintaining this free and open-source package manager.

This merge request tracks what kinds of build artifacts are produced, so cabal-install knows when it should prefer building a package from source to an already installed package that only provides static or dynamic files when more are needed.

This is about errors of the sort:

>  [1 of 1] Compiling Main             ( Main.hs, ../setup.dist/work/depender/dist/build/depender/depender-tmp/Main.o )
>
> Main.hs:3:1: error:
>     Could not find module `Dynamic'
>     There are files missing in the `dynamic-1.0' package,
>     try running 'ghc-pkg check'.
>     Use -v (or `:set -v` in ghci) to see a list of the files searched for.
>   |
>   | import qualified Dynamic (number)
>   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

What prompted this was attempting to build freshly cloned git repositories, indeed even packages from Hackage, using a default ‘ghc’ and ‘cabal-install’ setup on Archlinux. Archlinux currently provides system packages for many Haskell projects but (take, for instance, ‘haskell-scientific’), but seem to only include dynamic files because they are configured to build with (for instance, look at https://github.com/archlinux/svntogit-community/blob/master/haskell-scientific/trunk/PKGBUILD - this builds with ‘--enable-shared --enable-executable-dynamic --disable-library-vanilla’.)

(I noticed also that I had to make additional changes in order for build configuration flags to not be discarded, for these flags to affect the hash of installed packages.)

I found that the default setup was largely unusable for building packages, because I would often encounter errors of this sort. After looking further into this issue, I found that the default cabal configuration was such that it was trying to build static files, but the installed packages it was using lacked them.

(In case it's useful for others encountering similar issues, I'll also note that there some workarounds that can be applied besides my fixes. One is to just change ~/.cabal/config to share the same default dynamic-only configuration, by setting ‘library-vanilla: False’, ‘shared: True’, and ‘executable-dynamic: True’. However, I prefer including as many outputs as I can, especially for more basic system configuration files. So another workaround is simply to use only ‘stack’. I recommend what I did, which is, thirdly, to just build GHC locally (with dynamic and static support), using the system GHC to bootstrap it as a phase 0 compiler (and then also cabal-install). You can even build a patched GHC and cabal-install with my fixes by cloning ghc recursively, updating the Cabal submodule to point to my ‘fix-dynamic-deps’ branch, (optionally copy the sample file to build.mk if you want to make changes to your ghc build configuartion), make a few version changes (ghc.cabal.in, libraries/ghc-boot/ghc-boot.cabal.in, libraries/ghc-prim/ghc-prim.cabal, and libraries/unix can e.g. have an updated Cabal dependency to ‘>= 3.9 && < 3.10’ for your local build), and then run the usual ‘./boot’, ‘PATH=/usr/bin ./configure --prefix=/home/bairyn/local/ghc-9.4.2’, ‘make -j7’, and ‘make install’ sequence. Then you'll have a new GHC and also a new ghc-pkg that recognizes the new ghc-pkg InstalledPackageInfo fields that track which dynamic vs. static builds artifacts are included. Then when building a patched cabal-install from this branch, you can update Cabal requirements to 3.9.* and base requirements to ‘< 4.18’ from ‘< 4.17’ in 3 /.cabal files.)

The DynDeps test added by this MR is, I think, another good overview of what this merge request is about.

[Mikolaj]

Whoa, that's one big PR. Thanks a lot. Are you able to find any related tickets (e.g., error reports similar to what you describe, but also anything else related, e.g., old proposals along similar lines)? I assume such a substantial body of work will require a discussion (goals, design, implementation choices, etc.), so it may be best to tie in to an older one, if it exists. Also, all of us are either fresh cabal maintainers or very busy elsewhere, so we need to be educated first.

I got a bit lost in the variants of solutions/workarounds you present here (all are very welcome, BTW, e.g,. some may find their way into the readthedocs manual). Could we, for now, focus on the main variant? Does it involve patches to GHC? Recompiling GHC? Or is this PR completely standalone (bar any documentation changes, changelog, etc., which can wait until we decide on how to proceed)?

[Mikolaj]

BTW, is the CI failure plausible? Sometimes we get random CI corruption, so let us know if a failed CI job restart might be useful (unless you can restart yourself; I have no idea how the permissions work there).

[bairyn]

Thanks, and thanks for the feedback!

One related error report is one I just wrote an answer to: https://stackoverflow.com/questions/52429645/haskell-there-are-files-missing-in-the-quickcheck-2-11-3-package/.

This StackOverflow post should help explain the context behind this patchset.

The relevant issue this patchset helps to address is quick to be reproduced for some, as a default ghc and cabal-install installation on Archlinux or on another distribution where only dynamic files are provided cannot build projects without additional configuration (e.g. to disable static linking).

There are 2 main additions:

1. The InstalledPackageInfo files (that is, the *.conf files in a package-db that contain records of fields of information about installed packages) have 5 new fields to better track what build artifacts (static or dynamic) have been built and are provided by a package. (Besides cabal-install, ghc-pkg sees these new fields through its Cabal-syntax dependency. These fields default to True, so if they are unspecified, cabal-install will assume all build artifacts are available, essentially as it currently already does. IIRC, old ghc-pkgs will still work if Cabal is patched standalone, but won't emit or recognize these new fields, which would result in behaviour similar to current behavior.)
2. The dependency solver is aware of artifact requirements (static vs dynamic) so it knows when to prefer rebuilding a source package over using an already installed package if that installed package does not provide the static or dynamic files that are required as a dependency.

(Thus even if the system Haskell installation doesn't provide any static files even for base packages as an option, at least cabal patched with this patchset should report that build artifacts are missing if it can't find a source package to build from.)

Another change fixes some cases of cabal ignoring project-local configuration flags to change which build artifacts are built, e.g. to enable static linking. This is relevant to ensure that build artifacts are built as configured.

[bairyn]

Regarding the CI question.

Is a CI failure plausible? Well, because the MR is so big, I'd say it's plausible I may have missed something, but it seems to work for me.

I did do some testing locally before I submitted this merge request, although not as thoroughly as testing multiple versions of GHC in multiple environments etc. I'll explain what I did.

I pretty much only tested with a local GHC 9.4.2 and (then-)HEAD cabal-install, with patches. According to my notes I took around this time, I found some 5 errors in the automated PackageTests test suite without my patches (the control), and without digging too deeply, I assumed the test suite had not yet been updated to work with GHC 9.4.2 entirely. However, there were many other tests and most of them passed.

So I compared the results of this test suite without my patches to the results of running them with my patches. (Of course I added a couple tests that demonstrate problems that the additions fix; this is besides them.) I found, according to my notes, that the only additional failure was one that didn't seem relevant and seems to me like it should have been failing anyway: Regression/T5782Diamond/cabal.test.hs. I didn't look more deeply into it since I figured it probably wsan't too relevant to my fixes, and proceeded to submit the MR.

Here are the results of a run from my notes I recorded earlier:

428 tests, 5 skipped, 0 unexpected passes, 6 unexpected fails.
UNEXPECTED FAIL: Backpack/Includes2/setup-external.test.hs Backpack/Includes2/setup-per-component.test.hs Backpack/Includes3/setup-external-ok.test.hs LinkerOptions/DynDeps/setup.test.hs NewSdist/DeterministicTrivial/deterministic.test.hs Regression/T5782Diamond/cabal.test.hs
cabal-tests  --with-cabal=/home/bairyn/.cabal/bin/cabal -j7  1271.80s user 473.35s system 555% cpu 5:14.13 total

All of these but Regression/T5782Diamond/cabal.test.hs failed without my patches. I believe it was because elaboratedPackageToJ in ProjectPlanOutput.hs handles elabBuildStyle elab where for BuildInplaceOnly it produces dist-dir in the build plan .json file that cabal emits, but for BuildAndInstall it does not include this entry (but has the comment -- TODO: install dirs?). That is, I would expect it to fail.

Besides the automated test suite, I'm using my own ghc&cabal-install Archlinux AUR package I just created that seems to now be able to build things from Hackage and git repos. (It can also build cabal-install itself, which is part of the process of bootstrapping a patched cabal-install for the package.)

[Mikolaj]

Thank you for the explanation. Do I get it right that this change should be backward-compatible?

Could you try to search our issue tracker for relevant keywords and perhaps find an earlier proposal overlapping with yours? Yours has the definite advantage of being implemented :) but I think it may be interesting to find any older discussion about this and perhaps participants of any older discussions could be persuaded to review this PR (two reviews are needed for a PR to be merged).

In a day or two #8420 is supposed to land so GHC 9.4.2 will be available in our CI. Could you then rebase and try to make CI green? Let us know if you get stuck.

[gbaz]

Thanks for this PR.

It seems to me there are 3 components.

1. extend installed package info
2. change the solver to track dynamic/static
3. fix setup depends to allow it to build setup with the dynamic flag set.

Am I missing any, or is that it?

I would like to disentangle these if possible into 3 independent PRs. I think 3 is the smallest, and could stand alone, and definitely should happen asap. I think 1) makes sense, but is architecturally perhaps a bit contentious. I do think we would need to change ghc and ghc-pkg possibly as well, since they need to make use of installed package info even absent cabal-install. So perhaps we need to pull in @bgamari or @mpickering or other ghc people to involve.

As for 2 I'm not sure this is the right approach. For packages in the store, whether they're built with dynamic flags or not should be baked into their hash, just like with profiling libs. For packages in the global package set, we can simply solve as is, then error helpfully if they don't have the appropriate dynamic or static available, just like we do with profiling libs?

In fact, I confess I don't fully understand the motivation here at all: "The dependency solver is aware of artifact requirements (static vs dynamic) so it knows when to prefer rebuilding a source package over using an already installed package if that installed package does not provide the static or dynamic files that are required as a dependency."

As is, the dependency solver is entirely independent from choosing to rebuild or not. It always just finds the "best solution". Only after the solver, when the build plan is constructed, do we check if the thing we want already exists with the right hash (reflecting the right flags) or not.

In fact, I would go further and suggest that even for 1) perhaps the culprit is ghc and not cabal. It is ghc that when it sees a package is available assumes that the static lib is always there, and that the dynamic and profiling versions are only optionally there, and so ghci directly gets confused when there is a package with a dynamic lib and not a static one. And to cause even more trouble, I think that ghc should possibly be able to auto detect if static is present, just as it currently can with dynamic and profiling.

So I think the final change is still good and important, but am genuinely not certain that that the other two are the correct approach here.

[bairyn]

gbaz,

This PR doesn't add a new functionality to specify whether to rebuild or not. It just rejects installed packages missing required artifacts. The dependency solver already rejects installed packages that don't meet required versions. With this changeset, it also checks build artifacts to reject options that don't meet the build artifact requirements. (That is, the build plan it constructs checks not just for the correct version but for the correct artifacts too.) There is a list of error messages that Cabal provides for rejecting options for various reasons in cabal-install-solver/src/Distribution/Solver/Modular/Message.hs, and this patchest adds e.g. missing build artifacts: static artifacts.

The dependency solver IIRC is provided an index for installed packages and an index for source packages, and prefers installed packages where available, and if no installed packages meet the criteria, then it will check the source package index to see if a new one can be rebuilt. This part of the modular solver was already there. (convCP takes a package index for installed packages and one for source packages.)

That said, if you still think only part of it should be merged and the rest rejected, and there is enough consensus toward this end, then I suppose that's what the community wants and I or somebody else can perhaps try to accommodate by splitting the PRs. Although I think a Cabal that also checks build artifact availability and requirements when rejecting valid options is a better Cabal. (Also GHC can still be updated independently of these changes.)

Thanks for your feedback, and happy to contribute.

[bairyn]

Mikolaj,

Yes, this patchset should be backwards compatible. That is, a patched cabal will still work with installed packages and package databases from unpatched cabals; it would just assume that all required build artifacts are available, since the extended InstalledPackageInfos default to True.

And sure, I just skimmed through the issue list on this hosted repo and searched issue tracker a bit (not particularly carefully, at least for now) and noted issues that seemed like they might be relevant:

• cabal v2-install ignores flag --run-tests #7267
• -dynamic-too is not enabled for local library dependencies #7684
• v2-install does not apply flags to build #6472
• new-haddock: '-dynamic' flag is not passed to haddock (archlinux, dynamic GHC package) #4670
• Static/dynamic library detection #2055
• --extra-include-dirs and --extra-lib-dirs not respected when building Setup itself #1378
• local options do not work with install on local packages (was: Run okay with profiling enabled, but installed exe has NO profiling enabled) #7297
• cabal v2-build fails to override global config with --enable-static --disable-shared #6688
• Cabal says package hashable has missing files even though it doesn't. #5913
• Missing files in package #3093

I expect this would help many users, who have encountered similar issues as I have, where at least for me with the error message provided the problem wasn't initially clear to me (see, for instance, https://wiki.archlinux.org/title/Talk:Haskell#[Announcement]_Article_rewrite).

[gbaz]

"The dependency solver IIRC is provided an index for installed packages and an index for source packages, and prefers installed packages where available, and if no installed packages meet the criteria, then it will check the source package index to see if a new one can be rebuilt. This part of the modular solver was already there. (convCP takes a package index for installed packages and one for source packages.)"

I think this is incorrect, and the nub of the confusion. In v2-build the solver does not prefer installed packages where available. It always solves fresh. (However, it also will be given constraints that constrain it for solving for any but the installed version of nonreinstallable packages). So it is not clear to me what this accomplishes. I think we would need a reproducible situation where one thing happens without the solver patch, and another with it, so we can concretely discuss this.

This is to say that I know that working with arch is a mess, and I think if we could have some straightforward improvements to that situation it would be good, but I am not convinced that the solver component of this PR actually brings about such an improvement, and having a reproducible case that showed how the new behavior differs, so that we could discuss it concretely would help in that understanding.

[bairyn]

gbaz,

It is not incorrect. If you have a global database of installed packages, v2-build does not just ignore them and always rebuild everything from available source packages through Hackage. If you have a parsec dependency, and parsec is installed in the global packagedb (and nowhere else) from, say, system packages, then cabal-install will prefer the installed package. If, however, you only have versions 1.1 and 1.2 installed, and you require ‘>= 1.3’, then the solver will only find building from source as the only valid option to satisfy that dependcy.

What this PR accomplishes is ignoring (rejecting) not just packages with invalid versions, but packages that have missing build artifacts like required static files. This prevents Cabal from constructing build plans to try to build when it will fail to build because the resolved packages are missing things, so it doesn't even try to attempt those. Even without this patchset, Cabal with default configuration is already prevented from constructing build plans that have contradictory version requirements. And this PR makes Cabal aware not just of versions but also of artifact type configuration (static and dynamic).

If you look at the source, you'll find not just 1 but 2 package index arguments, collections of packages in cabal-install-solver/src/Distribution/Solver/Types/DependencyResolver.hs:

-- | A dependency resolver is a function that works out an installation plan
-- given the set of installed and available packages and a set of deps to
-- solve for.
--
-- The reason for this interface is because there are dozens of approaches to
-- solving the package dependency problem and we want to make it easy to swap
-- in alternatives.
--
type DependencyResolver loc = Platform
                           -> CompilerInfo
                           -> InstalledPackageIndex
                           -> PackageIndex (SourcePackage loc)
                           -> PkgConfigDb
                           -> (PackageName -> PackagePreferences)
                           -> [LabeledPackageConstraint]
                           -> Set PackageName
                           -> Progress String String [ResolverPackage loc]

See:
cabal-install-solver/src/Distribution/Solver/Types/ResolverPackage.hs:

-- | The dependency resolver picks either pre-existing installed packages
-- or it picks source packages along with package configuration.
--
-- This is like the 'InstallPlan.PlanPackage' but with fewer cases.
--
data ResolverPackage loc = PreExisting InstSolverPackage
                         | Configured  (SolverPackage loc)
  deriving (Eq, Show, Generic)

I also suggest taking a look at how convCP decides whether to use the source package index or the installed package index for a given package (https://github.com/haskell/cabal/blob/master/cabal-install-solver/src/Distribution/Solver/Modular/ConfiguredConversion.hs). Perhaps also see where these types are used in the modular resolver.

You wrote ‘I think we would need a reproducible situation where one thing happens without the solver patch, and another with it, so we can concretely discuss this.’.

Well, yes, I already added that test to the PR. It's called ‘DynDeps’.

[gbaz]

convCP is not about the solver at all. It is about, as the comment says, converting from a from the solver specific result into a 'ResolverPackage', which can then be converted into the install plan. It does not happen as part of solving. It happens after solving.

The call site for convCP also has a comment that explains it is called because "Results have to be converted into an install plan."

There is a possibility for cabal to prefer installed rather than latest when solving. However, note that this is a soft preference (i.e. nonbinding). https://github.com/haskell/cabal/blob/207d4ee08b929ae71ae2c746fe6da660bc129f05/cabal-install/src/Distribution/Client/Dependency/Types.hs

I see the source of confusion here though -- the actual behavior is a bit mixed. Cabal solves regardless of any other packages in the store but it does have a soft preference for installed "global" packages -- i.e. the default preference is PreferLatestForSelected which will attempt to prefer the latest packages for those that are selected explicitly, but prefer installed packages for those which are not.

That said, I still don't think changing the solver is the right approach. Again, the preference behavior is a soft suggestion that just changes the order cabal tries things in. One thing we could do would be to not prefer installed dynamic only packages if they're not actually providing static builds -- that makes sense. But also we could just change convCP and friends, while leaving the solver untouched -- i.e. if we solved and the answer was an installed package, but that package does not provide the desired static libs, we install another instance of it in the store, and use that one. This is perfectly fine, and in fact how cabal is designed. Does that sound like a good approach?

In your test, for example, supposing dynamic only had a single version. Then the approach as implemented here would fail -- the solver could not pick that version, because it was installed dynamic only, and there would be no other one to try. If the resolution/install-plan-construction was instead changed, then the solver could succeed and the desired lib with the desired static libraries would just be installed into the store, no problem!

[bairyn]

@gbaz,

In your test, for example, supposing dynamic only had a single version. Then the approach as implemented here would fail -- the solver could not pick that version, because it was installed dynamic only, and there would be no other one to try. If the resolution/install-plan-construction was instead changed, then the solver could succeed and the desired lib with the desired static libraries would just be installed into the store, no problem!

That's not what my implemented approach does; you are mistaken. It would try a source package if one were available (e.g. from Hackage), even if it has the same version; not nothing. Say the modular solver was provided only one version, and two package options: one is an installed version 3.8 (via the InstalledPackageInfo type), and the other is a source package (via the SourcePackage type) with version 3.8. (Through type aliases, the modularResolver function takes two PackageIndex types, one of InstalledPackageInfos, and another for SourcePackages.) Unpatched, default cabal-install would have 2 options to satisfy the pkgName dependency: the InstalledPackageInfo (later converted into Modular specific forms, maps of package names to maps of options/instances and package information (types are in Index.hs)), and the SourcePackage. Say the installed package is partial and lacks static artifacts. Unpatched, default cabal-install wouldn't reject either option (either I). When it comes time to convert from the solver types, convPI (called by convCP) branches on (PI _ (I _ (Inst pi))) to what will be PreExisting InstSolverPackage values, and pattern matches on all others to Configured (SolverPackage …) values. (Note: it looks like toCPs takes the reverse dependency map of qualified package names and components produced by the solver phases along with an assignment of QPNs to Is, to produce CPs, and then convCP chooses between source versus installed packages based on what's inside I. The I is not just the version but also whether or not it is pre-installed. I does not contain just the version, but also whether it an IPI or a source package.)

-- | The dependency resolver picks either pre-existing installed packages
-- or it picks source packages along with package configuration.
--
-- This is like the 'InstallPlan.PlanPackage' but with fewer cases.
--
data ResolverPackage loc = PreExisting InstSolverPackage
                         | Configured  (SolverPackage loc)
  deriving (Eq, Show, Generic)

So the modular solver deals with Is. These are the options. It can try these options. Some might fail. The modular solver can tell whether each I (option) is pre-installed or not. It can also tell what the version of each option is.

Now the unpatched cabal-install would see both options as valid. If it picked the pre-installed option in the Assignment (it's a valid option), then cabal-install would produce a build plan that would fail; you'd get the usual error message I've been referring to. The patched cabal-install would recognize picking the pre-installed option would fail. It would only treat the option that builds from source as the only valid option left - unless you explicitly hide Hackage and all source packages. Then there would be no valid options remaining. And to relate to this case, here's a quote from my test; what you would see in this case with a patched cabal-install is not a files missing build error, but an error like this:

            -- (For instance, if you comment out the sdist references so that we
            -- only see the installed package, you should see an error message
            -- like this:)
            -- > Error: cabal: Could not resolve dependencies:
            -- > [__0] trying: depender-1.0 (user goal)
            -- > [__1] next goal: dynamic (dependency of depender)
            -- > [__1] rejecting: dynamic-1.0/installed-19c7c1e50b8f1e56115c4f668dfdadd7114fc2c7dad267c2df43028892cc0ff5 (missing build artifacts: static artifacts)
            -- > [__1] fail (backjumping, conflict set: depender, dynamic)
            -- > After searching the rest of the dependency tree exhaustively, these were the goals I've had most trouble fulfilling: depender (3), dynamic (2)

This means also that if you have global system packages installed through an alternative package manager, and user packages installed in a user store, then a patched cabal-install can successfully rule out the global packages of whatever versions and use only the user store that it built with all artifacts, or otherwise it may produce a build plan that may have some packages from the user store, and some from the global database, but some of these installed packages are broken, and if cabal-install tries to build with the global packages to satisfy some dependencies, the build will fail. You'll get files missing errors.

Now I believe you proposed an alternative approach that doesn't rule out invalid Is on account of missing build artifacts, but instead picks an I (source or pre-installed), but then at the end (during convCP) if the I it ended up with is an IPI (pre-installed package) that is invalid on account of missing build artifacts, ignore all other potential Is we could have considered and instead just check again to see if there is a source package.

So do I think it's a good approach? I mean, while both approaches can handle at least some cases, this proposed approach could fail in the case I mentioned, where you have a global package DB but that does not provide all build artifacts, yet cabal-install has produced for you a user store that provides all needed build artifacts; because it could pick an I that corresponds to an installed package in the global package DB, but later on when you're not picking between various options, you discover that actually that option will fail, so patch in a source package instead if it's available and has the same version or so, so then it skips the user-store installed package you already built and just rebuilds a new one from source, when it could have picked the user-store installed package. So I think my approach in this PR is better.

(To be clear, let's say you have a global package DB that lacks static artifacts. You try to build an attoparsec you cloned, with default configuration, that requires static build artifacts. An unpatched cabal-install won't know that a build plan that uses those global package DB packages will fail, so it will consider them valid options, rather than ruling them out. Then you get files missing errors when you try to build. However, a patched cabal-install won't try to use those packages, and will instead download the scientific dependency from Hackage. If there already is a scientific package installed in the user store, then cabal-install if patched can recognize this option as valid.)

However, you did mention options and preferences, and I think the idea of having the equivalent of ‘allow-newer’ here to force cabal-install to bypass these checks and permit dependency assignments with missing build artifacts has potential to be a good one. I haven't yet looked in detail how ‘allow-newer’ is implemented and how a similar option could be integrated here, but I think it could probably be done without too much hassle. However, this PR I think could still be merged without it, and I could add this option later on top of it; or perhaps we could merge it all together. But it probably isn't necessary to delay merging this PR until this additional option is added.

Thanks for volunteering your time to engage with us.

[bairyn]

@Mikolaj

You mentioned 9.4.2-related changes, and I see that these have been merged. So once I return to this task (likely I'll find time to start by the end of this weekend), then I can take a look at the CI side of things, if nobody's beat me to it.

[bairyn]

And I also thought I'd just note:

Thanks for this PR.

It seems to me there are 3 components.

1. extend installed package info

2. change the solver to track dynamic/static

3. fix setup depends to allow it to build setup with the dynamic flag set.

Am I missing any, or is that it?
…

I think that's a good way to summarize this PR, yes. I think that's it.

[gbaz]

" Unpatched, default cabal-install would have 2 options to satisfy the pkgName dependency: the InstalledPackageInfo (later converted into Modular specific forms, maps of package names to maps of options/instances and package information (types are in Index.hs)), and the SourcePackage" -- ah this is what I wasn't seeing in your approach before. I'm coming around to understanding why this is the right approach.

I'd also like to understand why FailSeed needs to be introduced and the associated refactor to addChildren. Why not either just not add the things with missing artifacts directly, or handle the artifact checking later in the validation phase?

(I suppose the former doesn't give the error message you'd like, but I'd think the latter does?)

Alternately, one could maybe add the check in the Instance clause of addChildren instead, also bypassing the need for a new constructor?

[Mikolaj]

I'm not qualified to meaningfully contribute to the discussion, but I'd like to chime in that splitting a big PR into smaller self-contained PRs (or at least well described commits) is a wonderful practice with all kinds of benefits.

[bairyn]

@gbaz

Good questions.

I'd also like to understand why FailSeed needs to be introduced and the associated refactor to addChildren. Why not either just not add the things with missing artifacts directly, or handle the artifact checking later in the validation phase?

(Background: cabal-install-solver/src/Distribution/Solver/Modular/Solver.hs's solve has a long chain of phases.)

Yes, combining the generation and validation parts was a bit sloppy of me (and arguably reduces the standards a bit compared to the surrounding code; I like the modular design of the solver and the organization such that the generation and filtering are separated; and I suppose the modular design is meant specifically to cleanly accommodate adding additional phases such as this).

(The ability to be able to decouple generating solutions from finding the right one is one of the classical reasons why functional programming matters, and cabal-install’s constraint solver makes very heavy use of this. - https://well-typed.com/blog/2015/03/qualified-goals/)

And yes, thanks for pointing out the validation phase as what appears to be the more fitting place for this.

My plan is to clean this part up by moving this particular check (artifact satisfaction) from the build phase to the validation phase. Then once finished with that I can share the second iteration perhaps as a new full PR linked to this one and a partial PR after checking the CI tests.

(Basically I believe these changes work, but the implementation could be better organized.)

@Mikolaj

You mentioned ‘self-contained’; however, the test I added for the artifact
satisfaction checker (or whatever you may want to call it) depends on the first
fix (about ignored configuration settings).

This PR has 4 commits:

• ‘Fix project-local build flags being ignored’
• Add a test for ‘Fix project-local build flags being ignored’
• ‘Track build artifacts in installed packages’. - That is, 1) extend
  InstalledPackageInfo (e.g. the data inside
  /usr/lib/ghc-9.4.2/package.conf.d/….conf) with 5
  new fields, and 2) add a build artifact checker phase to the modular solver.
• Add a test for ‘Track build artifacts in installed packages’. I believe this depends
  on ‘Fix project-local build flags being ignored.’

(Now that I mention it, though, the test not working without the fix is such that probably it won't fail when it should; so probably actually it could be self-contained after all (I'd just have to double check), only the test I think would just not fail when should, so it wouldn't demonstrate failures that could happen without the fix, so it'd at least be related.)

[mouse07410]

I just want to chime in and express my support for and appreciation of this effort.

[Mikolaj]

@bairyn: right, as self-contained as possible, but not more. :) It's absolutely fine if some tests can only be placed in the final PR / after the final implementation commit. BTW, please don't get discouraged by our nit-picking (and the more substantial comments) --- these are precisely because we are very interested in getting this improvement merged.

[gbaz]

@bairyn have you had a chance to move this PR closer to the finish line? I think you've made a convincing case for the general approach, and it would be good to get this into the next release.

[bairyn]

Thanks for the support, and sorry to keep you waiting.

I wrote a second implementation (in which there is a new pass in the validation phase to detect missing build artifacts for package instance choices, assuming the solver configuration does not disable it), and I am planning as I have time to run some tests and finalize a few things (in particular handling the diamond test), and then I can submit the second iteration PRs.

[bairyn]

I've cleaned up and split this PR into #8623 and #8624; I'll close this PR with this comment as these new PRs replace this one.