Revert "Fix project-local build flags being ignored."

[Mikolaj]

This reverts commit b547ead
from #8623.
Unexpected side-effect has been found (#8719 and #8721),
so these code improvements have to be done differently.
The other commit in the reverted PR is a test and it's retained
and marked as expected broken.

[gbaz]

As per my comment (#8623 (comment)) I think this is the wrong approach. We should instead just make the change in #8723 . Most of the reverted commit is not about the misleading title (i.e. changing cabal install to handle build flags). Changing that single line alone should be enough to fix #8719.

Instead, most of the reverted commit is actually doing the unrelated but important thing of teaching cabal to use the same "way" (i.e. dynamic or static) to build the setup script as it uses to build the package itself. That's perfectly good code and should stay! Its too bad all these changes came bundled together, so we couldn't approach them one-by-one and instead we have such confusion...

[Mikolaj]

@gbaz: thanks. As long as we don't understand why it breaks cabal install --lib according to the author, I'm not particularly comfortable including it. Let me ask Oleg.

[Mikolaj]

BTW, that means you don't agree with the latter part of Oleg's "The proper fix is #8723, but it essentially reverts what this patch did."?

[Mikolaj]

And by "flipping the test" you don't mean the test that I retained and marked as broken, but the test change that has been done in the main commit in order to prevent the test failing with the problematic change that we want to revert?

[gbaz]

sorry, "flipping the test" was bad wording. I simply meant "removing the line LocalTarballPackage _ -> Just (packageId pkg)" (i.e. changing the outcome of the isInLocal function on local tarball packages from "Just" to "Nothing").

And yes, I don't agree that "mainly reverts what this patch did" because if you examine the patch being reverted, it actually does two extremely unrelated things, which is the source of all this confusion!

[Mikolaj]

@gbaz: actually the test that was changed in order to accommodate the PR changes may rather be T5782Diamond (or there were many? or am I mixing up PRs?). It's related to another part of this PR that we've got feedback about and that has an aborted fix here #8722. That at least indicates these changes are controversial, but the author did not engage @phadej about the feedback, so I'd rather give the author more time for that, while the PR waits back in the queue. What do you think about this particular item, @gbaz?

[phadej]

.e. changing the outcome of the isInLocal function o

@gbaz

Not enough. Also change to style2str is third (though coupled thing) which should be undone.

The cleanest way forward is to revert whole commit and only add bits one at the time and have tests for them.

You really shouldn't accept features without tests.

[Mikolaj]

Given that original PR is very valuable, fixing an old-standing bug, but @bairyn seems too busy ATM to revise it, we've agreed on today's dev meeting to revert the PR and, time permitting, recover the most important contribution of the original PR, unless the author (@bairyn) beats us to it. Therefore, please kindly accept the reversion PR. In the interest of time, because cabal 3.10 is night at hand, I will merge with one review, if necessary.

[bairyn]

(Update in issue #8719 .)

[Mikolaj]

@bairyn: thank you

@gbaz: would you mind ticking the box? Edit: never mind.

[Mikolaj]

@mergify backport 3.10

[mergify]

backport 3.10

✅ Backports have been created

• #8768 Revert "Fix project-local build flags being ignored." (backport #8744) has been created for branch 3.10