Add check on project root file and fail if broken link #10103

albertodvp · 2024-06-11T20:13:56Z

This PR modifies behaviour or interface**

Include the following checklist in your PR:

Patches conform to the coding conventions.
Any changes that could be relevant to users have been recorded in the changelog.
The documentation has been updated, if necessary.
Manual QA notes have been included.
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

albertodvp · 2024-06-13T19:27:46Z

I don't think it's particularly valuable to add a changelog for this, but let me know if you think otherwise

geekosaur · 2024-06-13T19:33:02Z

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/).

jasagredo · 2024-07-22T21:55:25Z

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?

albertodvp · 2024-07-25T14:31:41Z

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?

ulysses4ever · 2024-07-25T15:05:42Z

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.

jasagredo · 2024-07-25T15:06:58Z

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.

albertodvp · 2024-07-25T15:32:18Z

Thank you!

I did add some tests, I'm happy to add others if you have some requests ☺️

geekosaur

Little things.

Strictly speaking, this is a race condition. It doesn't really matter in this case, though; you just gets suboptimal messaging.

geekosaur · 2024-07-25T15:37:36Z

cabal-install/src/Distribution/Client/ProjectConfig.hs

+      let isUsableAciton =
+            handle @IOException
+              -- NOTE: if any IOException is raised, we assume the file does not exist.
+              -- That is what happen when we call @pathIsSymbolicLink@ on an @FilePath@ that does not exist.


Suggested change

-- That is what happen when we call @pathIsSymbolicLink@ on an @FilePath@ that does not exist.

-- That is what happen when we call @pathIsSymbolicLink@ on a @FilePath@ that does not exist.

geekosaur · 2024-07-25T15:38:06Z

cabal-install/src/Distribution/Client/ProjectConfig.hs

+              -- That is what happen when we call @pathIsSymbolicLink@ on an @FilePath@ that does not exist.
+              (const $ pure False)
+              ((||) <$> pathIsSymbolicLink filePath <*> doesPathExist filePath)
+      isUnusable <- isUsableAciton


Suggested change

isUnusable <- isUsableAciton

isUnusable <- isUsableAction

geekosaur · 2024-07-25T15:38:22Z

cabal-install/src/Distribution/Client/ProjectConfig.hs

+  if exists
+    then return ProjectRootUsabilityPresentAndUsable
+    else do
+      let isUsableAciton =


Suggested change

let isUsableAciton =

let isUsableAction =

albertodvp · 2024-07-25T15:56:32Z

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?

geekosaur · 2024-07-25T16:02:21Z

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.)

ulysses4ever

Perfect, thanks!

ulysses4ever · 2024-07-25T16:16:49Z

The next step would be setting a merge label when the author is happy with the PR and there are no lingering discussions.

geekosaur · 2024-08-26T04:06:41Z

"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.)

albertodvp · 2024-08-26T11:17:57Z

Hi @geekosaur - I've just rebased and force-pushed. I hope that was ok

geekosaur · 2024-08-26T11:24:09Z

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.

albertodvp force-pushed the master branch from d252c2f to 5e34110 Compare June 11, 2024 20:16

ffaf1 added the attention: needs-manual-qa PR is destined for manual QA label Jun 12, 2024

albertodvp force-pushed the master branch from 5e34110 to 24d62c6 Compare June 13, 2024 19:28

albertodvp marked this pull request as ready for review June 14, 2024 06:03

albertodvp force-pushed the master branch from 44d6127 to ceb0497 Compare June 14, 2024 08:00

fgaz added attention: needs-review cabal-install: other re: project-file Concerning cabal.project files labels Jun 14, 2024

albertodvp force-pushed the master branch from ceb0497 to 5a16563 Compare June 23, 2024 19:42

albertodvp force-pushed the master branch from 5a16563 to e436bb9 Compare July 8, 2024 18:57

