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.

Sign up for GitHub

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

[red-knot] Use POSIX representations of paths when creating the typeshed zip file #11982

Merged
merged 2 commits into from
Jun 22, 2024
Merged

[red-knot] Use POSIX representations of paths when creating the typeshed zip file #11982

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
74d02e9
[red-knot] Use POSIX representations of paths when creating the types…
AlexWaygood Jun 22, 2024
4fc0d75
Get rid of the dev dependency
AlexWaygood Jun 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions crates/red_knot_module_resolver/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ tracing = { workspace = true }
zip = { workspace = true }

[build-dependencies]
path-slash = { workspace = true }
walkdir = { workspace = true }
zip = { workspace = true }

Expand Down
21 changes: 11 additions & 10 deletions crates/red_knot_module_resolver/build.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
use std::fs::File;
use std::path::Path;

use path_slash::PathExt;
use zip::result::ZipResult;
use zip::write::{FileOptions, ZipWriter};
use zip::CompressionMethod;
Expand All @@ -28,25 +29,25 @@ fn zip_dir(directory_path: &str, writer: File) -> ZipResult<File> {

for entry in walkdir::WalkDir::new(directory_path) {
let dir_entry = entry.unwrap();
let relative_path = dir_entry.path();
let name = relative_path
let absolute_path = dir_entry.path();
let normalized_relative_path = absolute_path
.strip_prefix(Path::new(directory_path))
.unwrap()
.to_str()
.to_slash()
.expect("Unexpected non-utf8 typeshed path!");

// Write file or directory explicitly
// Some unzip tools unzip files with directory paths correctly, some do not!
if relative_path.is_file() {
println!("adding file {relative_path:?} as {name:?} ...");
zip.start_file(name, options)?;
let mut f = File::open(relative_path)?;
if absolute_path.is_file() {
println!("adding file {absolute_path:?} as {normalized_relative_path:?} ...");
zip.start_file(normalized_relative_path, options)?;
Copy link
Member

@MichaReiser MichaReiser Jun 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

There's a start_file_from_path function that takes care of the normalization

Copy link
Member Author

Choose a reason for hiding this comment

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

On v0.6.6, the version we have pinned in Cargo.toml, there is a deprecation warning attached to this method in the docs: https://docs.rs/zip/0.6.6/zip/write/struct.ZipWriter.html#method.start_file_from_path.

The deprecation warning isn't there on the latest version, so I suppose they must have fixed the issues and undeprecated the method. But we're keeping zip pinned to the old version due to astral-sh/uv#3642

Copy link
Member Author

@AlexWaygood AlexWaygood Jun 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This is what the change looks like (relative to this PR branch) if we want to keep the zip crate pinned to v0.6.6 and have Clippy pass:

diff --git a/crates/red_knot_module_resolver/build.rs b/crates/red_knot_module_resolver/build.rs
index 15f67f3bb..c138d3bc9 100644
--- a/crates/red_knot_module_resolver/build.rs
+++ b/crates/red_knot_module_resolver/build.rs
@@ -8,7 +8,6 @@
 use std::fs::File;
 use std::path::Path;
 
-use path_slash::PathExt;
 use zip::result::ZipResult;
 use zip::write::{FileOptions, ZipWriter};
 use zip::CompressionMethod;
@@ -30,24 +29,24 @@ fn zip_dir(directory_path: &str, writer: File) -> ZipResult<File> {
     for entry in walkdir::WalkDir::new(directory_path) {
         let dir_entry = entry.unwrap();
         let absolute_path = dir_entry.path();
-        let normalized_relative_path = absolute_path
+        let relative_path = absolute_path
             .strip_prefix(Path::new(directory_path))
-            .unwrap()
-            .to_slash()
-            .expect("Unexpected non-utf8 typeshed path!");
+            .unwrap();
 
         // Write file or directory explicitly
         // Some unzip tools unzip files with directory paths correctly, some do not!
         if absolute_path.is_file() {
-            println!("adding file {absolute_path:?} as {normalized_relative_path:?} ...");
-            zip.start_file(normalized_relative_path, options)?;
+            println!("adding file {absolute_path:?} as {relative_path:?} ...");
+            #[allow(deprecated)]
+            zip.start_file_from_path(relative_path, options)?;
             let mut f = File::open(absolute_path)?;
             std::io::copy(&mut f, &mut zip).unwrap();
-        } else if !normalized_relative_path.is_empty() {
+        } else if relative_path == Path::new("") {
             // Only if not root! Avoids path spec / warning
             // and mapname conversion failed error on unzip
-            println!("adding dir {absolute_path:?} as {normalized_relative_path:?} ...");
-            zip.add_directory(normalized_relative_path, options)?;
+            println!("adding dir {absolute_path:?} as {relative_path:?} ...");
+            #[allow(deprecated)]
+            zip.add_directory_from_path(relative_path, options)?;
         }
     }
     zip.finish()

let mut f = File::open(absolute_path)?;
std::io::copy(&mut f, &mut zip).unwrap();
} else if !name.is_empty() {
} else if !normalized_relative_path.is_empty() {
// Only if not root! Avoids path spec / warning
// and mapname conversion failed error on unzip
println!("adding dir {relative_path:?} as {name:?} ...");
zip.add_directory(name, options)?;
println!("adding dir {absolute_path:?} as {normalized_relative_path:?} ...");
zip.add_directory(normalized_relative_path, options)?;
}
}
zip.finish()
Expand Down
4 changes: 1 addition & 3 deletions crates/red_knot_module_resolver/src/typeshed.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ pub(crate) mod versions;
#[cfg(test)]
mod tests {
use std::io::{self, Read};
use std::path::Path;

#[test]
fn typeshed_zip_created_at_build_time() -> anyhow::Result<()> {
Expand All @@ -14,9 +13,8 @@ mod tests {

let mut typeshed_zip_archive = zip::ZipArchive::new(io::Cursor::new(TYPESHED_ZIP_BYTES))?;

let path_to_functools = Path::new("stdlib").join("functools.pyi");
let mut functools_module_stub = typeshed_zip_archive
.by_name(path_to_functools.to_str().unwrap())
.by_name("stdlib/functools.pyi")
.unwrap();
assert!(functools_module_stub.is_file());

Expand Down
Loading