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

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

Merged
merged 2 commits into from
Jun 22, 2024

Conversation

AlexWaygood
Copy link
Member

Summary

This PR fixes the bug that caused the tests added in #11970 to fail. In the higher-level ruff_db::vendored::VendoredFileSystem abstraction we use for querying whether files exist in the zip file, all paths are normalized to use POSIX path separators before they're looked up in the underlying zip archive. But when we're creating the zip archive in build.rs, we're currently just using the operating system's default path separator (\\ on Windows!). If we normalize the path separator in the ruff_db abstractions, we need to also do so in build.rs.

Test Plan

cargo test -p red_knot_module_resolver still passes for me on macOS; hopefully it will also pass on Windows in CI. I'm pretty confident this is the cause of the test failures we saw on #11970: in a PR to my fork here, I added some debug prints so we could figure out what was going on with Windows, and you can see that the zip file on Windows has paths such as stdlib\\abc.pyi instead of stdlib/abc.pyi: AlexWaygood#10.

@AlexWaygood AlexWaygood added the red-knot Multi-file analysis & type inference label Jun 22, 2024
Copy link
Member

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

Hmm I don't think that's necessary. At least according to the ZIP file standard:

4.4.17.1 The name of the file, with optional relative path.
The path stored MUST NOT contain a drive or
device letter, or a leading slash. All slashes
MUST be forward slashes '/' as opposed to
backwards slashes '' for compatibility with Amiga
and UNIX file systems etc. If input came from standard
input, there is no file name field.
https://pkware.cachefly.net/webdocs/casestudies/APPNOTE.TXT

I wonder if all that is needed is to fix the test?

Copy link
Contributor

github-actions bot commented Jun 22, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

@AlexWaygood
Copy link
Member Author

AlexWaygood commented Jun 22, 2024

Not sure I understand. I think that's all that I'm doing. All that this PR does is use path_slash to change \s to /s before writing the files to the zip archive at build time. No other normalization is being done here. What do you think I'm doing here that's unnecessary?

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

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

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()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
red-knot Multi-file analysis & type inference
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants