Skip to content

cabal-install: dynExe+profExe requires prof+dyn #10994

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

Merged
merged 1 commit into from
Jun 25, 2025

Conversation

sheaf
Copy link
Collaborator

@sheaf sheaf commented Jun 17, 2025

This commit updates the logic in cabal-install's elaborateInstallPlan function to ensure that we don't try to build a profiled dynamic executable if the compiler doesn't support the profiling dynamic way.

This brings the logic in cabal-install in sync with the Cabal configureProfiling function, which sets withDynExe to false if the user wants a profiled executable but prof+dyn is not supported by the compiler.


Template B: This PR does not modify behaviour or interface

E.g. the PR only touches documentation or tests, does refactorings, etc.

Include the following checklist in your PR:

@sheaf
Copy link
Collaborator Author

sheaf commented Jun 18, 2025

I haven't managed to write a test for this, because the way things currently work, we compute e.g. whether to build a prof+dyn executable in cabal-install, but then when we invoke ./Setup configure we re-compute this information. I tried writing a test that was based off printPlan but that has the same problem, as that calls the computeEffectiveProfiling function from the Cabal library, thus re-computing that field as well.

Thus, the underlying issue only manifests if you directly call the configure function in Cabal from within cabal-install, like I'm doing in #9871. In that case, the problem is detected by the existing ProfShared test (which fails without this change).

I'm not including a changelog entry either as this should be invisible to users.

@sheaf sheaf marked this pull request as ready for review June 18, 2025 10:47
@sheaf sheaf force-pushed the wip/profDyn branch 3 times, most recently from e238448 to 44e077a Compare June 18, 2025 10:52
@sheaf sheaf added the merge me Tell Mergify Bot to merge label Jun 19, 2025
@mergify mergify bot added ready and waiting Mergify is waiting out the cooldown period merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days labels Jun 19, 2025
Copy link
Contributor

mergify bot commented Jun 21, 2025

This pull request has been removed from the queue for the following reason: pull request branch update failed.

The pull request can't be updated.

You should update or rebase your pull request manually. If you do, this pull request will automatically be requeued once the queue conditions match again.
If you think this was a flaky issue, you can requeue the pull request, without updating it, by posting a @mergifyio requeue comment.

Copy link
Contributor

mergify bot commented Jun 24, 2025

This pull request has been removed from the queue for the following reason: pull request branch update failed.

The pull request can't be updated.

You should update or rebase your pull request manually. If you do, this pull request will automatically be requeued once the queue conditions match again.
If you think this was a flaky issue, you can requeue the pull request, without updating it, by posting a @mergifyio requeue comment.

@Mikolaj
Copy link
Member

Mikolaj commented Jun 24, 2025

Let me try first to rebase with mergify rebase.

@Mikolaj
Copy link
Member

Mikolaj commented Jun 24, 2025

@mergify rebase

Copy link
Contributor

mergify bot commented Jun 24, 2025

rebase

✅ Branch has been successfully rebased

Copy link
Contributor

mergify bot commented Jun 25, 2025

This pull request has been removed from the queue for the following reason: pull request branch update failed.

The pull request can't be updated.

You should update or rebase your pull request manually. If you do, this pull request will automatically be requeued once the queue conditions match again.
If you think this was a flaky issue, you can requeue the pull request, without updating it, by posting a @mergifyio requeue comment.

@ulysses4ever
Copy link
Collaborator

same result as I had

@Mikolaj
Copy link
Member

Mikolaj commented Jun 25, 2025

I think we have no choice but to change the merge label to merge_no_rebase.

@Mikolaj
Copy link
Member

Mikolaj commented Jun 25, 2025

And while we ponder that, let me rebase with github, for science.

@Mikolaj
Copy link
Member

Mikolaj commented Jun 25, 2025

Oh dear, I merged instead of rebasing. My bad.

@Mikolaj
Copy link
Member

Mikolaj commented Jun 25, 2025

@mergify rebase

This commit updates the logic in cabal-install's 'elaborateInstallPlan'
function to ensure that we don't try to build a profiled dynamic
executable if the compiler doesn't support the profiling dynamic way.

This brings the logic in cabal-install in sync with the Cabal
'configureProfiling' function, which sets 'withDynExe' to false if the
user wants a profiled executable but prof+dyn is not supported by the
compiler.
Copy link
Contributor

mergify bot commented Jun 25, 2025

rebase

✅ Branch has been successfully rebased

@Mikolaj
Copy link
Member

Mikolaj commented Jun 25, 2025

Phew, mergify rebase killed the merge commit.

@mergify mergify bot merged commit f647502 into haskell:master Jun 25, 2025
54 checks passed
@ulysses4ever
Copy link
Collaborator

In my experience, no matter how you rebase (with the mergify command, with GitHub UI, or locally), the catch is to have the master branch untouched by the time CI finishes after rebasing. If this is the case, the bot will auto-merge. If the master branch has advanced, you're back exactly to the same point where you were before rebasing.

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 ready and waiting Mergify is waiting out the cooldown period
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants