Add Backpack support to Stack#6865
Conversation
|
@philippedev101, thanks! There is a lot to read but, in advance of that, in implementing component-based builds, did you encounter, and overcome, the performance issue that was blocking @theobat making progress at: |
|
@mpilgrem Thanks for the pointer to #6356 and @theobat's work. After going through the discussion there and the branches on their fork in some detail: short answer, this PR takes a different architectural approach that sidesteps the performance problem, at the cost of not solving the broader set of issues that full per-component builds would address. How the two approaches differ@theobat's approach was to make Stack build each component of a package as a separate The approach in this PR is narrower. The build plan is keyed per-component using a What this does not solveFull per-component execution would address several long-standing issues that this approach leaves untouched: Unnecessary recompilation when switching between Rebuilding components that aren't dependencies of the target (#6569). If you ask for Parallel builds within a package (#4391). A package with five independent executables currently builds them sequentially (Cabal handles the ordering internally). Per-component execution would let Stack schedule them in parallel, the same way it parallelizes across packages. Component-level cycle resolution (#2583). When package A's library depends on package B's library, and B's test suite depends on A's library, that looks like a cycle at the package level. At the component level it's not: build A:lib, then B:lib, then both test suites. Per-component execution could handle this. Where this PR sitsThere are roughly three points on the spectrum:
This PR lands at point 2. It gets cross-package Backpack working today without regressing anything for existing users. Transitioning to full per-component builds laterThe
None of these changes require rethinking the plan-level architecture. They're about threading component information through the execution layer. And they're independent of the subprocess performance question, which could be tackled separately (the in-process |
|
@philippedev101, the failing STAN CI can wait, but the integration tests are failing on all operating systems for a reason that I can reproduce locally (on Windows 11), namely:
(Note to self: the poor output of the However, if (If I've changed the PR status to 'draft', in the interim. |
|
A minimal reproducer of the problem is a simple one-package project that has a The custom-setup:
dependencies:
- base >= 4.7 && < 5
- Cabal |
|
@mpilgrem Thanks for the clean repro and the draft-status pause. Root cause identified, fix pushed as a follow-up commit on this branch. Short answer: for Root causeAfter Phase 2, the build plan is split into For finalBuild = case mbuild of
Just _ -> []
Nothing -> [ ... singleBuild ... isFinalBuild=True ... ]When both Stack's own project reproduces this because Fix
In Two downstream guards match the new split: The decision table lives in a small pure helper TestsSeven new integration tests under
Thirteen new unit tests in Stack's own One incidental fixWhile running the Backpack integration suite as a regression check I noticed What this does not changeThe classic Happy to move the PR out of draft. |
|
@philippedev101, unfortunately, other existing integration tests fail. I had a look at one of them: With Stack 3.9.3, the first and the second Unlike when the test was originally written, in neither case is the The 'backpack' version of Stack has, for the first In this case, the For the second The stackCheckStderr
["build", ":alpha"]
(rejectMessage
(unlines
["Preprocessing executable 'beta' for foo-0..."]))))Perhaps the test should check for the absence of |
|
@philippedev101, I rebased on the |
|
I then looked at integration test With Stack 3.9.3, all is fine: The 'backpack' version of Stack fails with (extract): Stack is trying to build a library component, and one that does not exist. |
|
@philippedev101, I have rebased on With Stack 3.9.3, all is fine: The 'backpack' version of Stack fails with (extract): |
|
Given these three regressions, and the other failing integration tests, I have changed the status of the pull request to draft. |
|
@mpilgrem Thanks for checking the work and for your patience. The three regressions you flagged are fixed. Three things from your review style shaped how I verified the fixes this time:
I also ran both Stack test suites. Unit suite all green. Integration suite has 7 failures; all 7 also fail on the previous PR head, so none are introduced by this update. Five are environmental on my NixOS host: The three regressionsAll three sit downstream of the same root cause. The previous PR head used per-component planning for every Simple package. The split path is meant only for Backpack; routing everything through it broke three different shapes. The fix is to narrow the predicate so non-Backpack packages stay on the legacy whole-package path. The Backpack code path itself (per-component planning, Bug 01 ( shouldSplitComponents pkg =
pkg.buildType == Simple
&& not (hasIntraPackageDeps pkg)
&& not (hasMultipleRegisterableLibraries pkg)
&& packageUsesBackpack pkg
Bug 02 ( -- before
in case allComps of
[] -> [(ComponentKey name CLib, adr)]
_ -> map (\comp -> (ComponentKey name comp, adr)) allComps
-- after
in map (\comp -> (ComponentKey name comp, adr)) allCompsBug 03 ( -- before
exeComps = map CExe $ Set.toList $ buildableExes pkg
-- after
exeComps = map CExe $ Set.toList $
if lp.wanted
then exeComponents lp.components
else buildableExes pkg
Performance12 rounds each, warm snapshots, default Stack parallelism ( Non-Backpack project (one library + two executables):
Same Setup invocation count as Stack 3.9.3. Clean-build wall clock is within noise. The opening claim of "no performance hit for non-Backpack projects" holds. Backpack project (sig-pkg + impl-pkg + consumer-pkg, lib + 1 exe):
Slowdown vs the equivalent non-Backpack project: ~1.5x wall clock, ~4.7x phase count. Per-phase cost is actually lower for the Backpack project; each phase compiles a tiny module and the wall-clock difference is the fixed cost of each Inherent Backpack cost vs unsolved engineeringThe 14 phases break down as:
Of those:
So the extra cost of cross-package mixin use is "one indefinite build plus one instantiation per consumer-distinct filling". Everything else is either normal-package work that would exist anyway, or Stack engineering that can be reduced in a future PR. PR #6883 was essential for these fixes: the original Moving back out of draft. I think this is ready, but please push it back to draft if anything else surfaces. |
|
@philippedev101, I have rebased on
In case it affects your changes, the principal effect of #6908 (fixing #6905) was at the start of logDebugPlanS "addFinal" "Clearing the call stack."
res <- local (\ctx' -> ctx' { callStack = [] }) $
addPackageDeps package >>= \caseEDIT: I rebased again, because I added an integration test to |
mpilgrem
force-pushed
the
backpack
branch
2 times, most recently
from
d4bc6e3 to
0af67a2
Compare
|
@philippedev101, the three remaining regressions are:
With Stack 3.9.3, The 'backpack' Stack behaves as if it is ignoring the
This tests a project with project packages A and B. A depends on B and the test suite of B depends on A. With the However, the 'backpack' Stack fails during the execution of the build plan with:
The tests a project that is similar to |
Implement full support for GHC's Backpack module system, addressing the long-standing request in issue commercialhaskell#2540 (open since 2016). Phase 1 — Intra-package component ordering: Detect sub-library self-dependencies (the Backpack pattern) and skip per-component splitting for those packages, preserving Cabal's own component ordering. Phase 2 — Component-keyed build plan: Replace the per-package build plan with a per-component plan using ComponentKey (PackageName, UnqualCompName). Each library, executable, test, and benchmark gets its own entry in the plan, enabling fine-grained dependency tracking between components across packages. Phase 3 — Cross-package Backpack instantiation: When a consumer package uses mixins/signatures to depend on an indefinite (signature-only) package, Stack now automatically creates CInst instantiation tasks that compile the indefinite package against the concrete implementation. This includes: - Preserving Backpack metadata (signatures, mixins) through the plan - Detecting indefinite packages and creating CInst build tasks - Configuring CInst tasks with --instantiate-with flags - Module resolution scoped to consumer's build-depends - Transitive chain support (inherited signatures from indefinite deps) - Multiple instantiations with different implementations (deduplicated) - Sub-library mixin and module resolution - Remote/snapshot indefinite packages (loaded from Hackage/Pantry) - HidingRenaming support (no partial instantiation; hides propagate) - Per-CInst config cache (ConfigCacheTypeInstantiation) - Precompiled cache support for CInst tasks - Haddock generation for instantiated packages - Build output showing instantiation details Test coverage: 103 unit tests (ConstructPlanSpec), 8 integration tests (backpack-private, backpack-sublib-deps, backpack-cross-package-sublib, backpack-cross-package-sig, backpack-cross-package-rename, backpack-cross-package-transitive, backpack-cross-package-multi-inst, per-component-build). Documentation: new Backpack topic page, ChangeLog entry, and cross-references from tutorial pages.
|
@philippedev101, I've made unrelated changes in the
In particular, Stack now handles Cabal packages with sublibraries but no main library. That means, for Cabal packages that are installed as one or more installed packages with munged package identifiers, there is no single I tried to rebase myself, and got so far, but I was not confident that I knew in your added code when you were referring to a Cabal package and when you were referring to an installed package. So, I aborted. One approach might be for me to rebase what I do understand and then hand over to you to fix up. However, what I hand over will necessarily not compile. Let me know what will help most. |
Brings PR commercialhaskell#6865 up to current upstream master (e6170f1), past: * commercialhaskell#6921 / commercialhaskell#6920: don't ignore deps named like sub-libs or foreign libs, plus the reformatting/refactor in front of that fix. * commercialhaskell#6929: refactor/reformat sweep with `allDeps` documentation and a type synonym for the build-log suffix parameter. * commercialhaskell#6928 / commercialhaskell#6896 / commercialhaskell#6912: `InstalledLibraryInfo` now carries `Maybe GhcPkgId` for the main library plus a `Map StackUnqualCompName GhcPkgId` of sub-libraries (some Cabal packages have only sub-libraries with no main library, and now Stack represents that). `Task.present`, the custom-setup dependency map, and `ConfigCacheTypeFlagLibrary` are now keyed by `MungedPackageId` / `PackageIdentifier` instead of by `GhcPkgId`. Adapts the PR's code to the new shapes: * `findGhcPkgId` filters for main-library entries (`LMainLibName`) in the new `Map MungedPackageId GhcPkgId`. * `mkInstantiateWithOpts` takes the new map shape unchanged otherwise. * `addInstantiationTasks` wraps each implementing-package identifier with `toCabalMungedPackageId pid Nothing` before adding it to a CInst task's `present` map. * `ConfigCacheType`'s PersistField instance keeps the PR's `ConfigCacheTypeInstantiation Text` variant on top of upstream's `flagCache` helper restructure. `Task.allInOne` is intentionally NOT restored here; that lands as a separate commit on top of this merge. The PR's other changes (Phase 1 component-keyed plan, Phase 2 per component split path for Backpack-using packages, Phase 3 CInst tasks, the existing fixes for commercialhaskell#3996 / commercialhaskell#3959 / commercialhaskell#6451, the custom-setup TestSuite fix from PR commercialhaskell#6884, etc.) are unchanged. Verified: full stack build + 858 unit tests pass on the merged tree.
|
@mpilgrem Apologies. I thought I had verification properly nailed down before the previous push, but I got the baseline wrong: I'd thought I was using the upstream master SHA when in fact I had taken a previous commit of this PR. So "fails on baseline too, pre-existing" let three real regressions through. They're fixed in this push. I've also rebased the PR onto current master past #6929, so the partial-handoff you offered isn't needed; the next thing only you can do is the cross-platform CI matrix. The three regressions all sit downstream of the same thing. Phase 2's per-component refactor dropped master's
FixPutting
The per-component Backpack execution path ( On the Cabal-package vs installed-package distinction you flagged when you tried the rebase: that was the substantive part, not the type renames. The boundary is most explicit at VerificationThe baseline is freshly-fetched |
Fixes the three regressions reported in the latest review: * 3770-no-rerun-tests: `stack --no-rerun-tests test` re-runs the suite instead of skipping it. * 6905-cyclic-plan: `[Cabal-8010]` missing `myPackageA` when building `myPackageB`'s test suite that depends on it. * 6905-multi-test: same `[Cabal-8010]` signature, similar shape. All three share one root cause: Phase 2's per-component refactor dropped master's `allInOne` flag. `toActions` was left deriving the merged/separate choice from `isJust mfinal` and `isJust mbuild`, which conflates two unrelated questions and breaks each of the reported shapes. The fix puts `allInOne` back: * Added `allInOne :: Bool` as a field on `Task`, and re-added `ATBuildFinal` to `ActionType`. * `installPackage` computes the flag for the final task, curator-aware in the merged branch (a curator that expects test failures forces a split, so a failing test can't block the library from reaching its dependents) and `False` in the cyclic-plan fallback. Curator handling was silently dropped in Phase 2; it's back too. * `addFinal` forces `False` for the per-component split path (Backpack packages routed through `shouldSplitComponents`), whose finals live under distinct `CTest`/`CBench` keys and always need their own build step. * `toActions` reads `task.allInOne` instead of guessing. An all-in-one final is built by the primary's `ATBuild` in merged mode (or skipped entirely if the package is up to date); a non-all-in-one final gets its own `ATBuildFinal` action built from the final task's own `configOpts`. * `buildAnnSuffix` is updated to match upstream's annotation logic: a primary CLib build now produces `(lib)` / `(lib + exe)` / etc. suffixes when `task.allInOne` and the relevant component shapes are present, matching the test expectation introduced upstream in `6342-say-ghc-version-in-build`. The per-component Backpack execution path (`shouldSplitComponents`, `CInst` tasks, `--instantiate-with`, per-instantiation `--builddir`) is unchanged. This strictly restores the merged-build option Phase 2 took away. Adds two ChangeLog entries under "Changes since v3.11.1 > Bug fixes": * `stack test --no-rerun-tests` correctly skips test suites that have previously been built and run successfully, including for non- Backpack packages built by the per-component planner. * Stack's per-component build planner correctly handles the cross-package test/library cycle shape from issue commercialhaskell#6905. Verified: * Unit suite: 858 examples, 0 failures. * Targeted integration tests: 3770-no-rerun-tests, 6905-cyclic-plan, 6905-multi-test, plus the Backpack cross-package tests and backpack-doc-sublibs all pass. * Full Linux integration suite: 6 pre-existing environmentals on this NixOS host fail (also fail byte-identical on plain upstream/master); one Hackage Security TUF flake on Cloudflare-fronted hosts (basic-install) passes on isolated re-run. None are regressions.
|
@philippedev101, thanks! Unfortunately, the integration tests are failing on Windows (only). I don't think it is a GitHub glitch, as I re-ran them and the fail repeated. EDIT1: I did wonder: could it possibly be a problem with long file paths in the CI's Windows environment? I am going to investigate what happens locally on my Windows 11 machine (which has long paths enabled). EDIT2: I could reproduce the problem locally, in:
with (Also: the same experience just commanding Extract from output (just the EDIT3: (With EDIT4: Trying that command directly (in EDIT5: It looks to me that it is somehow long paths related, even on a Windows 11 system with long paths enabled. That is because if I move the EDIT6: This does, indeed, appear to be a bug in # stack.yaml
snapshot: ghc-9.10.3
extra-deps:
- "D:\\Users\\mike\\Code\\GitHub\\haskell\\cabal\\Cabal"
- "D:\\Users\\mike\\Code\\GitHub\\haskell\\cabal\\Cabal-syntax"
- alex-3.5.4.2
- process-1.6.29.0
flags:
process:
os-string: true
packages:
- app
- consumer-a
- consumer-b
- impl-a
- impl-b
- sig-pkgand after patching Stack's mkVerbosity defaultVerbosityHandles verbositywhere previously it used So, it looks like the |
|
Double-checking that the six failing integration tests in CI Windows pass when run locally and manually but from a short path location:
Note 1: The output from @philippedev101, are those warnings expected? They have the potential to disconcert users, even if it is safe to ignore them. (I usually associate those sort of warnings with something being up with the package |
|
@philippedev101, unrelated to this pull request, the Windows CI is now failing because of: |
|
@philippedev101, I think (and hope) that #6932 now merged in from the |
|
@philippedev101, on Windows 11, I built this against the improved The bad news is, that integration test What has me puzzled, is that if I build against the existing I need to investigate further. Perhaps 6b8f160 has broken something, somehow ... EDIT1: It is not that 6b8f160 has broken something, it is that there remains some long path problem somewhere, despite 6b8f160. EDIT2: More information from the local experience on that test: Something is going wrong with that final 'creating' ... |
|
I think I have identifed the residual long path bug in -- | Writes a file atomically.
--
-- The file is either written successfully or an IO exception is raised and
-- the original file is left unchanged.
--
-- On windows it is not possible to delete a file that is open by a process.
-- This case will give an IO exception but the atomic property is not affected.
writeFileAtomic :: FilePath -> LBS.ByteString -> IO ()
writeFileAtomic targetPath content = do
let (targetDir, targetFile) = splitFileName targetPath
Exception.bracketOnError
(openBinaryTempFileWithDefaultPermissions targetDir $ targetFile <.> "tmp")
(\(tmpPath, handle) -> hClose handle >> removeFile tmpPath)
( \(tmpPath, handle) -> do
LBS.hPut handle content
hClose handle
renameFile tmpPath targetPath
)which gets used by -- | Write the given autogenerated files in the autogenerated modules
-- directory for the component.
writeAutogenFiles
:: Verbosity
-> LocalBuildInfo
-> ComponentLocalBuildInfo
-> Map AutogenFile AutogenFileContents
-> IO ()
writeAutogenFiles verbosity lbi clbi autogenFiles = do
-- Ensure that the overall autogenerated files directory exists.
createDirectoryIfMissingVerbose verbosity True autogenDir
for_ (Map.assocs autogenFiles) $ \(file, contents) -> do
let path = case file of
AutogenModule modName (Suffix ext) ->
autogenDir </> ModuleName.toFilePath modName <.> ext
AutogenFile fileName ->
autogenDir </> fromShortText fileName
dir = takeDirectory path
-- Ensure that the directory subtree for this autogenerated file exists.
createDirectoryIfMissingVerbose verbosity True dir
-- Write the contents of the file.
rewriteFileLBS verbosity path contents
where
autogenDir = autogenComponentModulesDir lbi clbi
This has been fixed in the writeFileAtomic :: FilePath -> LBS.ByteString -> IO ()
writeFileAtomic targetPath content = do
let targetFile = takeFileName targetPath
tmpDir <- getTemporaryDirectory
Exception.bracketOnError
(openBinaryTempFileWithDefaultPermissions tmpDir $ targetFile <.> "tmp")
(\(tmpPath, handle) -> hClose handle >> removeFile tmpPath)
( \(tmpPath, handle) -> do
LBS.hPut handle content
hClose handle
renameFileWithRetry tmpPath targetPath
)as the temporary file is opening in a short path location and I don't think Stack can do anything about this Cabal bug. |
|
@philippedev101, the other integration tests use Hpack to describe packages and, so, I conformed the added integration tests. |
`backpack-cross-package-transitive` is renamed `backpack-x-pkg-transitive`, which is a sufficient shortening to avoid a problem with long paths on Windows. That test is re-enabled on Windows. Other tests are renamed to conform to the `x-pkg` abbreviation. The test descriptions where the `x-pkg` abbreviation is used state that support for 'cross-package Backpack' is being tested.
|
@philippedev101, I have released Stack 3.11.1. Unrelated to this pull request, the Docker-based integration tests will fail until the Docker image is updated. |
2 participants
Addresses #2540.
This PR adds support for GHC's Backpack module system, the feature request that has been open since August 2016. After this change, Stack can build projects that use
signatures,mixins, and cross-package mixin linking, which previously only worked undercabal-install.Background
Backpack lets you write a library against an abstract interface (a signature) and have the consumer decide which concrete implementation to plug in. The compiler recompiles the library for each implementation, so there's no runtime overhead. When the signature and the implementation live in the same package (using sub-libraries), this is "private Backpack" and has always worked in Stack. The hard part is cross-package Backpack, where the signature lives in one package and the implementation in another. This requires the build tool to create extra instantiation build steps, which Stack didn't do before.
What changed
The work breaks down into three layers:
Per-component build plan. The build plan is now keyed by
ComponentKey(package name + component) instead of just package name. This was a prerequisite: Backpack instantiation tasks need their own entries in the plan alongside the regular library task for the same package. This also lets Stack detect intra-package sub-library dependencies (the Backpack pattern) and avoid splitting those packages into independent component tasks, which would break the build order that Cabal expects.Cross-package instantiation. When a consumer package uses
mixinsto depend on an indefinite package, Stack now scans the consumer's dependencies, resolves which modules fill which signatures, and creates CInst (component instantiation) tasks. Each CInst task runsSetup configure --instantiate-with=Sig=impl-pkg:Modulefollowed bySetup buildin its owninst-<hash>build directory. The hash is derived from the signature-to-implementation mapping, so different instantiations of the same package get different build artifacts.This handles the full range of Backpack patterns: default renaming (name identity), explicit
ModuleRenaming,HidingRenaming(propagating unfilled holes), multiple instantiations of the same indefinite package with different implementations, transitive chains where an indefinite package depends on another indefinite package, sub-library signatures and implementations, and indefinite packages from Hackage or snapshots (not just local packages). CInst tasks also get their own config cache entries, precompiled cache support, and haddock generation.Documentation and changelog. A new topic page at
doc/topics/backpack.mdintroduces what Backpack is, walks through its features (signatures, mixin linking, renaming, multiple instantiations, sub-libraries, reexported modules, Template Haskell restrictions), and then explains how Stack supports it, including what the build output looks like and what the limitations are. The page is linked frommkdocs.ymlunder Topics, and cross-referenced from the package description tutorial (mentioning thesignaturesfield) and the multi-package projects tutorial (mentioning Backpack as a use case for multi-package setups). A major changes entry has been added toChangeLog.mdunder the unreleased section.Testing
103 unit tests in
ConstructPlanSpeccover the instantiation logic: signature resolution, renaming, hiding, deduplication, transitive chains, multiple instantiations, sub-library mixins, ADRFound (installed) packages, config cache round-trips, and various warning/error paths.8 integration tests build real multi-package Backpack projects end-to-end: private Backpack, sub-library dependencies, cross-package with default renaming, explicit renaming, sub-library mixins, transitive chains, and multiple instantiations.
The full existing test suite (814 tests) continues to pass. The two pre-existing
Stack.Config/loadConfigfailures are environmental (TLS certificates) and unrelated.What's not covered
requires hidingthat actually hides signatures does not create a partial instantiation. Cabal requires a closing substitution, so the hidden signatures propagate as holes to the consumer. Template Haskell in indefinite packages doesn't work, but that's a GHC limitation that affectscabal-installequally.