-
Notifications
You must be signed in to change notification settings - Fork 691
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
new-install: error on existing symlink, overwrite option #5638
Conversation
"url":"pull/5638", "account":"haskell", "repo":"cabal", "commit": "10f5a983ce727e9e745cbcf154e80c6817301b13", "tag":"linux-8.0.2" }
"url":"pull/5638", "account":"haskell", "repo":"cabal", "commit": "10f5a983ce727e9e745cbcf154e80c6817301b13", "tag":"linux-7.8.4" }
"url":"pull/5638", "account":"haskell", "repo":"cabal", "commit": "10f5a983ce727e9e745cbcf154e80c6817301b13", "tag":"linux-7.10.3" }
10f5a98
to
446be82
Compare
"url":"pull/5638", "account":"haskell", "repo":"cabal", "commit": "446be82f50735871092e9f7434c24aa782227f01", "tag":"linux-7.10.3" }
"url":"pull/5638", "account":"haskell", "repo":"cabal", "commit": "446be82f50735871092e9f7434c24aa782227f01", "tag":"linux-8.0.2" }
"url":"pull/5638", "account":"haskell", "repo":"cabal", "commit": "446be82f50735871092e9f7434c24aa782227f01", "tag":"linux-7.8.4" }
"url":"pull/5638", "account":"haskell", "repo":"cabal", "commit": "446be82f50735871092e9f7434c24aa782227f01", "tag":"linux-8.2.2" }
"url":"pull/5638", "account":"haskell", "repo":"cabal", "commit": "446be82f50735871092e9f7434c24aa782227f01", "tag":"linux-7.6.3" }
"url":"pull/5638", "account":"haskell", "repo":"cabal", "commit": "446be82f50735871092e9f7434c24aa782227f01", "tag":"linux-8.4.4" }
"url":"pull/5638", "account":"haskell", "repo":"cabal", "commit": "446be82f50735871092e9f7434c24aa782227f01", "tag":"linux-8.4.4-fdebug-expensive-assertions" }
"url":"pull/5638", "account":"haskell", "repo":"cabal", "commit": "446be82f50735871092e9f7434c24aa782227f01", "tag":"osx-7.8.4" }
446be82
to
7f13951
Compare
Before, new-install could exit successfully while actually _not_ symlinking anything. Moreover, without an overwrite option, upgrading cabal was impossible without moving links around. * Error out on overwrite * Overwrite if --force-overwrite is passed
7f13951
to
a3d222d
Compare
"url":"pull/5638", "account":"haskell", "repo":"cabal", "commit": "a3d222d2a7581465b902ba1962d7c56f45a66e7c", "tag":"linux-7.10.3" }
"url":"pull/5638", "account":"haskell", "repo":"cabal", "commit": "a3d222d2a7581465b902ba1962d7c56f45a66e7c", "tag":"linux-7.8.4" }
"url":"pull/5638", "account":"haskell", "repo":"cabal", "commit": "a3d222d2a7581465b902ba1962d7c56f45a66e7c", "tag":"linux-8.2.2" }
"url":"pull/5638", "account":"haskell", "repo":"cabal", "commit": "a3d222d2a7581465b902ba1962d7c56f45a66e7c", "tag":"linux-8.0.2" }
"url":"pull/5638", "account":"haskell", "repo":"cabal", "commit": "a3d222d2a7581465b902ba1962d7c56f45a66e7c", "tag":"linux-7.6.3" }
"url":"pull/5638", "account":"haskell", "repo":"cabal", "commit": "a3d222d2a7581465b902ba1962d7c56f45a66e7c", "tag":"linux-8.4.4" }
"url":"pull/5638", "account":"haskell", "repo":"cabal", "commit": "a3d222d2a7581465b902ba1962d7c56f45a66e7c", "tag":"linux-8.4.4-fdebug-expensive-assertions" }
"url":"pull/5638", "account":"haskell", "repo":"cabal", "commit": "a3d222d2a7581465b902ba1962d7c56f45a66e7c", "tag":"osx-7.8.4" }
"url":"pull/5638", "account":"haskell", "repo":"cabal", "commit": "a3d222d2a7581465b902ba1962d7c56f45a66e7c", "tag":"osx-8.0.2" }
"url":"pull/5638", "account":"haskell", "repo":"cabal", "commit": "bf4b9c949a50a55bf91ee30df07437c9d956b9a8", "tag":"linux-7.10.3" }
"url":"pull/5638", "account":"haskell", "repo":"cabal", "commit": "bf4b9c949a50a55bf91ee30df07437c9d956b9a8", "tag":"linux-8.0.2" }
"url":"pull/5638", "account":"haskell", "repo":"cabal", "commit": "bf4b9c949a50a55bf91ee30df07437c9d956b9a8", "tag":"linux-7.8.4" }
"url":"pull/5638", "account":"haskell", "repo":"cabal", "commit": "bf4b9c949a50a55bf91ee30df07437c9d956b9a8", "tag":"linux-8.2.2" }
"url":"pull/5638", "account":"haskell", "repo":"cabal", "commit": "bf4b9c949a50a55bf91ee30df07437c9d956b9a8", "tag":"linux-7.6.3" }
"url":"pull/5638", "account":"haskell", "repo":"cabal", "commit": "bf4b9c949a50a55bf91ee30df07437c9d956b9a8", "tag":"linux-8.4.4" }
"url":"pull/5638", "account":"haskell", "repo":"cabal", "commit": "bf4b9c949a50a55bf91ee30df07437c9d956b9a8", "tag":"linux-8.4.4-fdebug-expensive-assertions" }
"url":"pull/5638", "account":"haskell", "repo":"cabal", "commit": "bf4b9c949a50a55bf91ee30df07437c9d956b9a8", "tag":"osx-7.8.4" }
"url":"pull/5638", "account":"haskell", "repo":"cabal", "commit": "bf4b9c949a50a55bf91ee30df07437c9d956b9a8", "tag":"osx-8.0.2" }
"url":"pull/5638", "account":"haskell", "repo":"cabal", "commit": "bf4b9c949a50a55bf91ee30df07437c9d956b9a8", "tag":"osx-7.10.3" }
I gave this PR a test-run, and it appears to work reasonably. There's some minor aesthetic issues with some messages which e.g. claim that a "Symlink" already exists even though it's not a symlink but a regular file... but there's a few more (not high prio) UI details we might want to improve/fix there that this can be addressed by a future PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
"Overwrite an existing symlink." | ||
ninstForceOverwrite (\v flags -> flags { ninstForceOverwrite = v }) | ||
trueArg | ||
, option [] ["overwrite-policy"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can also make it work similarly to --allow-newer
, i.e. make --overwrite-symlink
w/o arguments be a shortcut for --overwrite-symlink=always
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@23Skidoo btw, didn't we want to deprecate the option-less --allow-newer
variant? :)
also, are you implying a suggestion of renaming --overwrite-policy=...
to --overwrite-symlink=...
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh and btw, I'd like to point out that during dogfooding I started using an abbrev, like e.g.
cabal v2-install alex --overwrite=always
or even
cabal v2-install alex --over=always
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@23Skidoo after thinking about this, the decision whether to support an argument less --overwrite-...
hinges on what default we want to pick. I.e. if we default to always
it'd be confusing to have a seemingly no-op alias which just reaffirms the default setting; thus I'd suggest to postpone adding a shortcut to this PR, and put it up for vote together with the decision for which default we want to pick for the benefit of interactive shell users
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thus I'd suggest to postpone adding a shortcut to this PR
Sure. Let's merge.
[ci skip]
Merged, thanks! |
new-install: error on existing symlink, overwrite option (cherry picked from commit a0684df)
Also cherry-picked to 2.4. |
Phase 2 of the fix for #5491. Followup to #5602.
Erroring out is safer and more explicit.
Next, we'll have to check for error/no-op as early as possible (currently we build everything first).
/cc @hvr