fix quoted commands not being parallelizable #744
Conversation
New PaSh version
|
OS = |
|
OS:ubuntu-20.04 |
|
OS = |
|
OS:ubuntu-20.04 |
There was a problem hiding this comment.
Thanks for the fix and for adding a test!
In addition to the change that I added in the text, it would also be great if you could rebase this to the future branch, since we always push new changes to future and then later push them to main everytime we do a release!
| @@ -66,8 +66,10 @@ def parse_arg_list_to_command_invocation( | |||
| command, flags_options_operands | |||
| ) -> CommandInvocationInitial: | |||
| cmd_name = format_arg_chars(command) | |||
| #strip quotes from command where necessary | |||
| #quoted commands are interpreted as non-parallelizable | |||
| cmd_name = cmd_name.strip().strip('"').strip("'") | |||
There was a problem hiding this comment.
I think it is better to do this change by making a new function replacing format_arg_chars since that function has access to the actual character codes and not just the string, making any transformation more robust! At this point in the execution, it should certainly be the case that command only is expanded and only has quotes (Q characters), normal (C), and escaped (E) characters I think. So we can make new function called remove_quotes_expanded_arg_chars that:
- First checks if the word is indeed expanded, only containing the characters above (to do this we need to traverse the whole argument hierarchically.
- Then removes the outer layers of quotes (if any).
There was a problem hiding this comment.
I attempted this in my latest commit but seems to be breaking some tests. Not sure if I am making an obvious error
kwakubiney
force-pushed
the
fix/quoted-commands-not-parallelizable
branch
from
17a4a54 to
6e30d15
Compare
|
OS = |
|
OS:ubuntu-20.04 |
kwakubiney
force-pushed
the
fix/quoted-commands-not-parallelizable
branch
from
6e30d15 to
145c130
Compare
|
OS = |
|
OS:ubuntu-20.04 |
|
OS = |
|
OS:ubuntu-20.04 |
kwakubiney
force-pushed
the
fix/quoted-commands-not-parallelizable
branch
2 times, most recently
from
f8e0d06 to
145c130
Compare
* Fix typo Signed-off-by: Bolun Thompson <[email protected]> * Add check that compile_ast result is an IR Signed-off-by: Bolun Thompson <[email protected]> * Add return type hint to compile_node Signed-off-by: Bolun Thompson <[email protected]> --------- Signed-off-by: Bolun Thompson <[email protected]>
* Fix: inform_daemon_exit on assertion exit Otherwise, --assert_compiler_success hangs. --assert_all_regions_parallelizable doesn't fail, but I also copied the change there. The tests using the flag pass. Signed-off-by: Bolun Thompson <[email protected]> * Fix: correctly parse "is parallelizable" output Signed-off-by: Bolun Thompson <[email protected]> --------- Signed-off-by: Bolun Thompson <[email protected]>
* Fix double-exit bug in parallel pipelines Signed-off-by: Bolun Thompson <[email protected]> * Assert running_procs is positive Signed-off-by: Bolun Thompson <[email protected]> * Fix: don't use exit handler The reason why it worked on Ubuntu 20.04 was that the EXIT handler was not called, while on Ubuntu 24.04 it correctly is. Signed-off-by: Bolun Thompson <[email protected]> * Add comment explanation Signed-off-by: Bolun Thompson <[email protected]> --------- Signed-off-by: Bolun Thompson <[email protected]>
Signed-off-by: kwakubiney <[email protected]>
Signed-off-by: kwakubiney <[email protected]>
kwakubiney
force-pushed
the
fix/quoted-commands-not-parallelizable
branch
from
145c130 to
5301786
Compare
commands to be parallelizable Signed-off-by: kwakubiney <[email protected]>
kwakubiney
force-pushed
the
fix/quoted-commands-not-parallelizable
branch
from
5301786 to
4afd1b9
Compare
Signed-off-by: kwakubiney <[email protected]>
|
OS = |
|
OS:ubuntu-20.04 |
|
OS:ubuntu-24.04 |
Attempt to fix #741