Prompt overwrite policy #5672

fredefox · 2018-11-08T12:40:27Z

When linking binaries into a globally accessible location on the machine --overwrite-policy=always is needed if an existing file sits where we are trying to create a link. This allows for a prompt-the-user strategy.

Please 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.
NA: Couldn't find any existing documentation about overwrite-policy.
If the change is docs-only, [ci skip] is used to avoid triggering the build bots.

How I tested the change:

I simply ran cabal new-install on cabal-install (after having installed cabal-install with my feature.

WIP

Testing on Windows. Help wanted
A way to globally configure --overwrite-policy=prompt as the default. This was my idea with the patch. I need a bit of help on how to achieve this though as I'm not familiar with the code-base.

"url":"pull/5672", "account":"haskell", "repo":"cabal", "commit": "f459bb10c1e75c425bdb8acdce892d00a231aed5", "tag":"linux-7.10.3" }

"url":"pull/5672", "account":"haskell", "repo":"cabal", "commit": "f459bb10c1e75c425bdb8acdce892d00a231aed5", "tag":"linux-8.0.2" }

fgaz · 2018-11-08T13:29:31Z

A way to globally configure --overwrite-policy=prompt as the default. This was my idea with the patch. I need a bit of help on how to achieve this though as I'm not familiar with the code-base.

See #5644. SavedConfig represents the ~/.cabal/config file, but right now NewInstallFlags isn't in there

"url":"pull/5672", "account":"haskell", "repo":"cabal", "commit": "f459bb10c1e75c425bdb8acdce892d00a231aed5", "tag":"linux-8.2.2" }

"url":"pull/5672", "account":"haskell", "repo":"cabal", "commit": "f459bb10c1e75c425bdb8acdce892d00a231aed5", "tag":"linux-8.4.3" }

"url":"pull/5672", "account":"haskell", "repo":"cabal", "commit": "f459bb10c1e75c425bdb8acdce892d00a231aed5", "tag":"linux-8.4.3-fdebug-expensive-assertions" }

"url":"pull/5672", "account":"haskell", "repo":"cabal", "commit": "f459bb10c1e75c425bdb8acdce892d00a231aed5", "tag":"osx-8.0.2" }

"url":"pull/5672", "account":"haskell", "repo":"cabal", "commit": "f459bb10c1e75c425bdb8acdce892d00a231aed5", "tag":"osx-7.10.3" }

23Skidoo · 2018-11-09T10:48:58Z

/cc @hvr

hvr · 2018-11-10T14:07:54Z

@23Skidoo I haven't yet looked at this in detail, but this is exactly the kind of overwrite policies I had in mind :-)

What I'm a bit unsure about though is how we can model interactivity into a future abstracted UI I/O layer (that thing for which the big "Cabal Monad" refactoring represents a pre-req) ... (but this isn't a blocker for this PR -- just something this reminds me of)

fredefox · 2019-02-03T14:46:53Z

I've changed the PR to target master. I just cherry-picked the one commit I have made towards this.

fgaz · 2019-03-26T08:49:23Z

I added the new-install flags to SavedConfig with #5870 (not working yet due to a bug in how flags are handled: #5942)

There's a conflict now though

gbaz · 2021-08-12T02:31:41Z

I hope this can be picked up and worked on, seems like it is a nice ux improvement that everyone is on board with.

gbaz · 2021-08-16T15:30:21Z

@fgaz can you try to resolve the conflict here? This looks like something we should make happen...

fgaz · 2021-08-20T11:34:38Z

I resolved the conflict, but the copy+prompt case is not handled. I left a TODO, if someone wants to finish this.

fgaz · 2021-08-20T11:36:56Z

Also sorry for force-pushing @fredefox, I remembered too late that this wasn't my branch (github kept the old ref archived though)

Mikolaj · 2021-08-25T12:34:55Z

@fredefox: thank you for your work so far. Would you like to add the missing case or are you busy ATM?

Mikolaj · 2021-09-11T08:49:47Z

Given that @fredefox seems busy, I'm opening this PR to a rescue operation by any noble volunteers. Please feel free to overwrite this branch (or create a new PR if you insist).

fredefox · 2021-09-24T09:39:40Z

Sorry @Mikolaj. Forgot to reply. My GitHub inbox is super swamped. I don't know if I'll look at this any time soon. Anyone should indeed feel free to take this over.

fredefox · 2021-10-24T13:05:59Z

I've updated the PR. I seem to have some laziness/buffering-issue. The prompt is not displayed before hitting enter after which a parse-error is also printed. I might return to it later. Other people are still invited to contribute patches on top of this.

I was unsure what heading to put my change under in the change log.

Mikolaj · 2021-10-25T08:45:48Z

Hi @fredefox! Thank you for you contribution. Please kindly format the changelog as, e.g., in #7766.

Mikolaj · 2021-11-08T15:02:05Z

@fredefox: hi! may I help to get this PR merged before it rots? :)

fredefox · 2021-11-09T11:33:56Z

Hi @Mikolaj. Feel free to take over. There is a remaining issue with my work. The prompt is not displayed eagerly enough. I'm not sure why. I suggest testing this, as I did, by running cabal-install Cabal twice. Once to install my change, and then once to see the prompt appear. I may return to this, but I make no promises as to when.

Mikolaj · 2021-11-09T11:57:24Z

@fredefox: got it. No pressure. Thanks for sharing the problem --- perhaps somebody takes a look and gets a hunch as to why this happens, before you return.

fredefox · 2021-11-15T18:00:09Z

Hi @Mikolaj. I found the issue and I wrote a solution to it. Emphases on "a". Perhaps someone might have a better proposal than what I did. I figured out that in promptDefault the call to putStr did not write to stdout before the call to getLine triggered. I'm actually surprised by this, because if I compare it to, say, this program:

main :: IO ()
main = putStr "Hello" *> getLine *> pure ()

The output does indeed appear before blocking for input. Perhaps some strange interaction from the Interactive instance for IO? I don't really know.

Anyways. In summation, the PR is ready to go as-is as far as I can see.

Mikolaj

I don't have an explanation, but I'm not surprised at all. I routinely flush whenever I finish adding to the displayed text.

Mikolaj · 2021-11-15T18:50:40Z

@Mergifyio rebase

mergify · 2021-11-15T18:51:07Z

rebase

✅ Branch has been successfully rebased

Kleidukos

Couple of changes, but it looks good overall. :)

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

Kleidukos · 2021-11-18T08:29:42Z

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

+    overwrite :: IO Bool
+    overwrite = remove >> copy
+    maybeOverwrite :: IO Bool
+    maybeOverwrite = do
+      a <- promptYesNo
+        "Existing file found while installing executable. Do you want to unlink that file? (y/n)"
+        MandatoryPrompt
+      if a then overwrite else pure a


You're doing this work twice, here and in InstallSymlink.hs. Could this maybe be unified? :)

I think it definitely might be able to do some refacturing to increase sharing between installBuiltExe and symlinkBinary. I'm not sure it's warranted in the case of maybeOverwrite. I'm personally not convinced that the code is made more legible by just doing it for maybeOverwrite, but I've included it in the latest commit (9f594be) for you to be the judge :)

Mikolaj · 2021-11-27T13:41:40Z

Is everybody happy with how the feedback was addressed? If so, I'm going to merge (and if nobody answers, I'm merging on Tuesday).

Mikolaj · 2021-11-30T18:05:00Z

@Mergifyio rebase

mergify · 2021-11-30T18:06:13Z

rebase

✅ Branch has been successfully rebased

Mikolaj · 2021-11-30T21:05:35Z

CI is OK. Merging. Thank you everybody!

fredefox · 2021-12-01T20:11:23Z

Thank you @Mikolaj!

fgaz added the cabal-install: nix-local-build label Nov 8, 2018

haskell-pushbot pushed a commit to haskell-pushbot/cabal-binaries that referenced this pull request Nov 8, 2018

{"origin":"haskell/cabal/pull/5672",

f894b2a

"url":"pull/5672", "account":"haskell", "repo":"cabal", "commit": "f459bb10c1e75c425bdb8acdce892d00a231aed5", "tag":"linux-7.10.3" }

haskell-pushbot pushed a commit to haskell-pushbot/cabal-binaries that referenced this pull request Nov 8, 2018

{"origin":"haskell/cabal/pull/5672",

a287fd4

"url":"pull/5672", "account":"haskell", "repo":"cabal", "commit": "f459bb10c1e75c425bdb8acdce892d00a231aed5", "tag":"linux-8.0.2" }

haskell-pushbot pushed a commit to haskell-pushbot/cabal-binaries that referenced this pull request Nov 8, 2018

{"origin":"haskell/cabal/pull/5672",

2a57762

"url":"pull/5672", "account":"haskell", "repo":"cabal", "commit": "f459bb10c1e75c425bdb8acdce892d00a231aed5", "tag":"linux-8.2.2" }

haskell-pushbot pushed a commit to haskell-pushbot/cabal-binaries that referenced this pull request Nov 8, 2018

{"origin":"haskell/cabal/pull/5672",

d23261d

"url":"pull/5672", "account":"haskell", "repo":"cabal", "commit": "f459bb10c1e75c425bdb8acdce892d00a231aed5", "tag":"linux-8.4.3" }

haskell-pushbot pushed a commit to haskell-pushbot/cabal-binaries that referenced this pull request Nov 8, 2018

{"origin":"haskell/cabal/pull/5672",

c031574

"url":"pull/5672", "account":"haskell", "repo":"cabal", "commit": "f459bb10c1e75c425bdb8acdce892d00a231aed5", "tag":"osx-8.0.2" }

haskell-pushbot pushed a commit to haskell-pushbot/cabal-binaries that referenced this pull request Nov 8, 2018

{"origin":"haskell/cabal/pull/5672",

2f8304c

"url":"pull/5672", "account":"haskell", "repo":"cabal", "commit": "f459bb10c1e75c425bdb8acdce892d00a231aed5", "tag":"osx-7.10.3" }

fredefox force-pushed the wip/prompt-overwrite-policy branch from f459bb1 to e61b60d Compare February 3, 2019 14:43

fredefox changed the base branch from 2.4 to master February 3, 2019 14:45

fgaz added the blocked: action-required label Mar 26, 2019

gbaz added the attention: needs-review label Aug 16, 2021

fgaz force-pushed the wip/prompt-overwrite-policy branch from e61b60d to f3f89d5 Compare August 20, 2021 11:31

Mikolaj added attention: pr-welcome and removed attention: needs-review labels Sep 11, 2021

fredefox force-pushed the wip/prompt-overwrite-policy branch from f3f89d5 to 7a4ba21 Compare October 24, 2021 13:01

fredefox force-pushed the wip/prompt-overwrite-policy branch from 7a4ba21 to 9be9bb7 Compare October 24, 2021 13:06

fredefox force-pushed the wip/prompt-overwrite-policy branch from b06b9e5 to 1d163d1 Compare November 9, 2021 11:50

fredefox force-pushed the wip/prompt-overwrite-policy branch 2 times, most recently from b06b9e5 to 9882a8d Compare November 15, 2021 16:57

Mikolaj approved these changes Nov 15, 2021

View reviewed changes

jneira force-pushed the wip/prompt-overwrite-policy branch from 35da53a to ae461cf Compare November 15, 2021 18:51

fgaz added attention: needs-review cabal-install: cmd/install and removed blocked: action-required attention: pr-welcome cabal-install: nix-local-build labels Nov 15, 2021

Kleidukos requested changes Nov 18, 2021

View reviewed changes

fredefox force-pushed the wip/prompt-overwrite-policy branch 2 times, most recently from 9f594be to b778903 Compare November 22, 2021 20:42

fredefox added 3 commits November 30, 2021 18:06

Add prompt strategy for --overwrite-policy

4462606

Flush stdout after prompt, before parsing response.

139ac4f

Address PR feedback

f27e87a

jneira force-pushed the wip/prompt-overwrite-policy branch from b778903 to f27e87a Compare November 30, 2021 18:06

Mikolaj merged commit 5ef4d23 into haskell:master Nov 30, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prompt overwrite policy #5672

Prompt overwrite policy #5672

fredefox commented Nov 8, 2018 •

edited

Loading

fgaz commented Nov 8, 2018

23Skidoo commented Nov 9, 2018

hvr commented Nov 10, 2018 •

edited

Loading

fredefox commented Feb 3, 2019

fgaz commented Mar 26, 2019

gbaz commented Aug 12, 2021

gbaz commented Aug 16, 2021

fgaz commented Aug 20, 2021

fgaz commented Aug 20, 2021

Mikolaj commented Aug 25, 2021

Mikolaj commented Sep 11, 2021 •

edited

Loading

fredefox commented Sep 24, 2021

fredefox commented Oct 24, 2021

Mikolaj commented Oct 25, 2021

Mikolaj commented Nov 8, 2021

fredefox commented Nov 9, 2021

Mikolaj commented Nov 9, 2021

fredefox commented Nov 15, 2021

Mikolaj left a comment

Mikolaj commented Nov 15, 2021

mergify bot commented Nov 15, 2021

Kleidukos left a comment

Kleidukos Nov 18, 2021

fredefox Nov 22, 2021

Mikolaj commented Nov 27, 2021

Mikolaj commented Nov 30, 2021

mergify bot commented Nov 30, 2021

Mikolaj commented Nov 30, 2021

fredefox commented Dec 1, 2021

Prompt overwrite policy #5672

Prompt overwrite policy #5672

Conversation

fredefox commented Nov 8, 2018 • edited Loading

WIP

fgaz commented Nov 8, 2018

23Skidoo commented Nov 9, 2018

hvr commented Nov 10, 2018 • edited Loading

fredefox commented Feb 3, 2019

fgaz commented Mar 26, 2019

gbaz commented Aug 12, 2021

gbaz commented Aug 16, 2021

fgaz commented Aug 20, 2021

fgaz commented Aug 20, 2021

Mikolaj commented Aug 25, 2021

Mikolaj commented Sep 11, 2021 • edited Loading

fredefox commented Sep 24, 2021

fredefox commented Oct 24, 2021

Mikolaj commented Oct 25, 2021

Mikolaj commented Nov 8, 2021

fredefox commented Nov 9, 2021

Mikolaj commented Nov 9, 2021

fredefox commented Nov 15, 2021

Mikolaj left a comment

Choose a reason for hiding this comment

Mikolaj commented Nov 15, 2021

mergify bot commented Nov 15, 2021

✅ Branch has been successfully rebased

Kleidukos left a comment

Choose a reason for hiding this comment

Kleidukos Nov 18, 2021

Choose a reason for hiding this comment

fredefox Nov 22, 2021

Choose a reason for hiding this comment

Mikolaj commented Nov 27, 2021

Mikolaj commented Nov 30, 2021

mergify bot commented Nov 30, 2021

✅ Branch has been successfully rebased

Mikolaj commented Nov 30, 2021

fredefox commented Dec 1, 2021

fredefox commented Nov 8, 2018 •

edited

Loading

hvr commented Nov 10, 2018 •

edited

Loading

Mikolaj commented Sep 11, 2021 •

edited

Loading