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

Add option to reuse shell #1115

Open
richard-hajek opened this issue Apr 9, 2023 · 18 comments · Fixed by #1202
Open

Add option to reuse shell #1115

richard-hajek opened this issue Apr 9, 2023 · 18 comments · Fixed by #1202

Comments

@richard-hajek
Copy link

richard-hajek commented Apr 9, 2023

I think it would be beneficial to allow for execution of an entire task in a single shell.

Motivation

It is currently very difficult to use Python's venvs or conda, due to the shell being reset on each command. The recommended way is to use absolute paths inside the venv, such as venv/bin/python instead of source venv/bin/activate;python. This however increasing the friction for newcomers and not all problems of this kind have similar solutions.

My current Taskfile, looks like this:

[...]
tasks:
  build:
    cmds:
      - mamba env create -f environment.yaml || true
      - mamba env update -f environment.yaml --prune
      - bash -c "
        source conda_init;
        mamba activate csidrl;
        poetry install;
        cd lib/SATNet;
        python setup.py install;
        "

Which is ugly and complicated. This approach also complicates cross-platform implementations.

There are other workarounds, such as in #512. There would be other uses of this feature, for example resolving #204.

Proposal

For a task, add subshell boolean. This boolean would be by default true, ensuring compatibility. On false, this option would make go-task invoke all cmds in a single mvdan.cc/sh/v3/interp. When false, this option would also cause all task: cmds to run in the same interp.

The unclear question is a treatment of deps. The current behavior would see them running in different shells, which may produce some confusion. The consistent behavior with the rest of this proposal would however sacrifice concurrency for simplicity and make deps run linearly after each other in the same shell (when subshell: false only). This behavior would also resolve #204. Neither of these options would result in consistent behavior throughout the go-task though.

Advantages

My humble opinion

I believe that each of the advantages by themselves would be a strong-enough for this feature to be accepted, but there are some issues.

Even though this feature may simplify some workflows, the added complexity may not be worth it. The code base as I understand it also wildly doesn't support this, so it would be a major change. I am willing to take a crack at it though.

Neither of these options would result in consistent behavior throughout the go-task though.

In my personal opinion, both options are better than the current behavior.

@task-bot task-bot added the state: needs triage Waiting to be triaged by a maintainer. label Apr 9, 2023
@pd93
Copy link
Member

pd93 commented Apr 9, 2023

I've never fully understood the appeal of putting a full shell script inside of a Taskfile. I understand it for simple 1/2 line cases, but as soon as you're doing anything vaguely complex. you're losing out of a bunch of functionality by writing the script in the Taskfile. You can't use tools like Shellcheck, you get no syntax highlighting, and as you said, splitting lines of the script into separate commands means that variables/venvs are not persisted.

Why not just create an sh/bash/zsh file and call it? This feels far cleaner than introducing a complex feature into Task which may introduce confusion in advanced use cases (like you mentioned with deps/subtasks).

@richard-hajek
Copy link
Author

richard-hajek commented Apr 9, 2023

For cleaner project directory most immediately. But also if you're gonna have multiple scripts, having a taskfile.yaml at that point, which would just call these scripts, is vastly reducing the utility of the taskfile. I am perfectly capable of calling the scripts myself

But I will probably do it this way in the end ( Taskfile calling the scripts ), for all the reasons you mentioned. And taskfile provides a single entrypoint which is also a very good value utility.

@richard-hajek
Copy link
Author

richard-hajek commented Apr 9, 2023

With that being said, the current behavior is still unexpected to newcomers coming from build scripts such as myself. Maybe a note here https://taskfile.dev/usage/#getting-started or https://taskfile.dev/api/#command ? With the expected workflow of calling .(ba/z/)sh scripts?

@pd93
Copy link
Member

pd93 commented Apr 9, 2023

For cleaner project directory most immediately

This is the only argument I can see. However, (just my opinion) I don't actually see merging different languages syntaxes into a single file as "clean". This is the same reason noone writes their CSS inline in their HTML files (forgive me, I haven't done frontend for many years 😆). It's much cleaner to separate them and have dedicated syntax highlighting and linting for both.

But also if you're gonna have multiple scripts, having a taskfile.yaml at that point, which would just call these scripts, is vastly reducing the utility of the taskfile. I am perfectly capable of calling the scripts myself

The location of the script makes no difference as to how you call the task. You're still just running task mytask. Having the ability to also manually call the script doesn't change this and you're still able to use all the same features of Task that you would if the script is inline.

With that being said, the current behavior is still unexpected to newcomers coming from build scripts such as myself. Maybe a note here https://taskfile.dev/usage/#getting-started or https://taskfile.dev/api/#command ? With the expected workflow of calling .(ba/z/)sh scripts?

This is an extremely frequently asked question. Generally, we would expect users coming from other build/task systems such as make to be used to this functionality, since they work the same way. However, I agree that adding something to our FAQ or style guide is probably a good idea.

@richard-hajek
Copy link
Author

richard-hajek commented Apr 9, 2023

One of the reasons of my departure from make was exactly this behavior haha, in addition to the obscure syntax which breaks on whitespaces with undecipherable errors.

However, I agree that adding something to our FAQ or style guide is probably a good idea.

Please do. Also make the tone as strong as possible and offer solutions with reasons we outlined so:

Q: How to use (Python?) virtual environments in a Taskfile.yaml?
A: Please do not. This is a known request but Taskfile is not intended to handle this case. Where shell interaction is necessary, put scripts in a separate file and run them from the Taskfile. See #1115 for more details.

Q: How to runtime persist shell state and variables across commands?
A: Do not. See above.

@richard-hajek
Copy link
Author

If the docs will be address, I will consider this issue resolved and this proposal in its original form rejected, which is fine though as more beneficial work flows have emerged.

@pd93
Copy link
Member

pd93 commented Apr 9, 2023

I will leave as-is for now, so that others have time to add their thoughts to this issue. If there is no change in consensus, we can make the changes to the docs.

@richard-hajek
Copy link
Author

richard-hajek commented Apr 9, 2023

However in support of the proposal, consider have the following example

tasks:
   build:
      cmds:
          - conda activate buildenv
          - python setup.py build

This really isn't a complicated use case. Having a separate file script for these two commands feels very wrong, unintuitive, messy and cumbersome.

@postlund
Copy link

postlund commented Apr 9, 2023

You can accomplish several shell commands treated as one task like this:

tasks:
   build:
      cmds:
          - |
            conda activate buildenv
            python setup.py build

That's what you want, right?

@pd93
Copy link
Member

pd93 commented Apr 9, 2023

I think for these simpler cases, it is reasonable to do one of the following instead of using a script

cmds:
 - conda activate buildenv && python setup.py build

Or

cmds:
 - |
   conda activate buildenv
   python setup.py build

Edit: @postlund took the words right out of my mouth 😛

@postlund
Copy link

postlund commented Apr 9, 2023

@pd93 Just a tad quicker 😉

@richard-hajek
Copy link
Author

richard-hajek commented Apr 9, 2023

You can accomplish several shell commands treated as one task like this

That is true, I forgot about that. Still unintuitive for a newcomer though

@pd93 pd93 added area: docs Changes related to documentation. and removed state: needs triage Waiting to be triaged by a maintainer. labels May 11, 2023
@andreynering
Copy link
Member

#1202 added this to the FAQ page.

I still think this proposal is valid, though, so I'll keep this issue open.

@eduardorubim
Copy link

I am quite in favor of having a setup for shell consistency.
Having said that, my current workaround for python venv depndent tasks is the following:

vars:
  PYTHON: ./.venv/bin/python

tasks:
  boot: 
    cmds:
      - "{{.PYTHON}} ./test.py"

you can even use variables like $HOME inside your proto python_path definition.
but, following this example, referencing this on another taskfile shoud come with the .venv parent dir defined as well.

includes:
 tests:
   taskfile: ./tests/Taskfile.yml
   dir: ./tests

but whis work around can get you into a relative path "../../" mess if you require running these tasks from the sub dir as cwd ¯_(ツ)_/¯

@pd93 pd93 added type: feature A new feature or functionality. and removed area: docs Changes related to documentation. labels May 24, 2024
@hhoeflin
Copy link

hhoeflin commented Jul 6, 2024

Also coming across this when trying to decide between go-task, just and make. Even make has the .ONESHELL: option to enable this behavior. Just also has this behavior of one line per shell (with different workaround).

One thing I have always struggled with is why one would want a clean new shell for every line? What is the advantage of it? Certainly when doing these things manually by hand, people don't open a new shell for every command that they execute, so is always more difficult to debug the behavior.

So wanted to second the option of having a single shell for a task.

@pd93
Copy link
Member

pd93 commented Jul 6, 2024

why one would want a clean new shell for every line?

@hhoeflin I think the main reason for every command using a new shell is reproducability (i.e. One command does not affect the way a subsequent command behaves). Make is originally a build tool, not a task runner and reproducability is very important for this. Task is still often used for building things and so I think a new shell per command is a good default.

Multiline commands and external scripts are still my preferred way of solving this problem as discussed previously, but I think it's worth considering an option at a task-level for changing this. I'd be concerned that having a global toggle means people will set it and forget it. It is not needed for the vast majority of tasks.

I am quite in favor of having a setup for shell consistency.

@eduardorubim #448 is the way to solve this imo

@ccxuy
Copy link

ccxuy commented Oct 9, 2024

I think this is a very important design issue, making go-task more behave like a shell (sequential) or like docker (isolate and easy to scale and repeat).
The first way is more easy to understand and adopt. When we build some complex stuff, it is hard to dive into detail of every command, the whole process may contain hundreds of steps, but we may need to stop on one of the step and do some other stuff.
The second way may benifits from further development but it may require extra work to rebuild the script, such as managing variables and applying it different steps.

@ccxuy
Copy link

ccxuy commented Oct 9, 2024

It is more like a higher level of abstraction to me:
Sequential commands (hard to reuse) -> Scripts with function calls (hard to remenber commands, or hard to stop in the middle stpes) -> [next step of abstraction]
In the next step, do we want a help menu with function calls of commands with help menu, or what? Which scenario or problem are we solving?

A possible proposal could be some receipts with some action templates, and developer need some data(variables) to fill in.
Some receipts are main course with orders, and some not. Higher level task could delegate task setting data to sub task.

@pd93 pd93 removed the type: feature A new feature or functionality. label Dec 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants