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 setup hook #668

Closed

Conversation

tylermmorton
Copy link
Contributor

@tylermmorton tylermmorton commented Feb 3, 2022

This PR is in response to issues described in #204. I've modified RunCommand to accept a slice of strings to execute in the same shell. Each string in the slice is run through syntax.Parser and the resulting statements are appended to a single syntax.File as if it were a small script.

This ultimately results in all commands under setup being run before every single command, the advantage here is that they are executed in the same shell.

version: '3'

setup:
  - source .env
  - export DIR=$(pwd)

tasks:
  test:
    cmds:
      - echo $DIR

This is a rough draft for this feature. Personally I feel like there could be some performance implications with running setup commands before every single command in a task. I'm open to suggestions/feedback from the community!

It might make more sense to have all commands under a task run in the same shell by using the same strategy applied here, ie appending all commands into one script and passing them all at once to the shell.

@andreynering
Copy link
Member

Hi @tylermmorton,

Personally I feel like there could be some performance implications with running setup commands before every single command in a task. I'm open to suggestions/feedback from the community!

Yeah. There's a refactor I have in mind that would benefit both this setup feature and another thing some users have been requesting: that commands in a single task would share state:

version: '3'

cmds:
  default:
    - export FOO=bar
    - echo $FOO # has access to previous declared variables

For this to be possible, I think we need to refactor the execext package so the mvdan.cc/sh/v3/interp.Runner struct is stored in another struct in our app. This way we would be able to do successive calls to runner.Run so the state is kept between them.

For setup specifically I think we would be able to run the setup once and cache the result. For the task, we could just clone/copy this struct/state before running the task commands. We may need to contribute to mvdan/sh to have a clone function.

My proposal is to do this refactor alone on its own PR as a POC, and once in master we should work on setup and this state behavior on their own PRs.

@ghostsquad
Copy link
Contributor

I don't agree with this being an improvement. I think this would result in more confusion, especially given the precedent that no other tool combines cmd lists into a single shell. Not Make, not GitHub actions, not Docker, etc.

@ghostsquad
Copy link
Contributor

ghostsquad commented Apr 6, 2022

@tylermmorton what I would like to understand in greater clarity is the use case that is solved by combining the separate commands into a single script, that a multiline yaml string cannot easily solve.

https://yaml-multiline.info/

@tylermmorton
Copy link
Contributor Author

@ghostsquad You bring up a good point that no other tool runs command lists in a single shell. I'm for keeping the precedent consistent across tools. There is also the problem that cmds and deps would then have diverging functionality if this change were to go in.

The use case where you can source an .env or .bashrc-like file containing variable and function definitions to be used by all tasks was my original inspiration for this PR.

As a specific example, in a monorepo I'm working in, the goal is to have a script that can resolve relative paths and export them as a series of environment variables. Now that I'm thinking about it, perhaps this could be done with the dotenv option... I'll have to try

I'm willing to close this and move on. Perhaps a better action item coming out of this is to document solutions for use cases mentioned in #204

@ghostsquad
Copy link
Contributor

@tylermmorton ok, sounds good. Ya, try the dotenv functionality described here: https://taskfile.dev/#/usage?id=env-files

And if you run into a situation that isn't working, please open a new issue here.

@tylermmorton
Copy link
Contributor Author

tylermmorton commented Apr 8, 2022

@ghostsquad So I just tried the dotenv feature and realized why it wasn't working for me at first. Here's a use case:

build.env

REPO_ROOT=$(git rev-parse --show-toplevel)

Taskfile.yml

version: 3

dotenv:
  - ./build.env

tasks:
  test:
    cmds:
      - echo $REPO_ROOT

In this case, test echos: "$(git rev-parse --show-toplevel)"

Obviously .env files are not shell scripts so it makes sense why this won't resolve to anything besides the literal string. But there is a workaround that isn't 100% apparent when reading the usage docs under Environment Variables.

env.taskfile.yml

version: 3

env:
  REPO_ROOT:
    sh: git rev-parse --show-toplevel

Taskfile.yml

version: 3

includes:
  env: ./env.taskfile.yml

tasks:
  test:
    cmds:
      - echo $REPO_ROOT

This works as you would expect and feels like a good strategy. However, relying on the environment variables here to declare additional variables does not work:

env.taskfile.yml

version: 3

env:
  REPO_ROOT:
    sh: git rev-parse --show-toplevel
  PROJECT1_ROOT:
    sh: $REPO_ROOT/project1

Taskfile.yml

version: 3

includes:
  env: ./env.taskfile.yml

tasks:
  test:
    cmds:
      - echo $PROJECT1_ROOT

Output:

stat /project1: no such file or directory
task: Command "$REPO_ROOT/project1" failed: exit status 127

I have not yet dug into the task source to figure out whats going on here. From an outside perspective it seems like the shell that is spawned under the env declaration does not actually use any of the declared environment vars. There's also the idea that env is a map and order can't be guaranteed. Curious to know your thoughts.

@ghostsquad
Copy link
Contributor

I'm not totally sure what's going on in V3 here. My plan in v4 is to lazily evaluate variables. Essentially waiting until they are used before discovering (and caching) the value. This would allow task to produce the correct order without the user having to think about it. jsonnet does this. It is an incredibly powerful feature I hope I can bring to task.

@tylermmorton
Copy link
Contributor Author

tylermmorton commented Apr 8, 2022

Lazy loading does sound like the solution to a lot of issues being reported in this area (#535, #582, #591) -- Do we have an issue or discussion created where we can discuss the feature and cross reference issues? I'm down to help design/implement/test it if needed.

@ghostsquad
Copy link
Contributor

I'm working on this in v4. I don't have anything yet that's ready for collaborative work yet.

@ghostsquad
Copy link
Contributor

Do we have an issue or discussion created where we can discuss the feature and cross reference issues?

The discussion is #694 - but unfortunately, when you mention a discussion from an issue (or vice versa), it only makes a link in the comment, not a reference in the discussion (like it works in issues).

@andreynering andreynering mentioned this pull request Apr 16, 2022
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 this pull request may close these issues.

3 participants