Warn early that overwrite-policy is needed before build #9268

philderbeast · 2023-09-17T13:41:37Z

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 (not necessary).
Manual QA notes have been included.

Bonus points for added automated tests!

philderbeast · 2023-09-17T14:00:07Z

To test, unpack a package from hackage that you've previously installed and try installing it without an overwrite-policy. It should stop with a warning before building.

$ cabal get updo-1.0.0
Unpacking to updo-1.0.0/

$ cd updo-1.0.0/

$ cabal --version
cabal-install version 3.11
compiled using version 3.11.0.0 of the Cabal library 

$ cabal install
Wrote tarball sdist to
/.../updo-1.0.0/dist-newstyle/sdist/updo-1.0.0.tar.gz
Resolving dependencies...
Build profile: -w ghc-9.4.6 -O1
In order, the following will be built (use -v for more details):
 - updo-1.0.0 (exe:updo-pkg-groups) (requires build)
 - updo-1.0.0 (exe:updo-pkgs-sorted) (requires build)
 - updo-1.0.0 (exe:updo-pkgs-upgrade-done) (requires build)
 - updo-1.0.0 (exe:updo-pkgs-upgrade-partition) (requires build)
 - updo-1.0.0 (exe:updo-sha256map) (requires build)
Error: cabal: Path '/.../.cabal/bin/updo-pkg-groups' already
exists. Use --overwrite-policy=always to overwrite.

$ cabal install --overwrite-policy=always
Wrote tarball sdist to
/.../updo-1.0.0/dist-newstyle/sdist/updo-1.0.0.tar.gz
Resolving dependencies...
Build profile: -w ghc-9.4.6 -O1
In order, the following will be built (use -v for more details):
 - updo-1.0.0 (exe:updo-pkg-groups) (requires build)
 - updo-1.0.0 (exe:updo-pkgs-sorted) (requires build)
 - updo-1.0.0 (exe:updo-pkgs-upgrade-done) (requires build)
 - updo-1.0.0 (exe:updo-pkgs-upgrade-partition) (requires build)
 - updo-1.0.0 (exe:updo-sha256map) (requires build)
Starting     updo-1.0.0 (exe:updo-pkg-groups)
Starting     updo-1.0.0 (exe:updo-pkgs-upgrade-done)
Starting     updo-1.0.0 (exe:updo-pkgs-upgrade-partition)
Starting     updo-1.0.0 (exe:updo-pkgs-sorted)
Starting     updo-1.0.0 (exe:updo-sha256map)
Building     updo-1.0.0 (exe:updo-pkgs-upgrade-done)
Building     updo-1.0.0 (exe:updo-pkgs-upgrade-partition)
Building     updo-1.0.0 (exe:updo-pkg-groups)
Building     updo-1.0.0 (exe:updo-pkgs-sorted)
Building     updo-1.0.0 (exe:updo-sha256map)
Installing   updo-1.0.0 (exe:updo-pkgs-upgrade-done)
Installing   updo-1.0.0 (exe:updo-pkgs-sorted)
Completed    updo-1.0.0 (exe:updo-pkgs-upgrade-done)
Completed    updo-1.0.0 (exe:updo-pkgs-sorted)
Installing   updo-1.0.0 (exe:updo-pkgs-upgrade-partition)
Completed    updo-1.0.0 (exe:updo-pkgs-upgrade-partition)
Installing   updo-1.0.0 (exe:updo-pkg-groups)
Completed    updo-1.0.0 (exe:updo-pkg-groups)
Installing   updo-1.0.0 (exe:updo-sha256map)
Completed    updo-1.0.0 (exe:updo-sha256map)
Symlinking 'updo-pkg-groups' to
'/.../.cabal/bin/updo-pkg-groups'
Symlinking 'updo-pkgs-sorted' to
'/.../.cabal/bin/updo-pkgs-sorted'
Symlinking 'updo-pkgs-upgrade-done' to
'/.../.cabal/bin/updo-pkgs-upgrade-done'
Symlinking 'updo-pkgs-upgrade-partition' to
'/.../.cabal/bin/updo-pkgs-upgrade-partition'
Symlinking 'updo-sha256map' to '/.../.cabal/bin/updo-sha256map'

philderbeast · 2023-09-18T14:37:47Z

@Mikolaj the check failures don't seem related to changes I've made. Is there anything I need to do to move these along?

Mikolaj · 2023-09-19T09:06:44Z

I "fixed" precisely that one yesterday in another PR by clearing a part of the CI cache. Let me restart your CI and if it doesn't help, I'd clear some other parts of the cache.

ulysses4ever · 2023-09-19T19:55:45Z

Great contribution! I'll try to review properly as soon as I'm free from some pressing stuff. Hope someone beats me to it though.

andreabedini

At a first glance this looks very good! I'll review it in more detail tomorrow; in the meantime it needs tests ☺️

I see tests and dismiss my request for changes. I didn't have the bandwidth to look at this unfortuntately.

changelog.d/issue-5993

cabal-testsuite/PackageTests/LinkerOptions/NonignoredConfigs/cabal.test.hs

philderbeast · 2023-10-26T14:45:07Z

Could I please be allowed to add labels? I would like to mark this as a WIP. I'm finding that adding the test is harder than making the fix and don't want to bother reviewers with something half done.

ffaf1 · 2023-10-26T15:44:13Z

@philderbeast there is a “Still in progress? convert to draft” button on the right bar of the UI. Would that do?

philderbeast · 2023-10-30T23:10:40Z

It looks like the normalization of the test output doesn't give the same result when running on Windows for CI run Validate / Validate windows-latest ghc-9.4.4 (pull_request):

Actual output differs from expected:

stderr:
--- "D:\\a\\cabal\\cabal\\cabal-testsuite\\PackageTests\\WarnEarlyOverwrite\\clean-install-by-copy.dist\\clean-install-by-copy.out.normalized"	2023-10-30 22:50:07.370377400 +0000
+++ "D:\\a\\cabal\\cabal\\cabal-testsuite\\PackageTests\\WarnEarlyOverwrite\\clean-install-by-copy.dist\\clean-install-by-copy.comp.out.normalized"	2023-10-30 22:50:07.370377400 +0000
@@ -8,5 +8,5 @@
 Preprocessing executable 'warn-early-overwrite' for WarnEarlyOverwrite-0.1.0.0...
 Building executable 'warn-early-overwrite' for WarnEarlyOverwrite-0.1.0.0...
 Installing executable warn-early-overwrite in <PATH>
-Warning: The directory <GBLTMPDIR>/ghc-<GHCVER>/incoming/new-<RAND><GBLTMPDIR>/ghc-<GHCVER>/<PACKAGE>-<HASH>/bin is not in the system search path.
-Copying 'warn-early-overwrite' to '<GBLTMPDIR>/warn-early-overwrite'
+Warning: The directory <GBLTMPDIR><GHCVER>/incoming/new-2448/Users/RUNNER~1/AppData/Local/Temp/cabal-test-store-28260/ghc-<GHCVER>/WarnEarlyOver_-0.1.0.0-4c19059e06a32b93b2812983631117e77a2d3833/bin is not in the system search path.
+Copying 'warn-early-overwrite' to '<GBLTMPDIR>'
*** Exception: ExitFailure 1

* Add symlinkableBinary * Extract installExesPrep * Add data InstallExe and data Symlink * Replace symlinkable with symlink taking InstallExe * Replace symlink/copy as symlink or copy * Improve a haddock, from check to try and stop * Get rid of duplication between install*Exes * Add InstallCfg * Drop the cfg prefix * Make errorMessage a where nested local * Add haddocks to InstallCheck

* Add project to avoid solver errors * SkipIfWindows for warn early overwrite tests * Windows does not natively include a touch command * Skip symlink install on windows * Add install by copy test and skip symlink install for Windows

philderbeast · 2023-12-01T18:08:10Z

I can have a look tomorrow. I find it a little difficult to keep track of what are refactorings and what is needed for functionality changes from a quick glance though.

Thanks @mpickering. I've squashed commits and rejigged the changes to minimize differences. @ulysses4ever you may want to take another look.

ulysses4ever · 2023-12-03T17:41:08Z

Is there no way to have two separate commits, one for refactoring and one for the behavioral change? It's fine if not, just asking.

geekosaur · 2023-12-03T17:53:57Z

It's possible but a pain, especially if the code changes are interleaved (which means you can't use git's ability to pick individual diff chunks). By far the easiest way to do it is to discard all changes (possibly stashing them for reference) and start over, doing and committing the refactoring first.

philderbeast · 2023-12-03T18:00:51Z

@ulysses4ever can you please point me at which parts you consider refactorings? I take a closer look then to see if those were necessary.

philderbeast · 2023-12-07T09:02:29Z

@Mikolaj should I add the merge+no-rebase label now?

mpickering · 2023-12-07T09:21:28Z

I have looked at this for a little bit but the diff is just too hard for me to read and understand without putting more effort in. I found it hard to work out what the functional changes were, what the refactoring was or what was an artifact introduced by formatting so I won't be able to review this one.

Mikolaj · 2023-12-07T09:29:38Z

@philderbeast: yes please, as soon as you are satisfied with the process. Beyond 2 reviews it's your call whether to ask for more.

philderbeast · 2023-12-07T12:14:00Z

I have looked at this for a little bit but the diff is just too hard for me to read and understand

@mpickering I read that as a negative review.

Overall, I found adding tests for this fix was a lot more time and work than doing the fix. The docs on how to write tests helped.

When I first put this up for review, I'd moved some functions to top level by need so that I could use them in two places. I had not placed these strategically to reduce the diff count. On request, I squashed commits but also moved lines back closer to where they'd been to minimize diffs (some diffs I couldn't minimize because the formatter would change the layout again, mostly indentation changes). Sorry I didn't think to minimize diffs before asking for review.

What could count as a refactoring is collecting repeated series of function arguments into records, InstallCfg and InstallExe. I feel the declutter there is worth it.

philderbeast · 2023-12-07T14:17:23Z

To run the test locally:

$ ghcup set ghc 9.4.7
$ cabal build all --enable-tests --enable-benchmarks
$ cabal run cabal-testsuite:cabal-tests -- --with-cabal=./dist-newstyle/build/x86_64-linux/ghc-9.4.7/cabal-install-3.11.0.0/x/cabal/build/cabal/cabal cabal-testsuite/PackageTests/WarnEarlyOverwrite/dirty-install.test.hs
...
-----BEGIN CABAL OUTPUT-----
Installing executable warn-early-overwrite in /tmp/cabal-test-store-62007/ghc-9.4.7/incoming/new-62101/tmp/cabal-test-store-62007/ghc-9.4.7/WarnEarlyOverwrite-0.1.0.0-e-warn-early-overwrite-81f111c4ea95736fd5a3f16b46b053fdec9c718a890fbe97dc2b5338f6a7de04/bin
-----END CABAL OUTPUT-----
-----BEGIN CABAL OUTPUT-----
Warning: The directory /tmp/cabal-test-store-62007/ghc-9.4.7/incoming/new-62101/tmp/cabal-test-store-62007/ghc-9.4.7/WarnEarlyOverwrite-0.1.0.0-e-warn-early-overwrite-81f111c4ea95736fd5a3f16b46b053fdec9c718a890fbe97dc2b5338f6a7de04/bin is not in the system search path.
-----END CABAL OUTPUT-----
...
-----BEGIN CABAL OUTPUT-----
Symlinking 'warn-early-overwrite' to '/tmp/cabal-test-store-62007/warn-early-overwrite'
-----END CABAL OUTPUT-----
OK

philderbeast · 2023-12-07T14:25:04Z

I thought I'd also test with ghc-9.8.1 locally. This was fine apart from a little fragility to GHC version changes adding a warning to the output.

$ $ ghcup set ghc 9.4.7

$ cabal build all --enable-tests --enable-benchmarks

$ cabal run cabal-testsuite:cabal-tests -- --with-cabal=./dist-newstyle/build/x86_64-linux/ghc-9.4.7/cabal-install-3.11.0.0/x/cabal/build/cabal/cabal cabal-testsuite/PackageTests/WarnEarlyOverwrite/dirty-install.test.hs --accept

$ git diff

No difference with ghc-9.4.7 but trying with ghc-9.8.1 there is a difference.

$ ghcup set ghc 9.8.1
...
$ cabal build all --enable-tests --enable-benchmarks
Warning: Unknown/unsupported 'ghc' version detected (Cabal 3.11.0.0 supports
'ghc' version < 9.8): /.../.ghcup/bin/ghc is version 9.8.1
Resolving dependencies...
Build profile: -w ghc-9.8.1 -O1
...

$ cabal run cabal-testsuite:cabal-tests -- --with-cabal=./dist-newstyle/build/x86_64-linux/ghc-9.8.1/cabal-install-3.11.0.0/x/cabal/build/cabal/cabal cabal-testsuite/PackageTests/WarnEarlyOverwrite/dirty-install.test.hs --accept

$ git diff
...

diff --git a/cabal-testsuite/PackageTests/WarnEarlyOverwrite/dirty-install.out b/cabal-testsuite/PackageTests/WarnEarlyOverwrite/dirty-install.out
index 3a813eebf..34d3ad6eb 100644
--- a/cabal-testsuite/PackageTests/WarnEarlyOverwrite/dirty-install.out
+++ b/cabal-testsuite/PackageTests/WarnEarlyOverwrite/dirty-install.out
@@ -1,5 +1,6 @@
 # cabal v2-install
 Wrote tarball sdist to <ROOT>/dirty-install.dist/work/./dist/sdist/WarnEarlyOverwrite-0.1.0.0.tar.gz
+Warning: Unknown/unsupported 'ghc' version detected (Cabal 3.11.0.0 supports 'ghc' version < 9.8): /.../.ghcup/bin/ghc is version <GHCVER>
 Resolving dependencies...
 Build profile: -w ghc-<GHCVER> -O1
 In order, the following will be built:
@@ -8,6 +9,7 @@ Error: [Cabal-7149]
 Path '<GBLTMPDIR>/warn-early-overwrite' already exists. Use --overwrite-policy=always to overwrite.
 # cabal v2-install
 Wrote tarball sdist to <ROOT>/dirty-install.dist/work/./dist/sdist/WarnEarlyOverwrite-0.1.0.0.tar.gz
+Warning: Unknown/unsupported 'ghc' version detected (Cabal 3.11.0.0 supports 'ghc' version < 9.8): /.../.ghcup/bin/ghc is version <GHCVER>
 Resolving dependencies...
 Build profile: -w ghc-<GHCVER> -O1
 In order, the following will be built:

philderbeast · 2023-12-07T14:26:59Z

Label merge+no rebase is necessary when the pull request is from an organisation.

ulysses4ever · 2023-12-08T14:52:21Z

What could count as a refactoring is collecting repeated series of function arguments into records, InstallCfg and InstallExe. I feel the declutter there is worth it.

definitely refractoring.

moved some functions to top level by need

Same.

Would be helpful for potential reviewers, I think, to have these in a separate commit. But that's okay.

Thank you for the contribution!

philderbeast · 2023-12-09T18:25:56Z

Closed in favour of #9506.

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

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

geekosaur approved these changes Sep 17, 2023

View reviewed changes

ulysses4ever added the attention: needs-review label Sep 19, 2023

philderbeast force-pushed the warn-early-overwrite branch 2 times, most recently from 7edf99d to d5fc1f8 Compare September 28, 2023 10:09

andreabedini previously requested changes Oct 3, 2023

View reviewed changes

philderbeast force-pushed the warn-early-overwrite branch from d5fc1f8 to 8ddf0b4 Compare October 6, 2023 20:10

philderbeast force-pushed the warn-early-overwrite branch 3 times, most recently from 1b9793a to 6c69db7 Compare October 21, 2023 18:59

ulysses4ever reviewed Oct 23, 2023

View reviewed changes

changelog.d/issue-5993 Show resolved Hide resolved

geekosaur approved these changes Oct 23, 2023

View reviewed changes

geekosaur reviewed Oct 23, 2023

View reviewed changes

cabal-testsuite/PackageTests/LinkerOptions/NonignoredConfigs/cabal.test.hs Outdated Show resolved Hide resolved

ffaf1 removed the attention: needs-review label Oct 26, 2023

philderbeast marked this pull request as draft October 26, 2023 16:04

philderbeast mentioned this pull request Oct 28, 2023

Add reinstall test to LinkerOptions/NonignoredConfigs #9377

Merged

1 task

philderbeast force-pushed the warn-early-overwrite branch 2 times, most recently from 6c69db7 to 0c8a53f Compare October 29, 2023 13:31

philderbeast marked this pull request as ready for review October 29, 2023 13:57

philderbeast force-pushed the warn-early-overwrite branch from c39a8ec to b74fcad Compare October 29, 2023 14:01

philderbeast force-pushed the warn-early-overwrite branch from 440ef73 to 221041b Compare October 31, 2023 14:02

philderbeast added the attention: needs-review label Oct 31, 2023

philderbeast requested a review from ulysses4ever October 31, 2023 20:59

philderbeast force-pushed the warn-early-overwrite branch 4 times, most recently from 060c327 to 9203e28 Compare December 1, 2023 14:03

philderbeast added 3 commits December 1, 2023 10:00

Add dirty and clean install tests

1e3214b

* Add project to avoid solver errors * SkipIfWindows for warn early overwrite tests * Windows does not natively include a touch command * Skip symlink install on windows * Add install by copy test and skip symlink install for Windows

Add changelog entry

e8aa842

philderbeast force-pushed the warn-early-overwrite branch from 9203e28 to e8aa842 Compare December 1, 2023 15:02

philderbeast added merge+no rebase and removed attention: needs-review labels Dec 7, 2023

philderbeast mentioned this pull request Dec 7, 2023

How to suppress "unsupported 'ghc' version detected" in test outputs? #9501

Open

philderbeast mentioned this pull request Dec 9, 2023

Warn early overwrite expanded #9506

Merged

4 tasks

philderbeast closed this Dec 9, 2023

philderbeast removed the merge+no rebase label Dec 9, 2023

ulysses4ever reviewed Dec 10, 2023

View reviewed changes

cabal-install/src/Distribution/Client/CmdInstall.hs Show resolved Hide resolved

ulysses4ever reviewed Dec 10, 2023

View reviewed changes

cabal-install/src/Distribution/Client/InstallSymlink.hs Show resolved Hide resolved

ulysses4ever reviewed Dec 10, 2023

View reviewed changes

cabal-install/src/Distribution/Client/InstallSymlink.hs Show resolved Hide resolved

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Warn early that overwrite-policy is needed before build #9268

Warn early that overwrite-policy is needed before build #9268

philderbeast commented Sep 17, 2023 •

edited

Loading

philderbeast commented Sep 17, 2023

philderbeast commented Sep 18, 2023

Mikolaj commented Sep 19, 2023

ulysses4ever commented Sep 19, 2023

andreabedini left a comment

philderbeast commented Oct 26, 2023

ffaf1 commented Oct 26, 2023

philderbeast commented Oct 30, 2023

philderbeast commented Dec 1, 2023

ulysses4ever commented Dec 3, 2023

geekosaur commented Dec 3, 2023 •

edited

Loading

philderbeast commented Dec 3, 2023

philderbeast commented Dec 7, 2023

mpickering commented Dec 7, 2023

Mikolaj commented Dec 7, 2023

philderbeast commented Dec 7, 2023 •

edited

Loading

philderbeast commented Dec 7, 2023

philderbeast commented Dec 7, 2023 •

edited

Loading

philderbeast commented Dec 7, 2023

ulysses4ever commented Dec 8, 2023

philderbeast commented Dec 9, 2023

Warn early that overwrite-policy is needed before build #9268

Warn early that overwrite-policy is needed before build #9268

Conversation

philderbeast commented Sep 17, 2023 • edited Loading

philderbeast commented Sep 17, 2023

philderbeast commented Sep 18, 2023

Mikolaj commented Sep 19, 2023

ulysses4ever commented Sep 19, 2023

andreabedini left a comment

Choose a reason for hiding this comment

philderbeast commented Oct 26, 2023

ffaf1 commented Oct 26, 2023

philderbeast commented Oct 30, 2023

philderbeast commented Dec 1, 2023

ulysses4ever commented Dec 3, 2023

geekosaur commented Dec 3, 2023 • edited Loading

philderbeast commented Dec 3, 2023

philderbeast commented Dec 7, 2023

mpickering commented Dec 7, 2023

Mikolaj commented Dec 7, 2023

philderbeast commented Dec 7, 2023 • edited Loading

philderbeast commented Dec 7, 2023

philderbeast commented Dec 7, 2023 • edited Loading

philderbeast commented Dec 7, 2023

ulysses4ever commented Dec 8, 2023

philderbeast commented Dec 9, 2023

philderbeast commented Sep 17, 2023 •

edited

Loading

geekosaur commented Dec 3, 2023 •

edited

Loading

philderbeast commented Dec 7, 2023 •

edited

Loading

philderbeast commented Dec 7, 2023 •

edited

Loading