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

hpc: Don't cover non-dependency libraries #10250

Merged
merged 1 commit into from
Aug 16, 2024

Conversation

alt-romes
Copy link
Collaborator

@alt-romes alt-romes commented Aug 7, 2024

We were passing the flag --coverage-for for every library in the package when building the testsuites.

However, if a particular testsuite doesn't depend on one of the packages passed with --coverage-for we would crash.

Since we look for the unit ids of all packages given in --coverage-for in the package database (to find hpc artifacts), if the testsuite builds before the library this lookup would fail.

The fix is easy: don't pass --coverage-for= to if doesn't depend on . If is an indirect dependency of it'll no longer be covered by HPC, but this is a weird behaviour that I doubt anyone relies on.

Fixes #10046

Please read Github PR Conventions and then fill in one of these two templates.

Template Α: This PR modifies behaviour or interface

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 alt-romes force-pushed the wip/romes/10046 branch 3 times, most recently from bbef74e to ab7b5e8 Compare August 7, 2024 16:48
@@ -1,4 +1,5 @@
{-# LANGUAGE DeriveFunctor, DeriveFoldable, DeriveTraversable #-}
{-# OPTIONS_GHC -ddump-to-file -ddump-simpl -ddump-stg-final #-}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks like a leftover?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes.

@ulysses4ever
Copy link
Collaborator

Good to see progress on this regression, thanks!

Template B: This PR does not modify behaviour or interface

I think it does modify behavior, because cabal will crash less after the patch. Remember, template B is mostly just for documentation, tests, CI.

@alt-romes alt-romes force-pushed the wip/romes/10046 branch 2 times, most recently from 812be41 to b8c16c7 Compare August 7, 2024 16:54
@alt-romes
Copy link
Collaborator Author

alt-romes commented Aug 7, 2024

Good to see progress on this regression, thanks!

Template B: This PR does not modify behaviour or interface

I think it does modify behavior, because cabal will crash less after the patch. Remember, template B is mostly just for documentation, tests, CI.

Fine, updated and pushed a changelog.

Copy link
Collaborator

@jasagredo jasagredo left a comment

Choose a reason for hiding this comment

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

The changes look good! Thanks for the super quick fix 🙏

Copy link
Collaborator

@ulysses4ever ulysses4ever left a comment

Choose a reason for hiding this comment

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

I'm not the best person to review it, but this is a pretty significant regression, so, in the interest of delivering 3.14 sooner, let's fire it 🔥

@mergify mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Aug 10, 2024
@alt-romes
Copy link
Collaborator Author

@mergify rebase

Copy link
Contributor

mergify bot commented Aug 16, 2024

rebase

❌ Unable to rebase: user alt-romes is unknown.

Please make sure alt-romes has logged in Mergify dashboard.

We were passing the flag --coverage-for for every library in the package
when building the testsuites.

However, if a particular testsuite doesn't depend on one of the packages
passed with --coverage-for we would crash.

Since we look for the unit ids of all packages given in --coverage-for in the
package database (to find hpc artifacts), if the testsuite builds before
the library this lookup would fail.

The fix is easy: don't pass --coverage-for=<lib> to <testsuite> if
<testsuite> doesn't depend on <lib>. If <lib> is an indirect dependency
of <testsuite> it'll no longer be covered by HPC, but this is a weird
behaviour that I doubt anyone relies on.

Fixes haskell#10046
@mergify mergify bot merged commit 969343a into haskell:master Aug 16, 2024
50 checks passed
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.

[3.12.0.0] Failed to find the installed unit
4 participants