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

Using glob curly braces expansion in sources prevents change detection for some files #2073

Open
Neonox31 opened this issue Feb 18, 2025 · 1 comment · May be fixed by #2075
Open

Using glob curly braces expansion in sources prevents change detection for some files #2073

Neonox31 opened this issue Feb 18, 2025 · 1 comment · May be fixed by #2075
Labels
area: fingerprinting Changes related to checksums and caching.

Comments

@Neonox31
Copy link

Neonox31 commented Feb 18, 2025

Description

Hello,

First thanks a lot for all your work on this project, I use it often for personal and professional contexts and saves me a lot of trouble 👍

Using :

sources:
  - src/{api,ingest-worker}/**/*

triggers correctly the job if I change source files in src/api folder but never trigger the job if I change files inside src/ingest-worker.
Instead the following :

sources:
  - src/api/**/*
  - src/ingest-worker/**/*

triggers the job if I change files inside both src/api and src/ingest-worker folders.

To be sure, I checked my syntax with zglob and seems to be OK :

❯ /bin/find dist/ src/
dist/
dist/ingest-worker
dist/ingest-worker/ingest-worker.out
dist/api
dist/api/api.out
src/
src/ingest-worker
src/ingest-worker/ingest-worker.source
src/foo
src/foo/foo.source
src/api
src/api/api.source

❯ ./zglob -d 'src/{api,ingest-worker}/**/*'
src/ingest-worker/ingest-worker.source
src/api/api.source

Maybe I misunderstood something but both methods should not match same files here ?

Version

Task version: v3.41.0 (h1:giUddhe0XZLbEWIQ/MuTPipR9ek+teulIA5xf/2IHXg=)

Operating system

Linux

Experiments Enabled

No response

Example Taskfile

version: '3'

tasks:
  build:
    cmds:
      - mkdir -p dist/{api,ingest-worker}
      - cp src/api/api.source dist/api/api.out
      - cp src/ingest-worker/ingest-worker.source dist/ingest-worker/ingest-worker.out
    sources:
      - src/{api,ingest-worker}/**/*
@task-bot task-bot added the state: needs triage Waiting to be triaged by a maintainer. label Feb 18, 2025
@Neonox31 Neonox31 changed the title Using glob curly braces expansion in sources prevents chnage detection for some files Using glob curly braces expansion in sources prevents change detection for some files Feb 18, 2025
@pd93 pd93 added area: fingerprinting Changes related to checksums and caching. and removed state: needs triage Waiting to be triaged by a maintainer. labels Feb 19, 2025
@pd93 pd93 linked a pull request Feb 19, 2025 that will close this issue
@pd93
Copy link
Member

pd93 commented Feb 19, 2025

Thanks for filing this and providing a detailed example. I am able to reproduce your issue.

As you said, this is not the fault of zglob. If you pass src/{api,ingest-worker}/**/* into zglob it will give the expected result. This issue is being caused by us preprocessing the string using execext.Expand. This then calls into the mvdan shell.Fields function which will perform the brace expansion and return a list of matching files. However, currently we are only returning the first match which means that by the time the string gets to zglob, any references to the second directory are gone.

Interestingly, shell.Fields actually calls into expand.Fields which then accepts a cfg object. In this config, you can enable globbing and globstar. So maybe we don't need zglob at all!

I've created a draft PR (#2075) for this, but I need to look into whether moving from zglob to mvdan's globbing has any other knock-on effects. I also want to check if we can avoid calling Expand in other places. For now, I have replaced any other calls to Expand with ExpandFirst which is a wrapper that will just return the first result (like we do now).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: fingerprinting Changes related to checksums and caching.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants