Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow per-component builds with coverage enabled #9464

Merged
merged 2 commits into from
Dec 18, 2023

Conversation

alt-romes
Copy link
Collaborator

@alt-romes alt-romes commented Nov 22, 2023

This commits re-enables per-component builds when coverage checking is
enabled. This restriction was previously added in #5004 to fix #4798.

Therefore the restriction to treat testsuites per-package
(legacy-fallback) is no longer needed.

We went further and fixed coverage for internal sublibraries, packages
with backpack (but without generating coverage information for
indefinite and instantiated units -- it is not clear what it would mean
for HPC to support this), and coverage for multi-package projects.

This (second) commit re-designs the mechanism by which we make the .mix files of
libraries available to produce the Haskell Program Coverage report after
running testsuites.

The idea, for the Cabal library, is:

* Cabal builds libraries with -fhpc, and store the hpc artifacts in
  build </> `extraCompilationArtifacts`
* At Cabal install time, `extraCompilationArtifacts` is copied into the
  package database
* At Cabal configure time, we both
    - receive as --coverage-for flags unit-ids of library components
      from the same package (ultimately, when #9493 is resolved, we will
      receive unit ids of libraries in other packages in the same
      project too),
    - and, when configuring a whole package instead of just a testsuite
      component, we determine the unit-ids of libraries in the package
  these unit-ids are written into `configCoverageFor` in `ConfigFlags`
* At Cabal test time, for each library to cover (stored in
  `configCoverageFor`), we look in the package database for the hpc
  dirs, which we eventually pass along to the `hpc markup` call as
  `--hpcdir` flags

As for cabal-install:

* After a plan has been elaborated, we select the packages which can be
  covered and pass them to Cabal's ./Setup configure as
  --coverage-for=<unit-id> flags.
    - Notably, valid libraries are non-indefinite and
      non-instantiations, since HPC does not support backpack.
    - Furthermore, we only include libraries in the same package as the
      component being configured, despite possibly there being
      more library components in other packages of the same project.
      When #9493 is resolved, we could lift this restriction and pass
      all libraries local to the package as --coverage-for. See
      `determineCoverageFor` and `shouldCoverPkg` in Distribution.Client.ProjectPlanning.

Fixes #6440 (internal libs coverage), #6397 (backpack breaks coverage),
doesn't yet fix #8609 (multi-package coverage report) which is tracked
in #9493, and fixes in a new way the previously fixed #4798, #5213.

QA Notes

Running cabal test cabal-install --enable-coverage in the root of the
cabal project should succeed and generate a coverage report for cabal-install.


Template Α: This PR modifies cabal behaviour

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.
  • Manual QA notes have been included.
  • Tests have been added. (Ask for help if you don’t know how to write them! Ask for an exemption if tests are too complex for too little coverage!)

@alt-romes
Copy link
Collaborator Author

There's a test missing for #6440, which I'm working on, and the test for #4798 is failing for some reason regarding the setup required to run the test as part of the cabal testsuite.

@alt-romes alt-romes force-pushed the wip/romes/4798 branch 2 times, most recently from aef4e35 to 7090763 Compare November 22, 2023 17:49
@alt-romes
Copy link
Collaborator Author

This is now good to go.

@alt-romes
Copy link
Collaborator Author

alt-romes commented Nov 22, 2023

Could you help me understand what went wrong in the CI pipeline?

@ffaf1
Copy link
Collaborator

ffaf1 commented Nov 22, 2023

Well, the test you introduced is failing. I assume it works fine on your machine?
The offending dieWithException is here

dieIfNotHaddockFailure :: Verbosity -> String -> IO ()
dieIfNotHaddockFailure verb str
| currentCommand == HaddockCommand = dieWithException verb $ DieIfNotHaddockFailureException str
| all isHaddockFailure failuresClassification = warn verb str
| otherwise = dieWithException verb $ DieIfNotHaddockFailureException str
where
isHaddockFailure
(_, ShowBuildSummaryOnly (HaddocksFailed _)) = True
isHaddockFailure
(_, ShowBuildSummaryAndLog (HaddocksFailed _) _) = True
isHaddockFailure
_ = False

@alt-romes
Copy link
Collaborator Author

This patch should fix #6397 as well (see #6397 (comment)) as well (to the best of our extent AFAICT)

Not done yet! I'll finish it tomorrow.

@alt-romes alt-romes force-pushed the wip/romes/4798 branch 4 times, most recently from c9fcfaa to 2cd3e6f Compare November 30, 2023 11:35
fgaz
fgaz previously requested changes Nov 30, 2023
Copy link
Member

@fgaz fgaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like you rewrote some older commits in the last force push

Cabal/src/Distribution/Simple/Hpc.hs Outdated Show resolved Hide resolved
cabal-install/src/Distribution/Client/ProjectPlanning.hs Outdated Show resolved Hide resolved
cabal-install/src/Distribution/Client/Setup.hs Outdated Show resolved Hide resolved
Comment on lines 1110 to 1111
, Cabal.testCoverageLibsModules = NoFlag
, Cabal.testCoverageDistPrefs = NoFlag
Copy link
Member

@fgaz fgaz Dec 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The flags have to be removed from flags_3_11_0. See the comments above for an explanation, and see filterConfigureFlags for how to add a new flags_ (they build on each other).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@alt-romes alt-romes force-pushed the wip/romes/4798 branch 3 times, most recently from fb411e9 to 5d29491 Compare December 4, 2023 11:44
Copy link
Collaborator

@andreabedini andreabedini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good work 👍

I will add the "merge me" label since you say it's ready.

@andreabedini andreabedini added merge me Tell Mergify Bot to merge and removed attention: needs-review labels Dec 14, 2023
@alt-romes
Copy link
Collaborator Author

Good work 👍

I will add the "merge me" label since you say it's ready.

Thanks Andrea, that would be great

@mergify mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Dec 17, 2023
This commits re-enables per-component builds when coverage checking is
enabled. This restriction was previously added in haskell#5004 to fix haskell#4798.

- haskell#4798 was subsequently fixed "again" with the fix for haskell#5213, in haskell#7493 by
fixing the paths of the testsuite `.mix` files to the same location as
that of the main library component.

Therefore the restriction to treat testsuites per-package
(legacy-fallback) is no longer needed.

We went further and fixed coverage for internal sublibraries, packages
with backpack (but without generating coverage information for
indefinite and instantiated units -- it is not clear what it would mean
for HPC to support this), and coverage for multi-package projects.

1. We allow hpc in per-component builds

2. To generate hpc files in the appropriate component directories in the
distribution tree, we remove the hack from haskell#7493 and instead determine
the `.mix` directories that are included in the call to `hpc markup` by
passing the list of components in the project from the cabal-install
invocation of test.
We also drop an unnecessary directory in the hpc file hierarchy.

3. To account for internal (non-backpack) libraries, we include the mix
   dirs and modules of all (non-indefinite and non-instantiations)
   libraries in the project

   Indefinite libraries and instantiations are ignored as it is not
   obvious what it means for HPC to support backpack, e.g. covering a
   library function that two different instantiations

4. We now only reject coverage if there are no libraries at all in the
   project, rather than if there are no libraries in the package.

This allows us to drop the coverage masking logic in
cabal.project.coverage while still having coverage of cabal-install
(i.e. cabal test --enable-coverage cabal-install now works without the
workaround)

Even though we allow multi-package project coverage, we still cover each
package independently -- the tix files resulting from all packages are
not combined for the time being.

Multi-package project coverage is fixed in Cabal, however, the
paths to the source files listed in the `.mix` files will be incorrect
because package sources will no longer be in the root of the project
tree, but rather under the subdir with the package. We add an error for
multi-package projects when coverage is enabled, and track lifting this
error in haskell#9493.

Includes tests for haskell#6440, haskell#6397, haskell#8609, and haskell#4798 (the test for haskell#5213 already exists)

Fixes haskell#6440 (internal libs coverage), haskell#6397 (backpack breaks coverage)
, doesn't yet fix haskell#8609 (multi-package coverage report) and fixes in a new way the
previously fixed haskell#4798, haskell#5213.
This commit re-designs the mechanism by which we make the .mix files of
libraries available to produce the Haskell Program Coverage report after
running testsuites.

The idea, for the Cabal library, is:

* Cabal builds libraries with -fhpc, and store the hpc artifacts in
  build </> `extraCompilationArtifacts`
* At Cabal install time, `extraCompilationArtifacts` is copied into the
  package database
* At Cabal configure time, we both
    - receive as --coverage-for flags unit-ids of library components
      from the same package (ultimately, when haskell#9493 is resolved, we will
      receive unit ids of libraries in other packages in the same
      project too),
    - and, when configuring a whole package instead of just a testsuite
      component, we determine the unit-ids of libraries in the package
  these unit-ids are written into `configCoverageFor` in `ConfigFlags`
* At Cabal test time, for each library to cover (stored in
  `configCoverageFor`), we look in the package database for the hpc
  dirs, which we eventually pass along to the `hpc markup` call as
  `--hpcdir` flags

As for cabal-install:

* After a plan has been elaborated, we select the packages which can be
  covered and pass them to Cabal's ./Setup configure as
  --coverage-for=<unit-id> flags.
    - Notably, valid libraries are non-indefinite and
      non-instantiations, since HPC does not support backpack.
    - Furthermore, we only include libraries in the same package as the
      component being configured, despite possibly there being
      more library components in other packages of the same project.
      When haskell#9493 is resolved, we could lift this restriction and pass
      all libraries local to the package as --coverage-for. See
      `determineCoverageFor` and `shouldCoverPkg` in Distribution.Client.ProjectPlanning.

Detail:
    We no longer pass the path to the testsuite's mix dirs to `hpc
    markup` because we only ever include modules in libraries, which
    means they were previously unused.

Fixes haskell#6440 (internal libs coverage), haskell#6397 (backpack breaks coverage),
doesn't yet fix haskell#8609 (multi-package coverage report) which is tracked
in haskell#9493, and fixes in a new way the previously fixed haskell#4798, haskell#5213.
@mergify mergify bot merged commit feaa338 into haskell:master Dec 18, 2023
50 checks passed
@alt-romes
Copy link
Collaborator Author

Thanks all!

@fgaz
Copy link
Member

fgaz commented Dec 19, 2023

@alt-romes could you complete the checklist I added back to OP in a follow-up PR? There were a few missing items.

@alt-romes
Copy link
Collaborator Author

@alt-romes could you complete the checklist I added back to OP in a follow-up PR? There were a few missing items.

I surely can, however, I don't understand what you mean by the "checklist I added back to OP". Which missing items are there?

@fgaz
Copy link
Member

fgaz commented Dec 19, 2023

I surely can, however, I don't understand what you mean by the "checklist I added back to OP". Which missing items are there?

There is a list that is part of the pull request template. It was removed in your last edit of the PR escription but it was not complete, so I restored it in the last edit. It's currently missing documentation, changelog, and QA notes.

@alt-romes
Copy link
Collaborator Author

@fgaz Yes, apologies. I'll take care of that in a separate PR.

@andreabedini
Copy link
Collaborator

@fgaz I am sorry for overlooking those details. I would not mind if we used "request changes" when changes are needed. It would have prevented my mistake.

@alt-romes
Copy link
Collaborator Author

alt-romes commented Dec 19, 2023

@andreabedini what does it mean exactly "documentation has been updated", i.e. which documentation?

@alt-romes alt-romes self-assigned this Dec 19, 2023
alt-romes added a commit to alt-romes/cabal that referenced this pull request Dec 19, 2023
Additionally, a Manual QA note for the same ticket:

Running `cabal test cabal-install --enable-coverage` in the root of the
cabal project should succeed and generate a coverage report for
`cabal-install`.
alt-romes added a commit to alt-romes/cabal that referenced this pull request Dec 19, 2023
Additionally, a Manual QA note for the same ticket:

Running `cabal test cabal-install --enable-coverage` in the root of the
cabal project should succeed and generate a coverage report for
`cabal-install`.
@andreabedini
Copy link
Collaborator

@alt-romes I think @fgaz refers to the "The documentation has been updated, if necessary" item in the checklist. I this case I would understand it as checking the documentation for references to code coverage and see if they have to be updated.

Mikolaj pushed a commit to alt-romes/cabal that referenced this pull request Jan 5, 2024
Additionally, a Manual QA note for the same ticket:

Running `cabal test cabal-install --enable-coverage` in the root of the
cabal project should succeed and generate a coverage report for
`cabal-install`.
mergify bot added a commit that referenced this pull request Jan 5, 2024
Update changelog for per-component with coverage #9464
erikd pushed a commit to erikd/cabal that referenced this pull request Apr 22, 2024
Additionally, a Manual QA note for the same ticket:

Running `cabal test cabal-install --enable-coverage` in the root of the
cabal project should succeed and generate a coverage report for
`cabal-install`.
@jasagredo
Copy link
Collaborator

jasagredo commented Aug 6, 2024

Maybe there is a good reason for this, but the lines:

-- Distribution/Simple/Configure.hs
          -- For whole-package configure, we determine the
          -- extraCoverageFor of the main lib and sub libs here.
          extraCoverageUnitIds = case enabled of
            -- Whole package configure, add package libs
            ComponentRequestedSpec{} -> mapMaybe mbCompUnitId buildComponents
            -- Component configure, no need to do anything
            OneComponentRequestedSpec{} -> []

(which I traced back as being added in this PR), are causing this error: #10046

I traced the values that are produced here:

-- Distribution/Simple/Test.hs
  let coverageFor =
        nub $
          fromFlagOrDefault [] (configCoverageFor (configFlags lbi))
            <> extraCoverageFor lbi
  ipkginfos <- getInstalledPackagesById verbosity lbi MissingCoveredInstalledLibrary (traceShowId coverageFor)

and it turns out that it adds the libraries as needing coverage, but if the test-suite doesn't depend on all libs and sublibs they might be built before those are built.

In the example in that issue, if I trace that value I see:

[1 of 1] Compiling Main             ( test\Main.hs, C:\Users\Javier\aa\dist-newstyle\build\x86_64-windows\ghc-9.8.2\aa-0.1.0.0\t\aa-test\build\aa-test\aa-test-tmp\Main.o )
Installing   uri-encode-1.5.0.7 (lib)
Installing   transformers-base-0.4.6 (lib)
Installing   peano-0.1.0.2 (lib)
Installing   gitrev-1.3.1 (lib)
[2 of 2] Linking C:\\Users\\Javier\\aa\\dist-newstyle\\build\\x86_64-windows\\ghc-9.8.2\\aa-0.1.0.0\\t\\aa-test\\build\\aa-test\\aa-test.exe
Installing   boxes-0.1.5 (lib)
Installing   equivalence-0.4.1 (lib)
Warning: this is a debug build of cabal-install with assertions enabled.
Installing   blaze-markup-0.8.3.0 (lib)
[UnitId "aa-0.1.0.0-inplace"]
Error: [Cabal-9341]
Failed to find the installed unit 'aa-0.1.0.0-inplace' in package database stack.

So running the aa-test test-suite which does not depend on the aa lib actually is requesting that aa is built with coverage?

cc @alt-romes

@jasagredo
Copy link
Collaborator

Another issue, probably being caused by the same underlying cause. I did cabal test all and one of the test-suites failed exactly for the reason above:

$ cabal test all
...
Error: [Cabal-9341]
Failed to find the installed unit 'ouroboros-consensus-protocol-0.9.0.1-inplace-unstable-protocol-testlib' in package database stack.
...
Error: [Cabal-7125]
Tests failed for test:protocol-test from ouroboros-consensus-protocol-0.9.0.1.

Now if I do cabal test test:protocol-test to re-run it, hopefully now with everything built, it turns out that it rebuilds a big chunk of the chain of dependencies, perhaps because it is fiddling with coverage enabled/disabled?

Error: [Cabal-7125]
Tests failed for test:protocol-test from ouroboros-consensus-protocol-0.9.0.1.


ouroboros-consensus on  utxo-hd-9.0 [$!] via λ 9.8.2 took 38m42s
❯ cabal test test:protocol-test
Build profile: -w ghc-9.8.2 -O1
In order, the following will be built (use -v for more details):
 - cardano-crypto-class-2.1.5.0 (lib) (requires build)
 - plutus-core-1.30.0.0 (lib) (requires build)
 - ouroboros-consensus-0.20.0.0 (lib) (configuration changed)
 - cardano-crypto-praos-2.1.2.0 (lib) (requires build)
 - plutus-core-1.30.0.0 (lib:plutus-ir) (requires build)
 - ouroboros-consensus-0.20.0.0 (lib:unstable-consensus-testlib) (configuration changed)
 - cardano-crypto-tests-2.1.2.0 (lib) (requires build)
 - plutus-tx-1.30.0.0 (lib) (requires build)
 - plutus-ledger-api-1.30.0.0 (lib) (requires build)
 - cardano-ledger-binary-1.3.3.0 (lib) (requires build)
 - cardano-ledger-binary-1.3.3.0 (lib:testlib) (requires build)
 - cardano-data-1.2.3.0 (lib) (requires build)
 - cardano-crypto-wrapper-1.5.1.2 (lib) (requires build)
 - set-algebra-1.1.0.3 (lib) (requires build)
 - cardano-ledger-byron-1.0.1.0 (lib) (requires build)
 - cardano-ledger-core-1.13.2.0 (lib) (requires build)
 - cardano-ledger-shelley-1.12.2.0 (lib) (requires build)
 - cardano-ledger-allegra-1.5.0.0 (lib) (requires build)
 - cardano-ledger-mary-1.6.1.0 (lib) (requires build)
 - cardano-ledger-alonzo-1.10.0.0 (lib) (requires build)
 - cardano-ledger-babbage-1.8.2.0 (lib) (requires build)
 - cardano-ledger-conway-1.16.0.0 (lib) (requires build)
 - cardano-protocol-tpraos-1.2.0.1 (lib) (requires build)
 - ouroboros-consensus-protocol-0.9.0.1 (lib) (configuration changed)
 - ouroboros-consensus-protocol-0.9.0.1 (test:protocol-test) (configuration changed)
...

@jasagredo
Copy link
Collaborator

Just as a clarification, my system is a Windows system, but this has been observed in my co-workers machine today which are all Linux systems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days merge me Tell Mergify Bot to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Program coverage crashes in cabal new-test
8 participants