-
Notifications
You must be signed in to change notification settings - Fork 314
feat: add custom interpreter support for pixi tasks (close #1844) #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. |
e769f42
to
b2eeb5e
Compare
@phreed @pavelzw The current interpreter accepts input in the form of template strings and uses temfile approach similar to those in GitHub workflow shells. I think this should be general enough. Feel free to ask me to add more tests. |
774a209
to
93981a6
Compare
94d0fed
to
d6b8001
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those changes look good.
The valid test set looks sufficient.
The invalid test set could be expanded but I think that should be discovered based on usage.
As mentioned before:
- I think the complexity of using deno_task_shell could be confusing in some cases.
- deno_task_shell behavior needs to be preserved for backwards compatibility.
2240833
to
5105e9a
Compare
I did a quick check to see how rattler-build handles the cmd (stdin vs. tempfile). |
thanks for your work! I've reviewed it and left some small comments! |
5105e9a
to
3954fd8
Compare
3954fd8
to
78062c8
Compare
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