Fix path redundancy new-build output folders. #4978

Mistuke · 2017-12-26T08:06:23Z

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.
If the change is docs-only, [ci skip] is used to avoid triggering the build bots.

Please also shortly describe how you tested your change. Bonus points for added tests!

Tested it by compiling Cabal itself using the Makefile on a long path.

The setup call with copyCommand doesn't do what the comment in ProjectBuilding.hs suggested. It literally only assigns the path you specified. So you can choose any format you want as long as you pass it to copyCommand. So instead of using @$tmpDir/$prefix@ we now use @$tmpDir/$packageName-$version@. Because prefix is a prefix to tmpDir this means that before this patch cabal was halving the Windows MAX_PATH. Effectively meaning new-build would only work on paths shorter than 125 characters.

The consistency is maintained with absoluteInstallDirs by also taking the base name instead of dropping the drive when combing the paths. Proof that this is all consistent is that the build succeeds. Cabal only issues createDirectory calls in one place. So if the paths did not end up being the same the build would fail because it can't find the specified path.

Mistuke · 2017-12-26T08:07:15Z

cc @alexbiehl , @hvr

23Skidoo · 2017-12-26T10:48:15Z

Awesome, I was just bitten by this the other day when trying to build cabal-install binaries on Windows.

23Skidoo · 2017-12-26T11:49:39Z

Looks like there's a test failure on Linux.

/cc @dcoutts

Mistuke · 2017-12-26T11:56:44Z

Hmm indeed, it seems to be skipped on a lot of configuration. But the error is a bit odd..

Main.hs:1:8:
    Could not find module `RemotePkg'
    There are files missing in the `remote-pkg-2.0@remot_0O9PJ1ccB6bKshvGETwlge' package,
    try running 'ghc-pkg check'.
    Use -v to see a list of the files searched for.

But the log shows it registering the package.. Is there a way for me to run this test locally on Windows?

23Skidoo · 2017-12-26T13:31:00Z

@Mistuke See cabal-testsuite/README.md. Though it looks like that test is skipped on Windows, which is why the AppVeyor build succeeded.

Mistuke · 2017-12-26T17:44:14Z

@23Skidoo Thanks, I'll boot to Linux and give it a try.

23Skidoo · 2017-12-27T01:35:36Z

Some clues to why that test is failing:

$ ghc-pkg --package-db [...]/cabal-testsuite/PackageTests/NewBuild/CustomSetup/RemotePackageWithCustomSetup/build-package-from-repo-with-custom-setup.dist/home/.cabal/store/ghc-8.2.2/package.db list 
[...]/cabal-testsuite/PackageTests/NewBuild/CustomSetup/RemotePackageWithCustomSetup/build-package-from-repo-with-custom-setup.dist/home/.cabal/store/ghc-8.2.2/package.db
    remote-pkg-2.0
    remote-setup-dep-3.0

$ ghc-pkg --package-db [...]/cabal-testsuite/PackageTests/NewBuild/CustomSetup/RemotePackageWithCustomSetup/build-package-from-repo-with-custom-setup.dist/home/.cabal/store/ghc-8.2.2/package.db check
[...]
There are problems in package remote-pkg-2.0:
  Warning: library-dirs: [...]/cabal-testsuite/PackageTests/NewBuild/CustomSetup/RemotePackageWithCustomSetup/build-package-from-repo-with-custom-setup.dist/home/.cabal/store/ghc-8.2.2/remote-pkg-2.0-49b4ea173d672c8399dff7218dd2512ab818030cc9fab93eca633ac6d5885ced/lib doesn't exist or isn't a directory
  Warning: dynamic-library-dirs: [...]/cabal-testsuite/PackageTests/NewBuild/CustomSetup/RemotePackageWithCustomSetup/build-package-from-repo-with-custom-setup.dist/home/.cabal/store/ghc-8.2.2/remote-pkg-2.0-49b4ea173d672c8399dff7218dd2512ab818030cc9fab93eca633ac6d5885ced/lib doesn't exist or isn't a directory
  Warning: haddock-interfaces: [...]/cabal-testsuite/PackageTests/NewBuild/CustomSetup/RemotePackageWithCustomSetup/build-package-from-repo-with-custom-setup.dist/home/.cabal/store/ghc-8.2.2/remote-pkg-2.0-49b4ea173d672c8399dff7218dd2512ab818030cc9fab93eca633ac6d5885ced/share/doc/html/remote-pkg.haddock doesn't exist or isn't a file
  Warning: haddock-html: [...]/cabal-testsuite/PackageTests/NewBuild/CustomSetup/RemotePackageWithCustomSetup/build-package-from-repo-with-custom-setup.dist/home/.cabal/store/ghc-8.2.2/remote-pkg-2.0-49b4ea173d672c8399dff7218dd2512ab818030cc9fab93eca633ac6d5885ced/share/doc/html doesn't exist or isn't a directory
  import-dirs: [...]/cabal-testsuite/PackageTests/NewBuild/CustomSetup/RemotePackageWithCustomSetup/build-package-from-repo-with-custom-setup.dist/home/.cabal/store/ghc-8.2.2/remote-pkg-2.0-49b4ea173d672c8399dff7218dd2512ab818030cc9fab93eca633ac6d5885ced/lib doesn't exist or isn't a directory
  cannot find any of ["RemotePkg.hi","RemotePkg.p_hi","RemotePkg.dyn_hi"]
  cannot find any of ["libHSremote-pkg-2.0-49b4ea173d672c8399dff7218dd2512ab818030cc9fab93eca633ac6d5885ced.a","libHSremote-pkg-2.0-49b4ea173d672c8399dff7218dd2512ab818030cc9fab93eca633ac6d5885ced.p_a","libHSremote-pkg-2.0-49b4ea173d672c8399dff7218dd2512ab818030cc9fab93eca633ac6d5885ced-ghc8.2.2.so","libHSremote-pkg-2.0-49b4ea173d672c8399dff7218dd2512ab818030cc9fab93eca633ac6d5885ced-ghc8.2.2.dylib","HSremote-pkg-2.0-49b4ea173d672c8399dff7218dd2512ab818030cc9fab93eca633ac6d5885ced-ghc8.2.2.dll"] on library path
[...]

The following packages are broken, either because they have a problem
listed above, or because they depend on a broken package.
remote-pkg-2.0

$ ghc-pkg --package-db [...]/cabal-testsuite/PackageTests/NewBuild/CustomSetup/RemotePackageWithCustomSetup/build-package-from-repo-with-custom-setup.dist/home/.cabal/store/ghc-8.2.2/package.db describe remote-pkg-2.0           
name: remote-pkg
version: 2.0
id: remote-pkg-2.0-49b4ea173d672c8399dff7218dd2512ab818030cc9fab93eca633ac6d5885ced
key: remote-pkg-2.0-49b4ea173d672c8399dff7218dd2512ab818030cc9fab93eca633ac6d5885ced
license: UnspecifiedLicense
exposed: True
indefinite: False
exposed-modules:
    RemotePkg
abi: aec19496483f5a6f8e3d43d06c33eb16
trusted: False
import-dirs: [...]/cabal-testsuite/PackageTests/NewBuild/CustomSetup/RemotePackageWithCustomSetup/build-package-from-repo-with-custom-setup.dist/home/.cabal/store/ghc-8.2.2/remote-pkg-2.0-49b4ea173d672c8399dff7218dd2512ab818030cc9fab93eca633ac6d5885ced/lib
library-dirs: [...]/cabal-testsuite/PackageTests/NewBuild/CustomSetup/RemotePackageWithCustomSetup/build-package-from-repo-with-custom-setup.dist/home/.cabal/store/ghc-8.2.2/remote-pkg-2.0-49b4ea173d672c8399dff7218dd2512ab818030cc9fab93eca633ac6d5885ced/lib
dynamic-library-dirs: [...]/cabal-testsuite/PackageTests/NewBuild/CustomSetup/RemotePackageWithCustomSetup/build-package-from-repo-with-custom-setup.dist/home/.cabal/store/ghc-8.2.2/remote-pkg-2.0-49b4ea173d672c8399dff7218dd2512ab818030cc9fab93eca633ac6d5885ced/lib
data-dir: [...]/cabal-testsuite/PackageTests/NewBuild/CustomSetup/RemotePackageWithCustomSetup/build-package-from-repo-with-custom-setup.dist/home/.cabal/store/ghc-8.2.2/remote-pkg-2.0-49b4ea173d672c8399dff7218dd2512ab818030cc9fab93eca633ac6d5885ced/share
hs-libraries: HSremote-pkg-2.0-49b4ea173d672c8399dff7218dd2512ab818030cc9fab93eca633ac6d5885ced
depends:
    base-4.10.1.0
