-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Reject uv build if the cache dir is enclosed
#19991
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -357,6 +357,20 @@ async fn build_impl( | |
| vec![AnnotatedSource::from(src)] | ||
| }; | ||
|
|
||
| // Build backends can include arbitrary files from the source directory in the distribution. | ||
| // Reject an active cache within the source to avoid including cache contents in the build. | ||
| for source in &packages { | ||
| if let Source::Directory(source_dir) = &source.source | ||
| && is_path_within(cache.root(), source_dir) | ||
| { | ||
| return Err(anyhow::anyhow!( | ||
| "The cache directory `{}` is inside the build source directory `{}`", | ||
| cache.root().user_display(), | ||
| source_dir.user_display() | ||
| )); | ||
| } | ||
| } | ||
|
|
||
| let results: Vec<_> = futures::future::join_all(packages.into_iter().map(|source| { | ||
| let future = build_package( | ||
| source.clone(), | ||
|
|
@@ -1241,6 +1255,21 @@ impl Source<'_> { | |
| } | ||
| } | ||
|
|
||
| /// Return `true` if `path` is within `directory`, resolving symlinks when possible. | ||
| fn is_path_within(path: &Path, directory: &Path) -> bool { | ||
| if path.starts_with(directory) { | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. NB: This is the happy path, I don't expect we'd usually have situations where the cache directory is only enclosed under the project directory after canonicalization.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wouldn't this cause false positives for
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. making sure you don't miss this comment^
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oops, thanks. Yeah, I think this is wrong. I thought
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (I'll do a follow-up PR with the fix.)
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. cc @EliteTK another case |
||
| return true; | ||
| } | ||
|
|
||
| if let Ok(path) = fs_err::canonicalize(path) | ||
| && let Ok(directory) = fs_err::canonicalize(directory) | ||
| { | ||
| path.starts_with(directory) | ||
| } else { | ||
| false | ||
| } | ||
| } | ||
|
|
||
| /// We run all builds in parallel, so we wait until all builds are done to show the success messages | ||
| /// in order. | ||
| #[derive(Debug, Clone)] | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: what does "active" mean here?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can create two cache dirs with
uv pip install tqdm --cache-dir aanduv pip install tqdm --cache-dir b, but whileais a uv cache dir, in the latter call onlybis an active cache dir.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm OK I guess, but how's that relevant from the context of this PR? When you're running
uv pip install tqdm --cache-dir bthenais just a regular directory, we don't care about it. The nit is that "active" in this context isn't a useful distinctionThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's two ways we could detect cache dirs: We could look for a uv marker that tells us this is any uv cache dir and ban that, or we could only consider the one active cache dir to reject. In this PR, we chose the latter approach: Only reject the active cache, but allow any other cache (without saying whether that's a good idea, we just say we don't explicitly reject those).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, exactly that. I figured rejecting the "active" one is the best balance between guarding most users and needing to identify all possible caches in the project tree. We could do the latter by walking and looking for CACHEDIR.TAG, but I was concerned about the performance hit from doing that 🙂
(OTOH maybe we should, at least for our own build backend? I'm not sure.)