Skip to content
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

Add support on Windows for copying to long paths. #4977

Merged
merged 3 commits into from
Dec 26, 2017

Conversation

Mistuke
Copy link
Collaborator

@Mistuke Mistuke commented Dec 26, 2017

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 by trying to compile cabal itself on a long path.

Should fix partially #3972, #4914, #4515. The reason why partial is because any operation that hits the GHC I/O manager will destroy device path support. Because the GHC I/O manager is using non-native posix like APIs which are mapped to the old Win32 APIs that are bound by MAX_PATH.

This patch works because it avoids touching the I/O manager for doing the file copy and instead does a direct copy using the Win32 API. The Windows APIs do not have the same limitations as the posix ones and so do not require the file to be on the same drive before issuing a copy/move. So this should be much faster too.

@Mistuke
Copy link
Collaborator Author

Mistuke commented Dec 26, 2017

cc @alexbiehl , @hvr

@23Skidoo
Copy link
Member

Do you reckon it'd make sense to implement a compat filepath layer that'd use extended length paths and native operations on Windows, and standard operations (System.Directory and friends) otherwise?

Copy link
Member

@23Skidoo 23Skidoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@@ -25,20 +27,36 @@ import System.IO.Error
import System.Directory
( doesFileExist, renameFile, removeFile )
import System.FilePath
( takeDirectory )
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This breaks the build on non-Windows, where takeDirectory is still used, see e.g. https://travis-ci.org/haskell/cabal/jobs/321706530#L778.

@Mistuke
Copy link
Collaborator Author

Mistuke commented Dec 26, 2017

@23Skidoo Well, System.Directory mostly does the right thing, it does convert to device paths and calls out to Win32 which calls the proper APIs. It's specifically copyFile that's problematic because of the limitations of the posix calls.

I only quickly glanced but I don't believe the other functions use the I/O manager and so should be fine. A "proper" fix would be a new I/O manager in GHC/base. I'm working towards an implementation, but it's a big task so not sure I'll finish by 8.6. But somewhere in the next two releases it should be done and then Cabal and cabal-install should just work without any changes.

@23Skidoo
Copy link
Member

Build still fails on Linux, now due to -Werror:

Distribution/Compat/CopyFile.hs:145:1:
    Warning: Defined but not used: `toExtendedLengthPath'

@23Skidoo 23Skidoo merged commit 0858ebb into haskell:master Dec 26, 2017
@23Skidoo
Copy link
Member

Merged, thanks!

@Mistuke Mistuke deleted the fix-windows-long-paths branch December 26, 2017 19:45
@BardurArantsson
Copy link
Collaborator

Great work! Especially the bit about trying to upstream into base... :)

(Not that I'm using Windows, personally, but this has been a pain for Cabal users and package developers who want "cross-platform-by-default" for quite a while...)

@Mistuke Mistuke mentioned this pull request Feb 6, 2018
3 tasks
@Mistuke
Copy link
Collaborator Author

Mistuke commented Mar 31, 2018

For future reference, this workaround is no longer need when cabal is compiled with GHC 8.6. See ghc/ghc@4de585a

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants