go-task / task

A task runner / simpler Make alternative written in Go
https://taskfile.dev
MIT License
11.01k stars 584 forks source link

Duplicate dynamic variables in multiple included taskfiles are overwritten #830

Open dkryptr opened 2 years ago

dkryptr commented 2 years ago

I believe this is a regression in functionality.

I have a monorepo that I haven't touched for around 6 or more months. I upgraded and narrowed down the version that the issue was introduced (v3.2.1). Each project is in its own folder with a taskfile that declare dynamic variables. There's a root Taskfile.yml that includes each project's Taskfile.yml and sets the directory to the project's directory.

The behavior in v3.2.0 is that dynamic variable's working directory is scoped to whatever directory the parent Taskfile set in the includes statement. I've found this isn't the case in >= v3.2.1.

I've included the folder structure along with all file contents and the command line output to recreate the issue.

monorepo/
├── Taskfile.yml
├── project1/
│   ├── version.txt
│   ├── Taskfile.yml
└── project2/
    ├── version.txt
    ├── Taskfile.yml
# monorepo/Taskfile.yml
version: '3'

includes:
  project1:
    taskfile: ./project1/Taskfile.yml
    dir: ./project1
  project2:
    taskfile: ./project2/Taskfile.yml
    dir: ./project2
monorepo/project1/version.txt
# monorepo/project1/Taskfile.yml
version: '3'

vars:
  VERSION:
    sh: cat version.txt

tasks:
  version:
    cmds:
      - echo {{.VERSION}}
monorepo/project2/version.txt
# monorepo/project2/Taskfile.yml
version: '3'

vars:
  VERSION:
    sh: cat version.txt

tasks:
  version:
    cmds:
      - echo {{.VERSION}}

Screenshot 2022-08-08 005931 Screenshot 2022-08-08 005913

MarioSchwalbe commented 2 years ago

Some remarks:

  1. It is quite hard to match the source and the output. The source prints the version, but the output shows paths.
  2. Global variables (even in multiple files) share the same namespace (scope) and overwrite each other. So you will always get the last definition of VERSION.
  3. If the variable is defined within the task, you might experience: https://github.com/go-task/task/issues/839
dominik-lekse commented 2 years ago

@dkryptr Thanks for describing the issue precisely.

I have encountered the issue recently in a similar structured repository. Tracking it down was not straightforward.

The Taskfile.yml of each sub project is included in the root Taskfile.yml while the dir attribute points to path of a sub project.

# root/Taskfile.yml
version: '3'

includes:
  project1:
    taskfile: ./project1/Taskfile.yml
    dir: ./project1
  project2:
    taskfile: ./project2/Taskfile.yml
    dir: ./project2
# root/project1/Taskfile.yml
# root/project2/Taskfile.yml
version: '3'

vars:
  PROJECT_DIR:
    sh: pwd

tasks:
  whereami:
    cmds:
      - echo "{{.PROJECT_DIR}}"
      - pwd

Within this context, the observed behaviour is as follows.

$ task project1:whereami
task: [project1:whereami] echo "root/project2"
root/project2
task: [project1:whereami] pwd
root/project1

$ task project2:whereami
task: [project2:whereami] echo "root/project2"
root/project2
task: [project2:whereami] pwd
root/project2

The reason for this different between echo "{{.PROJECT_DIR}}" and pwd in task project1:whereami is as @MarioSchwalbe described: Global variables (even in multiple files) share the same namespace (scope) and overwrite each other.

I am wondering if there has been a decision to keep global variables of included Taskfiles in the same namespace rather, or if scope these variables has not yet been implemented.

From a user of task, I had the intuition that global variables are scoped, similar to scoped task names when including in a parent Taskfile.

MarioSchwalbe commented 2 years ago

In fact, both use cases do make sense. If it is the same global scope, included taskfiles can overwrite variables of their parent taskfile to allow further customization. For instance, my taskfiles are usually structured linke this:

version: '3'

# Default config.
vars:
  VAR1: 1
  VAR2: 2
  # ...

includes:
  # Local override if present.
  my:
    taskfile: local.yml
    optional: yes

tasks:
  # ...

With respect to composability, however, this might become a problem.

dominik-lekse commented 2 years ago

@MarioSchwalbe I understand that in the use case you presented, you want to optionally override global variables in a parent Taskfile by global variables in an included Taskfile. For this case, overrides make sense

In contrast, in the use case, in which two Taskfiles are included in a parent Taskfile, the overrides should not occur.

I want to share some additional findings:

1) Dynamic variable values are overridden across two included Taskfiles if and only if the sh command is identical.

2) Static variable values are not override across two included Taskfiles.

It seems that the result of the command in the dynamic variable is cached and then reused across the scope. The issue did not occur with dynamic variables, which have the same name but use a different sh command. As a workaround, I have added a unique comment at the end of the command.

Therefore, I would argue that this issue is actually a duplicate of #524.

Example

# root/Taskfile.yml
version: '3'

includes:
  project1:
    taskfile: ./project1/Taskfile.yml
    dir: ./project1
  project2:
    taskfile: ./project2/Taskfile.yml
    dir: ./project2
# root/project1/Taskfile.yml
version: '3'

vars:
  PROJECT_NAME: project1
  PROJECT_DIR:
    # sh: pwd
    sh: 'pwd; # project1'

tasks:
  whereami:
    cmds:
      - echo "{{.PROJECT_NAME}}"
      - echo "{{.PROJECT_DIR}}"
      - pwd
# root/project2/Taskfile.yml
version: '3'

vars:
  PROJECT_NAME: project2
  PROJECT_DIR:
    # sh: pwd
    sh: 'pwd; # project2'

tasks:
  whereami:
    cmds:
      - echo "{{.PROJECT_NAME}}"
      - echo "{{.PROJECT_DIR}}"
      - pwd
max-sixty commented 1 year ago

I hit I have an even simpler reproduction of this, no need for sh:

# Taskfile.yml
version: 3

vars:
  bucket: '{{default "1" .bucket}}'

# includes:
#  other: Taskfile2.yml

tasks:
  task1:
    - "echo {{.bucket}}"
# Taskfile2.yml
version: 3
vars:
  bucket: '{{default "2" .bucket}}'

Without any include statement:

task task1

# 1

But only by including the second taskfile (uncommenting the includes above), we get a different result:

task task1

# 2

...which seems quite wrong — that we can change the default variable just by including another Taskfile.

toddlerya commented 1 year ago

I also encountered the same problem

megakoresh commented 7 months ago

Exactly same issue issue as well. Although I cannot reproduce it with directly set variables, only with dynamic variables. And it does not seem to be a race condition, as using sleep 0.$((1 + $RANDOM % 10)) && cat .version does not appear to solve the problem.

megakoresh commented 4 months ago

This really does seem to be identical to #524 I can confirm that adding a comment or some other meaningless string after the variable definition solves the issue. I think this can be closed then, no? Effort should be in #524 to fix the problem

max-sixty commented 4 months ago

This really does seem to be identical to #524 I can confirm that adding a comment or some other meaningless string after the variable definition solves the issue. I think this can be closed then, no? Effort should be in #524 to fix the problem

The example in https://github.com/go-task/task/issues/830#issuecomment-1376680448 is different IIUC — no need for sh:. Is that correct?

megakoresh commented 4 months ago

This really does seem to be identical to #524 I can confirm that adding a comment or some other meaningless string after the variable definition solves the issue. I think this can be closed then, no? Effort should be in #524 to fix the problem

The example in #830 (comment) is different IIUC — no need for sh:. Is that correct?

Well like I said, I cannot repro this without the sh: at least. If you can, then sure this is different (unless the caching logic in #524 also applies to static vars).

max-sixty commented 4 months ago

Well like I said, I cannot repro this without the sh: at least. If you can, then sure this is different (unless the caching logic in #524 also applies to static vars).

What output do you get from my repro above?

megakoresh commented 2 months ago

What output do you get from my repro above?

@max-sixty i did not get notification for this, so tagging so you get the notification. Yes, I was able to repro the issue with your second "simpler repro" steps (without sh and without the nested foldas). I suppose the trick here is to have the dynamic value (i.e. something evaluated). I suspect maybe the cache keys are just using the variable name or just the yaml line contents, without taking the taskfile into consideration, so thats why this happens.

Would be nice if someone familiar with the project internals took a look at this because if the issue is this easy to reproduce, then this should be relatively simple to find a fix, if you know where the code that is responsible for this lies, no?

max-sixty commented 2 months ago

OK, thanks for repro-ing.

Would be nice if someone familiar with the project internals took a look at this because if the issue is this easy to reproduce, then this should be relatively simple to find a fix, if you know where the code that is responsible for this lies, no?

Not necessarily — but feel free to take a look — the number of people who are familiar with the code is not fixed...