[Fix #11850] Build dependencies of build tool on disk too - #12084
Conversation
|
Thank you for the PR. Please mention the issue it fixes in the PR descriptions and please mark the PR with review_needed label once ready. |
|
Oh, and please add the changelog snippet. |
ef2430a to
d9570c7
Compare
|
@Mikolaj should be all there now, let me know if the changelog needs editing. |
Mikolaj
left a comment
There was a problem hiding this comment.
The changelog is perfect, thank you.
What can go wrong? Does this impair performance in some scenarios? Can the location that is used for the build be read-only? Anything else?
Like the subdirectory for the unit under
|
|
This PR would be great to get merged into cabal asap as it will considerably improve HLS behaviour for loading multiple components at once. There is still potential for more improvements, as suggested in #11850, but this is a good start. |
|
I confirmed manually that this patch allows us to run |
Mikolaj
left a comment
There was a problem hiding this comment.
Thank you. Please set the merge_me label.
Merge Queue Status
This pull request spent 2 hours 7 minutes 8 seconds in the queue, including 1 hour 56 minutes 31 seconds running CI. Required conditions to merge
|
They were not. @Saizan could you please add them? |
|
Full disclosure: I marked the AI comments "Resolved" to unblock Mergify, but maybe @Saizan you wanted to answer them? No pressure for a bot-generated text, obviously. Or did you read them and the don't make sense? |
@ffaf1 done, for some reason I thought they were needed only for breaking cli changes. |
@Mikolaj the comments seem sensible actually, haven't had the time yet and they are not major problems I think. I guess I can open a follow-up PR? |
|
@Saizan Yesterday this PR popped up in cabal-dev meeting. We tried to brainstorm what could go wrong with it. A question: what happens if we have no write access to disk? What would happen then? (Apologies for asking questions after the PR was merged, I am trying to decide whether this can go into |
This PR fixes #11850 by taking the simple approach of ensuring the build tool can be properly built.
Checklist:
significance: significantin the changelog file.QA Notes
Calling
cabal repl --enable-multi-repl exe:bar foowhen foo has abuild-tool-dependsonexe:barfrom the same project should buildbaras an executable first, and also include it in the interactive session.