jasagredo added the tested-on: windows QA has been successful on Windows label Jul 22, 2024

albertodvp force-pushed the master branch from e436bb9 to 37348e3 Compare July 25, 2024 15:28

geekosaur approved these changes Jul 25, 2024

View reviewed changes

ulysses4ever approved these changes Jul 25, 2024

View reviewed changes

ulysses4ever removed the attention: needs-review label Jul 25, 2024

albertodvp added the squash+merge me Tell Mergify Bot to squash-merge label Jul 25, 2024

mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Jul 27, 2024

albertodvp force-pushed the master branch from 82d4fe4 to 9f2b551 Compare July 29, 2024 08:15

albertodvp removed the squash+merge me Tell Mergify Bot to squash-merge label Jul 29, 2024

geekosaur removed the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Aug 22, 2024

albertodvp force-pushed the master branch 2 times, most recently from 9fe7e37 to f4fbcf4 Compare August 22, 2024 19:18

albertodvp added the squash+merge me Tell Mergify Bot to squash-merge label Aug 23, 2024

mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Aug 25, 2024

albertodvp added 4 commits August 26, 2024 10:34

Add ProjectRootUsability datatype

66110b4

Make findProjectRoot aware of broken files

01e335b

Add changelog entry

22fbc13

Fix typos

908e57e

albertodvp force-pushed the master branch from f4fbcf4 to 908e57e Compare August 26, 2024 08:34

mergify bot merged commit 4f01b76 into haskell:master Aug 26, 2024
50 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add check on project root file and fail if broken link #10103

Add check on project root file and fail if broken link #10103

albertodvp commented Jun 11, 2024 •

edited

Loading

albertodvp commented Jun 13, 2024

geekosaur commented Jun 13, 2024

jasagredo commented Jul 22, 2024

albertodvp commented Jul 25, 2024

ulysses4ever commented Jul 25, 2024

jasagredo commented Jul 25, 2024 •

edited

Loading

albertodvp commented Jul 25, 2024

geekosaur left a comment

geekosaur Jul 25, 2024

geekosaur Jul 25, 2024

geekosaur Jul 25, 2024

albertodvp commented Jul 25, 2024

geekosaur commented Jul 25, 2024

ulysses4ever left a comment

ulysses4ever commented Jul 25, 2024

geekosaur commented Aug 26, 2024

albertodvp commented Aug 26, 2024

geekosaur commented Aug 26, 2024

	-- That is what happen when we call @pathIsSymbolicLink@ on an @FilePath@ that does not exist.
	-- That is what happen when we call @pathIsSymbolicLink@ on a @FilePath@ that does not exist.

Add check on project root file and fail if broken link #10103

Add check on project root file and fail if broken link #10103

Conversation

albertodvp commented Jun 11, 2024 • edited Loading

QA Notes

Build patched version

Reproduce

Not patched version

Patched version

albertodvp commented Jun 13, 2024

geekosaur commented Jun 13, 2024

jasagredo commented Jul 22, 2024

albertodvp commented Jul 25, 2024

ulysses4ever commented Jul 25, 2024

jasagredo commented Jul 25, 2024 • edited Loading

albertodvp commented Jul 25, 2024

geekosaur left a comment

Choose a reason for hiding this comment

geekosaur Jul 25, 2024

Choose a reason for hiding this comment

geekosaur Jul 25, 2024

Choose a reason for hiding this comment

geekosaur Jul 25, 2024

Choose a reason for hiding this comment

albertodvp commented Jul 25, 2024

geekosaur commented Jul 25, 2024

ulysses4ever left a comment

Choose a reason for hiding this comment

ulysses4ever commented Jul 25, 2024

geekosaur commented Aug 26, 2024

albertodvp commented Aug 26, 2024

geekosaur commented Aug 26, 2024

albertodvp commented Jun 11, 2024 •

edited

Loading

jasagredo commented Jul 25, 2024 •

edited

Loading