-
Notifications
You must be signed in to change notification settings - Fork 323
feat(tasks): add custom interpreter support #3929
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
base: main
Are you sure you want to change the base?
Conversation
6004610
to
13b5dee
Compare
a0aef07
to
f627642
Compare
2ffd9c8
to
a71f4bd
Compare
Hey @fecet Can you please add more tests to this feature? Pref testing different shells with the combination of different operating systems, since from rattler-build we had a lot of errors with cmd-bash compatibility on windows during interpreter selections, this would actually make the pr a lot better with confidence in bug-free approach. Also, in rattler-build we have dedicated support with different interpreters, but generally we also could use cmd/bash to reach the PATH of system and installed tools could be used that way too! So this gives both a sense of freedom to use any interpreter through system shells, but also have dedicated support for nushell/bash/cmd/rscript etc. I think going with this approach could be nice yeah. Also we could avoid skipping parsing the script and just directly dedicate to interpreter as well, this would reduce overhead and we could make it faster! |
In general, the interpreter can be run without arguments and interpret what arrives on stdin (and write to stdout) using pipes. There may be certain cases where the interpreter also needs some arguments. [tasks.my-task]
cmd = "ls | where size > 1MB"
interpreter = "nu"
args = [ "--execute", "print 'Big Files'"] Where are the interpreters acquired?
|
3626705
to
6c7f612
Compare
Looking at the tests I see that it preserves the previous deno-shell behavior. cmd: The command being interpreted. interpreter: (new) The command (what processes this command?) actually run as an interpreter, it must read from stdin and write to stdout. args: (new) Some named arguments substituted (with minijinja?) into the cmd and interpreter elements. envs: (new) Set some environment parameters. The "interpreter" is limited to providing the command to start the interpreter. It was not immediately obvious to me that the tests effectively write a custom interpreter in python. |
Would it be helpful for pixi-tasks themselves to be interpreters. In the tests you can find: # Test complex interpreter that removes spaces from input
manifest_content["tasks"] = {
"remove-spaces-blackbox": {
"interpreter": '''python -c "import sys; data=sys.stdin.read(); sys.stdout.write(data.replace(' ',''))"''',
},
"remove-spaces": {"cmd": "echo 'hello world' | pixi run remove-spaces-blackbox"},
} I think the "interpreter" would be better as an array as that would make the interpretation of the interpreter unnecessary. (Direct use of https://doc.rust-lang.org/std/process/struct.Command.html rather than spawning a command processor first.) manifest_content["tasks"] = {
"remove-spaces-blackbox": {
"interpreter": ["python", "-c", '''
import sys
data=sys.stdin.read()
sys.stdout.write(data.replace(' ',''))
''']
},
"remove-spaces":
"cmd": "echo 'hello world' | remove-spaces-blackbox",
"interpreter": ["deno"]
},
} If tasks could be interpreters then could it could become?: manifest_content["tasks"] = {
"remove-spaces-interpreter": {
"cmd": '''
import sys
data=sys.stdin.read()
sys.stdout.write(data.replace(' ',''))
''',
"interpreter": ["python"]
},
"remove-spaces-blackbox": {
"interpreter": ["remove-spaces-interpreter"]
},
"remove-spaces":
"cmd": "echo 'hello world' | remove-spaces-blackbox",
"interpreter": ["deno"]
},
} |
6c7f612
to
b0211da
Compare
Thanks for driving this discussion and for all the detailed reviews so far. I’ve gone back through the thread, and here are my thoughts: The existing tests were really just proof-of-concepts to show how you could chain multiple If we decide that these advanced features deserve official, ergonomic support, the best path is for the Pixi core maintainers to define exactly what the API should look like:
|
I’ve just pushed an expanded test suite but I’m not deeply familiar with macOS or Windows internals, so if you spot any gaps, please let me know @zelosleone |
I agree. My main concern is that the "interpreter" is unnecessarily complex. interpreter = '''python -c "import ..."''' Which necessitates the use of 'deno' or something like it to parse. interpreter = ["python", "-c", '''import ...'''] The list would be the command and its arguments to https://doc.rust-lang.org/std/process/struct.Command.html That seems like the opposite of "deeply baking" to me. Note: The issue about interpreter chains is definitely a separate topic:
|
@phreed, you’re right to flag that concern, but for now the interpreter field will primarily hold single strings like |
I made a PR to show what might be entailed in making the changes mentioned. |
Here are some reason why this change might be wanted: Python Buffered/Unbuffered stdoutIf the interpreter is ["python","-u"] rather than just "python" (or equivalently ["python"]) the progress of the process can be monitored. The project may be an interpreterIt is pretty common for an application to be an interpreter. |
thanks for your work! I've reviewed it and left some small comments! |
5105e9a
to
3954fd8
Compare
3954fd8
to
78062c8
Compare
a2a74d8
to
e79bbca
Compare
3b46457
to
6ae3959
Compare
Happy to announce that it mostly works. A few missing parts:
But most importantly, while I got excited that it worked, I found myself replacing:
with:
Basically replacing the complexity of This makes it a hard feature to promote for myself. Could you tell me why you think it's better that Pixi handles this extra behavior instead of just writing it out? |
0a33ef9
to
b541cc0
Compare
For simple cases the 'interpreter' certainly seems like overkill. Personally, I find the evaluation of the interpreter by deno-shell to be mystical (preferring the https://doc.rust-lang.org/std/process/struct.Command.html). |
83eafbe
to
348f86f
Compare
348f86f
to
92cd61f
Compare
I'd like to add one more point: it depends on whether the pixi task intends to unify most command runner operations. Personally, I don't want to be forced to introduce just as a dependency and migrate most pixi tasks to just once the project reaches a certain stage — that has happened multiple times before. |
Fixes #1844
Motivation
The current pixi task execution relies on
deno_task_shell
, which while powerful, has several limitations:deno_task_shell
doesn't support complex conditional statements likeif-then-else
structures${var:-default}
and other bash-specific featuresTo address these limitations, we introduce custom interpreter support, allowing users to specify particular shells (such as bash, sh, nushell, etc.) to execute tasks.
Implementation Details
Core Changes
interpreter
field to task definitionsTechnical Implementation
1. Task Definition Extension
2. Execution Flow
{interpreter} <<< {processed_command}
(by using deno_task_shell::execute_with_pipes)3. Template Processing Pipeline
Usage Examples
CLI Usage