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

Try using smaller package ids on Windows (see #3430) #3431

Merged
merged 1 commit into from
May 17, 2016

Conversation

dcoutts
Copy link
Contributor

@dcoutts dcoutts commented May 14, 2016

On Windows we have serious problems with path lengths (see #3430).

Windows imposes a maximum path length of 260 chars, and even if we
can use the windows long path APIs ourselves, we cannot guarantee
that ghc, gcc, ld, ar, etc etc all do so too.

So our only choice is to limit the lengths of the paths, and the only
real way to do that is to limit the size of the 'InstalledPackageId's
that we generate. We do this by truncating the package names and
versions and also by truncating the hash sizes.

Truncating the package names and versions is technically ok because they
are just included for human convenience, the full source package id is
included in the hash.

Truncating the hash size is disappointing but also technically ok. We
rely on the hash primarily for collision avoidance not for any securty
properties (at least for now).

On Windows we have serious problems with path lengths. Windows imposes a
maximum path length of 260 chars, and even if we can use the windows
long path APIs ourselves, we cannot guarantee that ghc, gcc, ld, ar, etc
etc all do so too.

So our only choice is to limit the lengths of the paths, and the only
real way to do that is to limit the size of the 'InstalledPackageId's
that we generate. We do this by truncating the package names and
versions and also by truncating the hash sizes.

Truncating the package names and versions is technically ok because they
are just included for human convenience, the full source package id is
included in the hash.

Truncating the hash size is disappointing but also technically ok. We
rely on the hash primarily for collision avoidance not for any securty
properties (at least for now).
@dcoutts
Copy link
Contributor Author

dcoutts commented May 14, 2016

This will require a little bit of testing on windows from someone.

truncateHash (HashValue h) = HashValue (BS.take 20 h)

-- Truncate a string, with a visual indication that it is truncated.
truncateStr n s | length s <= n = s
Copy link
Member

@hvr hvr May 15, 2016

Choose a reason for hiding this comment

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

minor nitpick: length (truncateStr 0 "x") /= 0 :-)

Copy link
Contributor Author

@dcoutts dcoutts May 15, 2016

Choose a reason for hiding this comment

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

Right it will not work for 0, since it has to have a visual indication of the truncation. It's only a local function and we're using it with >0 values so... erm... so nitpick be gone! :-)

@23Skidoo
Copy link
Member

What kinds of tests need to be done? Are automatic tests on AppVeyor not enough?

@dcoutts
Copy link
Contributor Author

dcoutts commented May 15, 2016

@23Skidoo tbh, I'm not sure. The appveyor tests were not enough before to detect this problem. It will vary with user name, ghc-version, and various other things that affect the install paths.

@hvr you noticed it. What would you suggest?

@23Skidoo
Copy link
Member

@dcoutts BTW, I've enabled integration-tests2 on AppVeyor and they seem to fail on Windows: https://ci.appveyor.com/project/23Skidoo/cabal/build/%23830%20%28master%29

Can you look into it?

@dcoutts
Copy link
Contributor Author

dcoutts commented May 15, 2016

@23Skidoo looks like the same issue as on unix. I pushed a fix in #3432. We'll see if the AppVeyor build is happy.

@dcoutts
Copy link
Contributor Author

dcoutts commented May 15, 2016

@23Skidoo so that failure was unrelated, so still need some test case so we can check if this fixes things.

@dcoutts
Copy link
Contributor Author

dcoutts commented May 15, 2016

@23Skidoo so borrowing @hvr's windows box, I can reproduce a failure with vector-binary-instances-0.2.3.2 and then it works with this change.

Failure:

Installing
dist\build\libHSvector-binary-instances-0.2.3.2-a90e0fc22ad57edb020bdbc183d5459aacf94f530e68d54eb5e0e1190af52e39.a
to
C:\Users\hvr\AppData\Roaming\cabal\store\ghc-8.0.0.20160421\vector-binary-instances-0.2.3.2-a90e0fc22ad57edb020bdbc183d5459aacf94f530e68d54eb5e0e1190af52e39\lib\libHSvector-binary-instances-0.2.3.2-a90e0fc22ad57edb020bdbc183d5459aacf94f530e68d54eb5e0e1190af52e39.a
copyFile: does not exist (The system cannot find the path specified.)

becomes the successful

Installing
dist\build\libHSvector-binary_-0.2.3.2-27361c87867534e4c5bfbb97fada20a145c94824.a
to
C:\Users\hvr\AppData\Roaming\cabal\store\ghc-8.0.0.20160421\vector-binary_-0.2.3.2-27361c87867534e4c5bfbb97fada20a145c94824\lib\libHSvector-binary_-0.2.3.2-27361c87867534e4c5bfbb97fada20a145c94824.a

That long path goes from length 264 to 198.

So we could add an integration test with "a-totally-outrageous-and-unreasonably-long-package-name-0.0.0.1"

@dcoutts
Copy link
Contributor Author

dcoutts commented May 15, 2016

So actually it's a bit hard to add an integration test for this yet since it needs to install into the store, and currently that is only done for packages from hackage. The plan is to support installing to the store also for local tarball packages, so once we have that support then we can more easily make a test like this.

@23Skidoo
Copy link
Member

23Skidoo commented May 16, 2016

Can we have a ticket about adding a test for this? OK, I see that you added it to #3104.

Otherwise LGTM.

@dcoutts
Copy link
Contributor Author

dcoutts commented May 17, 2016

Ok.

@dcoutts dcoutts merged commit d28d230 into haskell:master May 17, 2016
@hvr
Copy link
Member

hvr commented Aug 31, 2016

@23Skidoo was this ever backported to the 1.24 branch?

@23Skidoo
Copy link
Member

23Skidoo commented Sep 1, 2016

@hvr Nope.

ezyang added a commit that referenced this pull request Oct 8, 2016
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