haskell
Add check on project root file and fail if broken link
This PR modifies behaviour or interface**
Include the following checklist in your PR:
- [x] Patches conform to the coding conventions.
- [x] Any changes that could be relevant to users have been recorded in the changelog.
- [x] The documentation has been updated, if necessary.
- [x] Manual QA notes have been included.
- [x] Tests have been added.
QA Notes
Build patched version
cabal build cabal-install
alias patchedCabal=$(cabal list-bin cabal-install)
Reproduce
Not patched version
cd "$(mktemp --directory)"
cabal init --exe --simple
ln -s I-do-not-exist cabal.project
cabal build
Expected behavour: cabal ignores the broken link and builds the project (ignoring the cabal.project and not signaling anything)
Patched version
Then, slighly adapting the example provided in the issue
cd "$(mktemp --directory)"
patchedCabal init --exe --simple
ln -s I-do-not-exist cabal.project
patchedCabal build
Expected behavour: cabal should exit with status code 1 and should print:
The given project file 'cabal.project' is broken. Is it a broken symbolic link?
Resolves #9937
I don't think it's particularly valuable to add a changelog for this, but let me know if you think otherwise
It's still a user-visible change, and entirely possible that someone relies on the old behavior (granting that that's relying on a bug, but https://xkcd.com/1172/).
Confirmed working on Windows:
CMD> mklink cabal.project I-do-not-exist
symbolic link created for cabal.project <<===>> I-do-not-exist
➜ rm I-do-not-exist
➜ ls -lah cabal.project
lrwxrwxrwx 1 Javier Javier 4 Jul 22 23:53 cabal.project -> I-do-not-exist
➜ $CABAL build
Warning: this is a debug build of cabal-install with assertions enabled.
The given project file 'cabal.project' is broken. Is it a broken symbolic link?
This is my first PR on Cabal, and I'm not sure how the process works (I couldn't find any information in the CONTRIBUTING file). Should I rebase it periodically? Am I missing anything to get this PR reviewed?
Hey @albertodvp! I'm sorry you had to wait for so long. Don't worry about rebasing. We just need to get around to reviewing your PR: two approvals are necessary. I prodded people on Matrix, so, hopefully, this can be done this or next week. After two approvals we'll see how to proceed.
Usually the attention: needs-review label is enough, but PRs sometimes get stuck there. My suggestion would be:
- Rebase the feature on top of
master. - Seems you were in the process of adding tests? or were you just fixing those cabal.project files (were they broken?)?
- I would suggest adding a test in any case.
- Hopefully someone will come and review the PR, otherwise you can ask in the Matrix channel.
Thank you!
I did add some tests, I'm happy to add others if you have some requests :relaxed:
Little things.
Strictly speaking, this is a race condition. It doesn't really matter in this case, though; you just gets suboptimal messaging.
Thank you for the review! Could you please help me see where I've introduced a race condition?
It's the very concept: checking something before use instead of at the time of use; in this case, that would be reporting why opening the project file in one of the withProject… etc. functions failed. (Look up "TOCTOU" in a search engine.)
The next step would be setting a merge label when the author is happy with the PR and there are no lingering discussions.
"Pull request #10103 has been removed from the queue squash-merge: The pull request cannot be checked because of an incompatibility with branch protections.". Help?
(I thought this might be a problem with #10260 at first, but (a) we've had successful merges since then (b) Mergify's log says this also happened on 27 July which was well before #10260.)
Yes, that's fine and probably what it needed. Mergify's been being a bit weird of late for some reason; it doesn't like something that's in master that it's failing to rebase on itself, but manual rebases seem to un-stick it.