abi-depends: base-4.10.1.0=35a7f6be752ee4f7385cb5bf28677879
haddock-interfaces: [...]/cabal-testsuite/PackageTests/NewBuild/CustomSetup/RemotePackageWithCustomSetup/build-package-from-repo-with-custom-setup.dist/home/.cabal/store/ghc-8.2.2/remote-pkg-2.0-49b4ea173d672c8399dff7218dd2512ab818030cc9fab93eca633ac6d5885ced/share/doc/html/remote-pkg.haddock
haddock-html: [...]/cabal-testsuite/PackageTests/NewBuild/CustomSetup/RemotePackageWithCustomSetup/build-package-from-repo-with-custom-setup.dist/home/.cabal/store/ghc-8.2.2/remote-pkg-2.0-49b4ea173d672c8399dff7218dd2512ab818030cc9fab93eca633ac6d5885ced/share/doc/html
pkgroot: "[...]/cabal-testsuite/PackageTests/NewBuild/CustomSetup/RemotePackageWithCustomSetup/build-package-from-repo-with-custom-setup.dist/home/.cabal/store/ghc-8.2.2"

$ cd cabal-testsuite/PackageTests/NewBuild/CustomSetup/RemotePackageWithCustomSetup/build-package-from-repo-with-custom-setup.dist/home/.cabal/        

$ tree
.
├── config
├── packages
│   └── test-local-repo
│       ├── 01-index.cache
│       ├── 01-index.tar
│       ├── 01-index.tar.gz
│       ├── 01-index.tar.idx
│       ├── 01-index.timestamp
│       ├── mirrors.json
│       ├── remote-pkg
│       │   └── 2.0
│       │       └── remote-pkg-2.0.tar.gz
│       ├── remote-setup-dep
│       │   └── 3.0
│       │       └── remote-setup-dep-3.0.tar.gz
│       ├── root.json
│       ├── snapshot.json
│       └── timestamp.json
└── store
    └── ghc-8.2.2
        ├── incoming
        │   ├── remote-pkg-2.0-49b4ea173d672c8399dff7218dd2512ab818030cc9fab93eca633ac6d5885ced.lock
        │   └── remote-setup-dep-3.0-95e21c2632e40b32212bf1add9fcfe5272e685432265d9d7cc77221afb0d4286.lock
        ├── package.db
        │   ├── package.cache
        │   ├── package.cache.lock
        │   ├── remote-pkg-2.0-49b4ea173d672c8399dff7218dd2512ab818030cc9fab93eca633ac6d5885ced.conf
        │   └── remote-setup-dep-3.0-95e21c2632e40b32212bf1add9fcfe5272e685432265d9d7cc77221afb0d4286.conf
        ├── remote-pkg-2.0-49b4ea173d672c8399dff7218dd2512ab818030cc9fab93eca633ac6d5885ced
        │   ├── cabal-hash.txt
        │   └── home
        │       └── refold
        │           └── code
        │               └── haskell
        │                   └── cabal
        │                       └── cabal-testsuite
        │                           └── PackageTests
        │                               └── NewBuild
        │                                   └── CustomSetup
        │                                       └── RemotePackageWithCustomSetup
        │                                           └── build-package-from-repo-with-custom-setup.dist
        │                                               └── home
        └── remote-setup-dep-3.0-95e21c2632e40b32212bf1add9fcfe5272e685432265d9d7cc77221afb0d4286
            ├── cabal-hash.txt
            └── lib
                ├── libHSremote-setup-dep-3.0-95e21c2632e40b32212bf1add9fcfe5272e685432265d9d7cc77221afb0d4286.a
                ├── libHSremote-setup-dep-3.0-95e21c2632e40b32212bf1add9fcfe5272e685432265d9d7cc77221afb0d4286-ghc8.2.2.so
                ├── RemoteSetupDep.dyn_hi
                └── RemoteSetupDep.hi

