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

fix: use project root directory instead of task.working_directory for base dir when hashing #1202

Merged
merged 4 commits into from
Apr 16, 2024

Conversation

wolfv
Copy link
Member

@wolfv wolfv commented Apr 16, 2024

Should improve #1139

I still want to implement a warning when glob does not match anything at all.

Copy link
Contributor

@ruben-arts ruben-arts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@wolfv
Copy link
Member Author

wolfv commented Apr 16, 2024

There is one behavior that we might still want to discuss.

If a glob is just outputs = ["built.txt"] it matches on any built.txt that is found anywhere (e.g. also build/built.txt).

When using /built.txt that seems to be avoided. Using ./built.txt doesn't seem to work though.

I think, intuitively I would expect a simple built.txt to only match in the root of the project and not anywhere else.

What do you think @bollwyvl?

@ruben-arts
Copy link
Contributor

Oh that sounds a little weird, why does it auto glob on the name? I would expect it to be a path or if you do */build.txt it would start searching.

@wolfv
Copy link
Member Author

wolfv commented Apr 16, 2024

I would argue that that's something we can fix in a follow-up PR as well (this one already contains a few improvements). WDYT/

@ruben-arts ruben-arts merged commit 3e88ca0 into prefix-dev:main Apr 16, 2024
24 checks passed
@wolfv wolfv deleted the file-hash-root-dir branch April 16, 2024 08:32
@ruben-arts ruben-arts added the breaking Breaks something in the api or config label Apr 16, 2024
@bollwyvl
Copy link
Contributor

Nice. I see this was already merged, but ...

I think, intuitively I would expect a simple built.txt to only match in the root of the project and not anywhere else.

agreed, unlike .gitignore, this is including things, and inputs=["build.txt"] should likely not be found in multiple places unless there's a * in there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Breaks something in the api or config
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants