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

Shorten script-builds paths #8898

Merged
merged 5 commits into from
May 24, 2023
Merged

Conversation

bacchanalia
Copy link
Collaborator

@bacchanalia bacchanalia commented Apr 5, 2023

  • Use Base64 hash truncated to 26 chars for script-build cache directories.
  • Use the cache directory as the dist directory.
  • Use script- as the component name instead of cabal-script-<...>.
  • Use cabal-script- for the executable name.
  • This change is incompatible with previous cabal versions in terms of cache location,
    you should manually remove your old caches once you no longer need them.

Bug #8841

@Mikolaj
Copy link
Member

Mikolaj commented Apr 5, 2023

Let me repeat from the bug ticket: Could a kind Windows user test if this suffices in any standard Windows install? Alternatively, could somebody tweak our CI not to shorten the path by setting CABAL_DIR to something unnaturally short?

@bacchanalia
Copy link
Collaborator Author

The other low hanging fruit for saving chars is removing the cabal-script- prefix for script names, which would save length "cabal-script-" * 3 == 39. The rational for adding it is explained in #7938, but the extra chars might be more precious? or a compromise could be to replace cabal-script- with cabal! to save 21 chars. cc @jneira

@bacchanalia
Copy link
Collaborator Author

Another thing we probably want in this PR is detecting old-style script-build directories and relocating them.

@jneira
Copy link
Member

jneira commented Apr 5, 2023

@bacchanalia thanks for the fix
well we could use simply script-, no?
it is inside the Cabal config dir so there is no confusion possible iirc

@ulysses4ever
Copy link
Collaborator

I didn't realize this happens inside cabal dir. In that case, I'd propose to go bold and use s- and possibly manually truncate the hash to something like 7 positions.

@jneira
Copy link
Member

jneira commented Apr 6, 2023

hmm maybe too much, not sure, I would want to try with different package+executable names if they are included in the path

@Kleidukos
Copy link
Member

During this week's maintainers meeting we agreed that the prefix was a good idea and that we ought to be careful about trimming the hash in a way that can provoke collision.

@bacchanalia
Copy link
Collaborator Author

re: prefix - I think I have a good solution for this, which is to use a short fixed name for the executable, and then make a symlink to it with the (prefixed?) unmangled name of the script
re: trimming the hash - 7 chars (35 bits) feels uncomfortably short to me, but the full 256 bit hash seems like overkill. how do people feel about using 26 chars (130 bits, about the same as a 128bit uuid)?

@gbaz
Copy link
Collaborator

gbaz commented Apr 6, 2023

26 sounds like a good trade-off.

@jneira
Copy link
Member

jneira commented Apr 7, 2023

iirc symlinking is not reliable in all windows systems with default configuration 😔

@bacchanalia bacchanalia force-pushed the 8841-long-paths branch 3 times, most recently from c5ec8ee to ab6b854 Compare April 9, 2023 21:22
@bacchanalia bacchanalia changed the title Use Base64 hash for script-builds directories Shorten script-builds paths Apr 9, 2023
@bacchanalia
Copy link
Collaborator Author

This should be ready for review now.

@bacchanalia
Copy link
Collaborator Author

actually, thought of one more thing I should fix

@bacchanalia bacchanalia force-pushed the 8841-long-paths branch 2 times, most recently from 79e804b to 9adeffd Compare April 9, 2023 23:45
@ulysses4ever
Copy link
Collaborator

Has anyone confirmed that this actually fixes the long path problem on Windows? (Recall that CI is unable to check this because it sets CABAL_DIR to something short explicitly.)

@Kleidukos
Copy link
Member

@bacchanalia Would you mind writing down some manual QA instructions so that people may ensure that there is no regression before the next release? :)

@bacchanalia
Copy link
Collaborator Author

@Kleidukos I've been traveling, and my personal laptop died, but I should be able to get back to this by next week hopefully.

@mazuschlag
Copy link

Hello, I can be the manual QA for this if it's needed!

@Kleidukos
Copy link
Member

Kleidukos commented May 3, 2023

@mazuschlag We'd be delighted! :) The PR hasn't been merged yet but feel free to try it once it has been merged.

Copy link
Member

@Mikolaj Mikolaj left a comment

Choose a reason for hiding this comment

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

Thank you.

@Mikolaj
Copy link
Member

Mikolaj commented May 22, 2023

@bacchanalia: could you kindly set the merge_me or squash+merge_me label?

@Mikolaj
Copy link
Member

Mikolaj commented May 22, 2023

@mergify backport 3.10

@mergify
Copy link
Contributor

mergify bot commented May 22, 2023

backport 3.10

✅ Backports have been created

@Kleidukos Kleidukos added squash+merge me Tell Mergify Bot to squash-merge and removed attention: needs-review labels May 22, 2023
@mergify mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label May 24, 2023
@Mikolaj
Copy link
Member

Mikolaj commented May 24, 2023

@mergify refresh

@mergify
Copy link
Contributor

mergify bot commented May 24, 2023

refresh

✅ Pull request refreshed

@Mikolaj
Copy link
Member

Mikolaj commented May 24, 2023

@mergify rebase

Using a Base64 hash and truncating it to 26 characters, saves 38 chars,
which helps avoid long paths issues on Windows, while still providing
130 bits of hash in order to avoid collisions.

Bug haskell#8841
Putting script build products under dist-newstyle within the cache
directory is unnecessary because we already control the cache directory
and can ensure there are no conflicts.
Previously, the script name was sanitized in final executable name,
because the executable name had to match the component name, which only
allowed for a limited character set. Now we can use the actual script
name in the executable name. This only lets us shorten the component
name without losing clarity.
@mergify
Copy link
Contributor

mergify bot commented May 24, 2023

rebase

✅ Branch has been successfully rebased

@mergify mergify bot merged commit a482a63 into haskell:master May 24, 2023
44 checks passed
mergify bot pushed a commit that referenced this pull request May 24, 2023
* Use shorter hash for script-builds directories

Using a Base64 hash and truncating it to 26 characters, saves 38 chars,
which helps avoid long paths issues on Windows, while still providing
130 bits of hash in order to avoid collisions.

Bug #8841

* Use the script cache dir as the dist dir

Putting script build products under dist-newstyle within the cache
directory is unnecessary because we already control the cache directory
and can ensure there are no conflicts.

* Use the actual script name in the executable name

Previously, the script name was sanitized in final executable name,
because the executable name had to match the component name, which only
allowed for a limited character set. Now we can use the actual script
name in the executable name. This only lets us shorten the component
name without losing clarity.

* Add changelog entry

* Reenable script tests for Windows/ghc-9.4.*

(cherry picked from commit a482a63)

# Conflicts:
#	cabal-install/src/Distribution/Client/CmdRun.hs
#	cabal-install/src/Distribution/Client/ProjectConfig.hs
@Mikolaj
Copy link
Member

Mikolaj commented May 24, 2023

Uhoh, the automatic backport to branch 3.10 failed. Any volunteer to push this through? Manual backport and merge conflict resolution is going to be required.

@Mikolaj
Copy link
Member

Mikolaj commented May 24, 2023

Supposedly only two files have conflicts, from a refactoring missing on 3.10, from the looks of it. Probably rather easy to resolve (disclaimer: backports I described by these very words are still WIP after months of work by heroic developers).

@jneira
Copy link
Member

jneira commented May 24, 2023

Hi, not sure if this is the place to add qa notes but i tried cabal-head with this pr and it seems it works and intended:

PS D:\dev\ws\haskell\cabal-test\scripts> cabal-3.10.1.0.exe run .\say.hs --ignore-project
Hello World

PS D:\dev\ws\haskell\cabal-test\scripts> cabal-3.10.1.0.exe run .\say123456789.hs --ignore-project
C:\Users\atrey\AppData\Roaming\cabal\script-builds\d0229a0b2f3659d6f0da31e181ad13612b054b6add2a16b4d6852550a3963b17\dist-newstyle\build\x86_64-windows\ghc-8.10.7\fake-package-0\x\cabal-script-say123456789.hs\build\cabal-script-say123456789.hs\autogen\: openBinaryTempFileWithDefaultPermissions: invalid argument (invalid argument
)

PS D:\dev\ws\haskell\cabal-test\scripts>  D:\bin\cabal\cabal.exe run .\say1234567890123456789.hs --ignore-project
Warning: this is a debug build of cabal-install with assertions enabled.
Hello World

PS D:\dev\ws\haskell\cabal-test\scripts>  D:\bin\cabal\cabal.exe run .\say12345678901234567890123456789.hs --ignore-project
Warning: this is a debug build of cabal-install with assertions enabled.
realgcc.exe: error: C:\Users\atrey\AppData\Roaming\cabal\script-builds\kp1mliqmakzJzmCQa+HPYgM9dB\build\x86_64-windows\ghc-8.10.7\fake-package-0\x\script-say12345678901234567890123456789.hs\build\script-say12345678901234567890123456789.hs\script-say12345678901234567890123456789.hs-tmp\Main.o: No such file or directory
`gcc.exe' failed in phase `Linker'. (Exit code: 1)

so we have alliviated the issue 😄

ulysses4ever added a commit that referenced this pull request May 30, 2023
Co-authored-by: Mel Zuser <mel@mzero.dev>
Co-authored-by: Artem Pelenitsyn <a.pelenitsyn@gmail.com>
@ffaf1 ffaf1 mentioned this pull request Jun 1, 2023
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
attention: needs-manual-qa PR is destined for manual QA merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days platform: windows squash+merge me Tell Mergify Bot to squash-merge
Projects
Status: Testing Approved
Development

Successfully merging this pull request may close these issues.

None yet

8 participants