25 directories, 24 files

So remote-pkg-2.0 gets registered, but instead of lib and share under store/remote-pkg-2.0-49b4ea173d672c8399dff7218dd2512ab818030cc9fab93eca633ac6d5885ced/ we get some crazy thing.

23Skidoo · 2017-12-27T02:04:44Z

Cabal/Distribution/Simple/InstallDirs.hs

@@ -286,7 +286,7 @@ absoluteInstallDirs :: PackageIdentifier
                    -> InstallDirs FilePath
 absoluteInstallDirs pkgId libname compilerId copydest platform dirs =
    (case copydest of
-       CopyTo destdir -> fmap ((destdir </>) . dropDrive)
+       CopyTo destdir -> fmap ((destdir </>) . takeBaseName)


I don't think this is right. Here you take InstallDirs and substitute all absolute paths by destdir </> basename absPath. E.g. if libdir was $prefix/lib/$abi/$libname, you make it $destdir/$libname.

Though it seems to me that it's underspecified what copy --destdir should mean. I'd expect the value of --destdir to replace $prefix, but apparently it just gets prepended to the full absolute path including $prefix minus the drive letter.

Looks like there was a --copy-prefix feature that actually worked that way, but it was removed ages ago (981473b).

Hmm thanks! that's a great hint. I've also managed to reproduce it on Linux so hopefully will have a fix today.

Mistuke · 2017-12-28T11:31:49Z

Haven't forgotten this, just taking some time to really understand how cabal constructs all it's paths.

Mistuke · 2018-02-06T09:26:28Z

Hmm so I figured out what's wrong and I fixed the paths, but now cabal seems to think the package has already been registered and skips the registration. It does so because the folder already exists, so It assumes it already registered it. Have to take a closer look at that logic now.

In the mean time, I've also written patches for GHC 8.6 which will fix long path support in the I/O manager.

hvr · 2018-02-06T09:37:09Z

It does so because the folder already exists, so It assumes it already registered it.

did you weaken the transactional assumption that the folder is staged/constructed in a different place, and moved atomically via a single syscall into its target location at once?

Mistuke · 2018-02-06T09:48:25Z

did you weaken the transactional assumption that the folder is staged/constructed in a different place, and moved atomically via a single syscall into its target location at once?

Hmm maybe, I'll take a look tomorrow, but it shouldn't have though. I'm still honouring any CopyTo call, I'm just doing it before it flattens the structure by applying prefix to the existing path. This is where the duplication comes from.

gbaz · 2021-08-12T02:27:02Z

Is this still something worth pursuing given the other longpath support?

Mistuke · 2021-08-13T13:39:06Z

I believe it is, since the long paths support in ghc and toolchain doesn't extend to any random program that may be called by cabal. For instance in a custom preprocessor

Mikolaj · 2021-08-13T16:05:24Z

In that case, what can we do to help rescue this PR?

Mistuke · 2021-08-13T17:03:14Z

I'm not sure how easily this still applies to current master. At the time I was having a hard time tracking down why after the change the folder would be created earlier. So it created a race condition. But I have limited understanding of Cabal's overal inner working for new-build so it lingered..

I guess the first question is if the code can be rebased or if it needs to be rewritten

Mikolaj · 2022-01-29T15:49:55Z

@Mergifyio rebase

mergify · 2022-01-29T15:50:32Z

rebase

❌ Base branch update has failed

Git reported the following error:

Rebasing (1/1)
Auto-merging Cabal/src/Distribution/Simple/InstallDirs.hs
CONFLICT (content): Merge conflict in Cabal/src/Distribution/Simple/InstallDirs.hs
Auto-merging cabal-install/changelog
Auto-merging cabal-install/src/Distribution/Client/ProjectBuilding.hs
CONFLICT (content): Merge conflict in cabal-install/src/Distribution/Client/ProjectBuilding.hs
Auto-merging cabal-install/src/Distribution/Client/Store.hs
CONFLICT (content): Merge conflict in cabal-install/src/Distribution/Client/Store.hs
error: could not apply b5c693fa5... Cabal: Fix path redundancy new-build output folders.
hint: Resolve all conflicts manually, mark them as resolved with
hint: "git add/rm <conflicted_files>", then run "git rebase --continue".
hint: You can instead skip this commit: run "git rebase --skip".
hint: To abort and get back to the state before "git rebase", run "git rebase --abort".
Could not apply b5c693fa5... Cabal: Fix path redundancy new-build output folders.

err-code: 13F15

Mikolaj · 2022-01-29T15:52:57Z

Well, at least it didn't rebase automatically. :(

Volunteers kindly welcome.

Kleidukos · 2023-05-17T06:33:35Z

Marking this PR as draft 🙂

Cabal: Fix path redundancy new-build output folders.

b5c693f

23Skidoo added the blocked: action-required label Dec 27, 2017

23Skidoo reviewed Dec 27, 2017

View reviewed changes

Mistuke mentioned this pull request Feb 6, 2018

Backpack raaz-crypto/raaz#359

Merged

3 tasks

Mistuke mentioned this pull request Jun 17, 2018

Cabal new-build generating insanely long and redundant build PATHS on Windows. #5376

Closed

jneira mentioned this pull request Jul 11, 2018

Error building eta projects with etlas and long paths (around 50) in windows typelead/eta#691

Open

bacchanalia mentioned this pull request Jan 30, 2022

Improve names of generated executables for scripts #7938

Merged

jneira added the platform: windows label Mar 28, 2022

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

ulysses4ever removed the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Sep 3, 2022

Kleidukos marked this pull request as draft May 17, 2023 06:33

ulysses4ever added the attention: needs-rebase label May 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix path redundancy new-build output folders. #4978

Fix path redundancy new-build output folders. #4978

Mistuke commented Dec 26, 2017

Mistuke commented Dec 26, 2017

23Skidoo commented Dec 26, 2017

23Skidoo commented Dec 26, 2017

Mistuke commented Dec 26, 2017

23Skidoo commented Dec 26, 2017

Mistuke commented Dec 26, 2017

23Skidoo commented Dec 27, 2017

23Skidoo Dec 27, 2017

23Skidoo Dec 27, 2017

23Skidoo Dec 27, 2017

Mistuke Dec 27, 2017

Mistuke commented Dec 28, 2017

Mistuke commented Feb 6, 2018

hvr commented Feb 6, 2018 •

edited

Loading

Mistuke commented Feb 6, 2018

gbaz commented Aug 12, 2021

Mistuke commented Aug 13, 2021

Mikolaj commented Aug 13, 2021

Mistuke commented Aug 13, 2021

Mikolaj commented Jan 29, 2022

mergify bot commented Jan 29, 2022

Mikolaj commented Jan 29, 2022

Kleidukos commented May 17, 2023

Fix path redundancy new-build output folders. #4978

Are you sure you want to change the base?

Fix path redundancy new-build output folders. #4978

Conversation

Mistuke commented Dec 26, 2017

Mistuke commented Dec 26, 2017

23Skidoo commented Dec 26, 2017

23Skidoo commented Dec 26, 2017

Mistuke commented Dec 26, 2017

23Skidoo commented Dec 26, 2017

Mistuke commented Dec 26, 2017

23Skidoo commented Dec 27, 2017

23Skidoo Dec 27, 2017

Choose a reason for hiding this comment

23Skidoo Dec 27, 2017

Choose a reason for hiding this comment

23Skidoo Dec 27, 2017

Choose a reason for hiding this comment

Mistuke Dec 27, 2017

Choose a reason for hiding this comment

Mistuke commented Dec 28, 2017

Mistuke commented Feb 6, 2018

hvr commented Feb 6, 2018 • edited Loading

Mistuke commented Feb 6, 2018

gbaz commented Aug 12, 2021

Mistuke commented Aug 13, 2021

Mikolaj commented Aug 13, 2021

Mistuke commented Aug 13, 2021

Mikolaj commented Jan 29, 2022

mergify bot commented Jan 29, 2022

❌ Base branch update has failed

Mikolaj commented Jan 29, 2022

Kleidukos commented May 17, 2023

hvr commented Feb 6, 2018 •

edited

Loading