ruff
[red-knot] Include vendored typeshed stubs as a zipfile in the Ruff binary
#11779
Merged
Overview
Commits
9
Changes
View On GitHub

[red-knot] Include vendored typeshed stubs as a zipfile in the Ruff binary #11779

AlexWaygood merged 9 commits into main from zip-typeshed
AlexWaygood
AlexWaygood356 days ago (edited 356 days ago)

Summary

Work towards #11653.

This PR adds a build.rs script to the red_knot crate that compresses our vendored typeshed stubs into a zip that is then included in the Ruff binary via include_bytes!() in module.rs. This will enable us to resolve modules to vendored stdlib stubs from typeshed in the future.

Details:

  • The build.rs script is run before anything else in the crate is built
  • We print a cargo:rerun-if-changed directive to stdout to let Cargo know that the build.rs only needs to be rerun if the contents of crates/red_knot/vendor/typeshed changes, or if the build.rs script itself changes. Docs here: https://doc.rust-lang.org/cargo/reference/build-scripts.html#rerun-if-changed. We need to use a syntax for this directive that's officially deprecated, due to the fact that our pinned minimum Rust version is old enough that it doesn't support the new version. (The deprecated syntax is cargo:rerun-if-changed=, with one colon. The new syntax uses cargo::rerun-if-changed=, with two colons.)
  • The zipfile that is created during the build step is .gitignored. Unfortunately there will be a need to manually keep that .gitignore entry in sync with the hardcoded path in build.rs.
  • The PR adds a new Ruff dependency on the zip crate. Following the precedent in astral-sh/uv#3642, I pinned to an old version of the zip crate, and added a Renovate rule to make sure that we don't get dependency-update PRs for that crate.

Test Plan

Some basic tests have been added that show that the zip archive containing typeshed is available from the Ruff after it has been built, and that the zip archive has some stdlib stubs in it.

AlexWaygood AlexWaygood requested a review from carljm carljm 356 days ago
AlexWaygood AlexWaygood requested a review from MichaReiser MichaReiser 356 days ago
AlexWaygood AlexWaygood added ty
carljm
carljm approved these changes on 2024-06-06
carljm
AlexWaygood
github-actions
carljm
MichaReiser
MichaReiser commented on 2024-06-07
MichaReiser355 days ago (edited 355 days ago)

Nice work. I've some questions before approving

  • What's the binary size increase?
  • What's the build time increase on an incremental (change to a rust file) and clean build. You can get the timings with cargo build --timings -p red_knot
  • Did you open a created archive. Does it look correct?
Conversation is marked as resolved
Show resolved
.github/renovate.json5
4242 enabled: false,
4343 },
44 {
45
 // Disable updates of `zip-rs`; intentionally pinned for now due to ownership change
46
 // See: https://github.com/astral-sh/uv/issues/3642
47
 matchPackagePatterns: ["zip"],
MichaReiser355 days ago👍 1

Can you add a note on that issue to mention that red_knot now also uses the zip crate. Or maybe this is something we can discuss in today's standup? The main concern seems to be with 0.66 and 1 but we're now at version 2 already.

AlexWaygood355 days ago

Left a note in astral-sh/uv#3642 (comment)

Conversation is marked as resolved
Show resolved
.gitignore
216216!crates/ruff_python_resolver/resources/test/airflow/venv/lib/python3.11/site-packages/_watchdog_fsevents.cpython-311-darwin.so
217217!crates/ruff_python_resolver/resources/test/airflow/venv/lib/python3.11/site-packages/orjson/orjson.cpython-311-darwin.so
218
219
# Ignore the zipped version of typeshed that we create at build time
220
crates/red_knot/vendor/zipped_typeshed.zip
MichaReiser355 days ago👍 1

Can we create the zip in the target directory where cargo stores all its build artifacts? This way, we can avoid adding an additional ignore, and it also makes it clear that this is a build-time-created artifact.

AlexWaygood355 days ago (edited 355 days ago)

To clarify: do you mean $ROOT/target (which already exists), or do you mean creating a new $ROOT/crates/red_knot/target directory for redknot-specific build artifacts? (I'm happy to do either)

AlexWaygood355 days ago (edited 355 days ago)

Reading through rust-lang/cargo#9661, it sounds like getting the location of the target directory reliably from a build script is a deliberately unsolved problem, and that build scripts are not meant to put artifacts there. rust-lang/cargo#9661 says:

OUT_DIR — If the package has a build script, this is set to the folder where the build script should place its output. See below for more information. (Only set during compilation.)

That implies to me that I should be putting the zip there, rather than where I have it in this PR currently or in target/

MichaReiser355 days ago👍 1

Yeah sorry, I'm not very familiar with build scripts. Out dir seems to be what I was looking for.

Conversation is marked as resolved
Show resolved
Cargo.lock
MichaReiser355 days ago👍 1

Uff, so many new dependencies. Would you mind having a look at https://docs.rs/crate/zip/0.6.6/features and removing all features that we don't need? We probably only need a very specific subset

Conversation is marked as resolved
Show resolved
crates/red_knot/build.rs
77
78 let zipped_typeshed = File::create(Path::new(TYPESHED_ZIP_LOCATION)).unwrap();
79
80
 let mut typeshed_traverser = WalkDir::new(TYPESHED_SOURCE_DIR)
81
 .into_iter()
MichaReiser355 days ago👍 1

I don't think we want to filter here. Instead we want to error if we can't read a file. It could otherwise happen that we don't include a file because of a read error and we would only learn about it post release.

I think I would also move the WalkDir call into zip_dir and instead change zip_dir to take a path as an argument. That removes the ugly impl Iterator ;)

AlexWaygood355 days ago

I don't think we want to filter here.

Yeah, I wondered about that... In the end I guess I was lazy and didn't make too many changes to the recipe Carl and I found at https://github.com/zip-rs/zip-old/blob/5d0f198124946b7be4e5969719a7f29f363118cd/examples/write_dir.rs. But this makes sense; I'll make the change.

crates/red_knot/build.rs
48 // Write file or directory explicitly
49 // Some unzip tools unzip files with directory paths correctly, some do not!
50 if path.is_file() {
51
 println!("adding file {path:?} as {name:?} ...");
MichaReiser355 days ago

Can we remove the println statements? Especially if they clutter the CLI output

AlexWaygood355 days ago

Because this is a build script, these aren't actually printed to stdout during the build process unless the build actually fails -- it's more like logging, really. But if the build fails, then the entire captured stdout from the build is printed in a summary, e.g.

image

Conversation is marked as resolved
Show resolved
crates/red_knot/build.rs
52 zip.start_file(name, options)?;
53 let mut f = File::open(path)?;
54
55
 f.read_to_end(&mut buffer)?;
56
 zip.write_all(&buffer)?;
MichaReiser355 days ago

I wonder if you could use std::io::copy here, assuming that f implements Read and zip implements Write

https://doc.rust-lang.org/std/io/fn.copy.html

AlexWaygood355 days ago

Nice, thank you!

AlexWaygood [red-knot] Include vendored typeshed stubs as a zipfile in the Ruff b…
0a5b4b15
AlexWaygood hopefully this fixes Windows?
6283dd26
AlexWaygood this?
049e137b
AlexWaygood Update crates/red_knot/build.rs
4eb13e16
AlexWaygood AlexWaygood force pushed from 3eb687c6 to 4eb13e16 355 days ago
AlexWaygood Misc review comments
f6c451ca
AlexWaygood AlexWaygood force pushed from bfa038a4 to 7d951e59 355 days ago
AlexWaygood
AlexWaygood Put the zip in OUT_DIR
6c7a33c9
AlexWaygood AlexWaygood force pushed from 7d951e59 to 6c7a33c9 355 days ago
AlexWaygood
AlexWaygood AlexWaygood requested a review from MichaReiser MichaReiser 355 days ago
MichaReiser
AlexWaygood
MichaReiser
MichaReiser approved these changes on 2024-06-07
AlexWaygood
AlexWaygood Set the location of the zip via environment variable
c5ad8321
AlexWaygood
MichaReiser
AlexWaygood
AlexWaygood Revert "Set the location of the zip via environment variable"
a5f2bcb9
AlexWaygood nit
f5e6966c
AlexWaygood AlexWaygood force pushed from 1e3641de to f5e6966c 355 days ago
MichaReiser
AlexWaygood
AlexWaygood AlexWaygood enabled auto-merge (squash) 355 days ago
AlexWaygood AlexWaygood merged 37d8de33 into main 355 days ago
AlexWaygood AlexWaygood deleted the zip-typeshed branch 355 days ago

Login to write a write a comment.

Login via GitHub

Reviewers
Assignees
No one assigned
Labels
ty
Milestone
No milestone
Terms Of Use Privacy Policy Refund Policy Impressum