Fix path redundancy new-build output folders.#4978
Fix path redundancy new-build output folders.#4978Mistuke wants to merge 1 commit intohaskell:masterfrom
Conversation
|
cc @alexbiehl , @hvr |
|
Awesome, I was just bitten by this the other day when trying to build |
|
Looks like there's a test failure on Linux. /cc @dcoutts |
|
Hmm indeed, it seems to be skipped on a lot of configuration. But the error is a bit odd.. But the log shows it registering the package.. Is there a way for me to run this test locally on Windows? |
|
@Mistuke See |
|
@23Skidoo Thanks, I'll boot to Linux and give it a try. |
|
Some clues to why that test is failing: So |
| absoluteInstallDirs pkgId libname compilerId copydest platform dirs = | ||
| (case copydest of | ||
| CopyTo destdir -> fmap ((destdir </>) . dropDrive) | ||
| CopyTo destdir -> fmap ((destdir </>) . takeBaseName) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Looks like there was a --copy-prefix feature that actually worked that way, but it was removed ages ago (981473b).
There was a problem hiding this comment.
Hmm thanks! that's a great hint. I've also managed to reproduce it on Linux so hopefully will have a fix today.
|
Haven't forgotten this, just taking some time to really understand how cabal constructs all it's paths. |
|
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. |
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 |
|
Is this still something worth pursuing given the other longpath support? |
|
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 |
|
In that case, what can we do to help rescue this PR? |
|
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 |
|
@Mergifyio rebase |
❌ Base branch update has failedDetailsGit reported the following error: err-code: 13F15 |
|
Well, at least it didn't rebase automatically. :( Volunteers kindly welcome. |
mergify
bot
added
the
merge delay passed
ulysses4ever
removed
the
merge delay passed
|
Marking this PR as draft 🙂 |
|
Hello, I am going through old PRs to check whether they are stale. If this PR is still “live”, write a comment and I will remove the |
9 participants
Please include the following checklist in your PR:
[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.
Fixes #4515, #3972.
The
setupcall withcopyCommanddoesn't do what the comment inProjectBuilding.hssuggested. It literally only assigns the path you specified. So you can choose any format you want as long as you pass it tocopyCommand. So instead of using@$tmpDir/$prefix@we now use@$tmpDir/$packageName-$version@. Becauseprefixis a prefix totmpDirthis means that before this patch cabal was halving the Windows MAX_PATH. Effectively meaningnew-buildwould only work on paths shorter than 125 characters.The consistency is maintained with
absoluteInstallDirsby also taking thebase nameinstead of dropping thedrivewhen combing the paths. Proof that this is all consistent is that the build succeeds. Cabal only issuescreateDirectorycalls 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.