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

Improve names of generated executables for scripts #7938

Merged
merged 3 commits into from
Jan 31, 2022

Conversation

bacchanalia
Copy link
Collaborator

The motivation for this PR is that previously all executables generated for scripts were named "script". This was confusing from a process management standpoint.

Ideally the name of the generated executable would be identical to the name of the script, but currently the executable name is generated from the executable section in the generated .cabal file, and there is a limited character set that is legal for that name. Since there's no easy way to use the script name directly, I replace the illegal characters with '_'. Also on @jneira's suggestion, I prefix the name with "cabal-script-" to make clear to end users that it is a generated name.

It might be possible to detect it's a script on the the Cabal side and set things there appropriately to avoid the name mangling, but I don't think the added complexity is worth it.

This change already effects existing tests for scripts, and therefore does not need a standalone test.
I do not believe the change effects any exiting documentation.

@bacchanalia bacchanalia force-pushed the better-script-exe-names branch from cf6c25c to fe556df Compare January 28, 2022 19:15
@jneira jneira requested review from jneira and fendor January 28, 2022 19:34
@bacchanalia
Copy link
Collaborator Author

bacchanalia commented Jan 28, 2022

It looks like tests are failing on Windows to the file path length limit, which is by default 260, but some build products have path length 262.

@Mikolaj
Copy link
Member

Mikolaj commented Jan 28, 2022

That's a bummer. I wonder if this can be remedied. Could you shorten the paths in your test for now?

@bacchanalia
Copy link
Collaborator Author

@Mikolaj yeah, I can shave 21 chars off pretty easily

@bacchanalia bacchanalia force-pushed the better-script-exe-names branch 2 times, most recently from a98de68 to 28301bf Compare January 28, 2022 22:39
@jneira
Copy link
Member

jneira commented Jan 28, 2022

Also on @jneira's suggestion, I prefix the name with "cabal-script-" to make clear to end users that it is a generated name.

We talked that it could help to reach that limit, but windows users are used to use short paths with haskell nevertheless.
But it exploded in tests 🤦

Maybe it is time to cut off the hash from the script binary full path (if you are not already doing it)?

@Mikolaj
Copy link
Member

Mikolaj commented Jan 29, 2022

The prefix sounds useful. I think @bacchanalia is cutting corners only in tests, not in the actual code. Otherwise, we'd need to sit and google and ponder how to fix this properly.

@bacchanalia
Copy link
Collaborator Author

bacchanalia commented Jan 29, 2022

I'm getting the same error now, despite shortening the path to well under 260, so it seems the problem is something else.

edit: Could still be "D:\a\cabal\cabal\cabal-testsuite\PackageTests\NewBuild\CmdRun\ScriptNoExtention\cabal.dist\work\.\dist\build\x86_64-windows\ghc-8.6.5\fake-package-0\x\cabal-script-with_space\build\cabal-script-with_space\cabal-script-with_space-tmp\Main.o" if it's actually passed with the double back slashes.

@bacchanalia
Copy link
Collaborator Author

It is now confirmed to have been path length, since last patch fixed it.

@bacchanalia
Copy link
Collaborator Author

@jneira The hash doesn't even appear in the path that was too long.

Copy link
Collaborator

@fendor fendor left a comment

Choose a reason for hiding this comment

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

This is a rather disgusting hack, we should have a better definition of cabal-syntax, but that's well out of scope here. Looks like a clear improvement to me, thank you!

Copy link
Member

@jneira jneira left a comment

Choose a reason for hiding this comment

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

lgtm, thanks for this nice refinement to your main patch

@bacchanalia
Copy link
Collaborator Author

@fendor I assume you mean the censor function? I agree but I couldn't find a clear definition of the legal character set anywhere, so that's what I came up with.

@bacchanalia
Copy link
Collaborator Author

Before I hit "merge_me" do you think this is could cause trouble for other people working on cabal on Windows, because they're base dir is any higher up they're going to get a test failure when they try to running tests? It might be better to wait for #4978 for that reason.

@fendor
Copy link
Collaborator

fendor commented Jan 30, 2022

I couldn't find a clear definition of the legal character set anywhere

Neither can I, but there definitely is somewhere. And if not, there should be!

@bacchanalia
Copy link
Collaborator Author

bacchanalia commented Jan 30, 2022

@fendor Looking again I found the definition in "templates/Lexer.x", so I'm going to rewrite censor to use the definition of namecore transcribed from there.

Copy link
Collaborator

@fendor fendor left a comment

Choose a reason for hiding this comment

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

Much better!

@bacchanalia bacchanalia added the merge me Tell Mergify Bot to merge label Jan 31, 2022
@jneira jneira force-pushed the better-script-exe-names branch from 7acf297 to e13d214 Compare January 31, 2022 14:47
@mergify mergify bot merged commit 73a343b into haskell:master Jan 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge me Tell Mergify Bot to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants