Correctly provision build tools in all situations #9912
Conversation
sheaf
force-pushed
the
wip/program-db-paths
branch
4 times, most recently
from
eb044a1 to
e2e4a3a
Compare
Mikolaj
added
the
re: build-tool
alt-romes
force-pushed
the
wip/program-db-paths
branch
3 times, most recently
from
a5becf7 to
947eea5
Compare
sheaf
force-pushed
the
wip/program-db-paths
branch
4 times, most recently
from
26ddcb5 to
88e5941
Compare
|
This PR is now ready for review and should be passing CI. @jasagredo, might you be interested in taking a look? Edit: CI failing for silly Windows reasons even though the test passes locally on my Windows machine. Investigating. |
sheaf
force-pushed
the
wip/program-db-paths
branch
from
88e5941 to
1e32da6
Compare
sheaf
force-pushed
the
wip/program-db-paths
branch
from
1e32da6 to
cebcca4
Compare
|
This is quite a key improvement to Cabal's handling of build-tools (the tests serve as good examples of things that were previously not possible because of incorrect PATH handling for build-tools). |
There was a problem hiding this comment.
I don't feel comfortable approving because I do not have time to fully analyze the impact of this, or whether it is cohesive with the rest of the code.
A top-to-bottom read of the PR looks reasonable, steps look documented, and the tests pass and seem thorough, so I think it is fine.
|
Thanks @jasagredo. Would anyone else like to review this PR? Perhaps @andreabedini or @gbaz ? |
|
Let's not let it bitrot. Last chance for extra reviews. |
mergify
bot
added
the
merge delay passed
This commit ensures the test-suite driver also provides the Cabal-hooks package, on top of Cabal and Cabal-syntax.
This patch ensures that we correctly provision executables declared
in the build-tool-depends fields in all circumstances:
- whether the build tool is external (from another package) or
internal (declared in the current package)
- whether the build tool is used at compile time (e.g. in a pre-build
rule or in a Template Haskell splice) or at run time (e.g. when
running a test-suite, benchmark or executable).
Note that correctly provisioning a build tool requires two pieces of
information:
- making it available in PATH,
- ensuring it has the correct environment variables overrides;
in particular, the build tool needs to be able to find its own
data directory.
The test case BuildToolPaths checks all of these situations are handled
correctly.
Mikolaj
force-pushed
the
wip/program-db-paths
branch
from
cebcca4 to
ee11ac6
Compare
|
This looks like a correctness fix, so, formally speaking, should be backported. But I fear that it can bring regressions. Should we shelve it till 3.14? |
|
I think the risk/reward in this case is not in favour of backporting it: it fixes problems, but they don't seem particularly grave nor commonly occurring nor recent regressions. OTOH, this introduces significant changes that may result in expected interactions with other changes in the release, etc. And it didn't make it to the pre-release, so it wasn't tested enough in the wild. I'd vote for 3.14. |
This PR ensures that we correctly provision executables declared in the build-tool-depends fields in all circumstances:
or at run time (e.g. when running a test-suite, benchmark or executable).
Note that correctly provisioning a build tool requires two pieces of information:
in particular, the build tool needs to be able to find its own data directory.
The test case BuildToolPaths checks all of these situations are handled correctly.