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 using relative paths with --local-cache-dir and --named-caches-dir #10398

Merged
merged 5 commits into from Jul 20, 2020

Conversation

Eric-Arellano
Copy link
Contributor

Before, this would not actually write to anything:

./pants --no-process-execution-use-local-cache --named-caches-dir=test_cache --pants-ignore=test_cache --no-pantsd test src/python/pants/util/dirutil_test.py

While a user could use ${PWD}/test_cache} or %(buildroot)s in a config file, we generally allow relative paths with Pants, like --pants-ignore=test_cache, so there's an argument that we should be consistent here.

[ci skip-rust-tests]

…-dir`

# Rust tests will be skipped. Delete if not intended.
[ci skip-rust-tests]
Copy link
Contributor Author

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

I'm not certain that this is a good change. An alternative approach would be to eagerly fail if a relative path is used and to give instructions about using %(buildroot)s.

But, I think I ultimately am in favor because we generally allow relative paths every where else, like with --pants-ignore and --isort-config. Thoughts?

Whatever we decide, we should be consistent with pantsbuild/setup#83, such as possibly reverting it.

@g-cassie
Copy link
Contributor

g-cassie commented Jul 17, 2020

Just my personal style would be to force everyone on to one approach (i.e. absolute paths) but provide really helpful assertion errors when devs try other common approaches (i.e. relative paths). I can't think of any relative path use case that is not solved by %(buildroot)s and maybe there are even potential security concerns with paths that start with a character other than / (e.g. ../../secret_thing). [EDIT: can't think actually think of any real example of how this exploit would work where absolute paths couldn't do the same...]

All that said, Gitlab does require all cached files to go in the buildroot so need a clear consistent way to move the three cache directories from their default locations to the buildroot/working directory.

@stuhood
Copy link
Sponsor Member

stuhood commented Jul 17, 2020

So, in the thread I started to recommended making paths absolute via interpolation of those variables, but I ended up deleting that because consistently resolving all file/dir options relative to either the location of the file they were declared in or on the CLI would be the least surprising thing, probably.

Copy link
Sponsor Contributor

@benjyw benjyw left a comment

Choose a reason for hiding this comment

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

I think this is fine, and probably the least hassle to users.

@coveralls
Copy link

coveralls commented Jul 20, 2020

Coverage Status

Coverage remained the same at 0.0% when pulling faa3536 on Eric-Arellano:fix-cache into 1f6267f on pantsbuild:master.

[ci skip-rust-tests]
@Eric-Arellano Eric-Arellano merged commit 26c73d8 into pantsbuild:master Jul 20, 2020
@Eric-Arellano Eric-Arellano deleted the fix-cache branch July 20, 2020 19:44
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.

None yet

5 participants