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

Snapshots of single files store normalized paths. #10707

Merged
merged 1 commit into from
Aug 30, 2020

Conversation

jsirois
Copy link
Contributor

@jsirois jsirois commented Aug 30, 2020

Previously we were not enforcing or normalizing relative paths. There
are two callers, DownloadedFile::load_or_download and the
create_digest_to_digest intrinsic. The former happens to always pass a
normalized relative path due to the parsing behavior of Url but the
latter did no validation or normalization taking a path string directly
via cpython. A normalized relative path is now enforced by demanding a
RelativePath.

[ci skip-build-wheels]

Previously we were not enforcing or normalizing relative paths. There
are two callers, `DownloadedFile::load_or_download` and the
`create_digest_to_digest` intrinsic. The former happens to always pass a
normalized relative path due to the parsing behavior of `Url` but the
latter did no validation or normalization taking a path string directly
via cpython. A normalized relative path is now enforced by demanding a
`RelativePath`.

[ci skip-build-wheels]
@jsirois
Copy link
Contributor Author

jsirois commented Aug 30, 2020

An alternative to #10706.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 0.0% when pulling 44bdf70 on jsirois:pull/10706/alternative into cf483ea on pantsbuild:master.

@jsirois
Copy link
Contributor Author

jsirois commented Aug 30, 2020

The best fix may be in Stat/PathStat but I'm not sure enough about the contexts those deeper structures are used in to be sure those paths should always be relative. I'll be happy to circle back if this validation can be pushed deeper.

@jsirois jsirois merged commit b173bcb into pantsbuild:master Aug 30, 2020
@jsirois jsirois deleted the pull/10706/alternative branch August 30, 2020 18:10
@tdyas
Copy link
Contributor

tdyas commented Aug 30, 2020

This should also fix the Buildgrid test in remote-apis-testing which encountered a similar error:

worker_1    | 2020-08-30T20:16:47.877+0200 [21:140538559145984] [buildboxcommon_runner.cpp:273] [ERROR] [actionDigest=144eabe448bf056e043e09864c90044ee8a6acd13784fcc0a0978aeb728bb258/141] Error executing command: std::system_error exception thrown at [buildboxcommon_client.cpp:321] [system:17], errMsg = "Error in mkdir for directory "/tmp/buildboxrunUChQrL/."", errno : File exists

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