[red-knot] Typeshed patching: use build.rs instead of workflow#15370
[red-knot] Typeshed patching: use build.rs instead of workflow#15370
Conversation
f460e0f to
35f55f4
Compare
.github/workflows/sync_typeshed.yaml
Outdated
| source="crates/red_knot_vendored/knot_extensions/knot_extensions.pyi" | ||
| target="crates/red_knot_vendored/vendor/typeshed/stdlib/knot_extensions.pyi" | ||
| ( | ||
| echo "# WARNING: This file is generated by the typeshed sync workflow. Do not edit it manually." | ||
| echo "# Edit the source file at '$source' instead and then copy it over to this file." | ||
| ) > "ruff/$target" | ||
| cat "ruff/$source" >> "ruff/$target" | ||
| ( | ||
| echo "# Patch applied for red_knot:" | ||
| echo "knot_extensions: 3.0-" | ||
| ) >> ruff/crates/red_knot_vendored/vendor/typeshed/stdlib/VERSIONS |
There was a problem hiding this comment.
I don't mind the approach taken here. An alternative could be to copy the file in the red_knot_vendored's build.rs.
There was a problem hiding this comment.
Ohhh, I like this much more. Thanks!
| } | ||
|
|
||
| fn main() { | ||
| println!("cargo::rerun-if-changed={TYPESHED_SOURCE_DIR}"); |
There was a problem hiding this comment.
Removing this means that we will rebuild red_knot_vendored if any file in the package has changed (ref). I need this, because knot_extensions.pyi is not in the vendor/typeshed directory, and we need to pick up changes to it.
And I don't think there's a huge drawback since there are no files in red_knot_vendored that should not trigger a rebuild except for the README.
|
AlexWaygood
left a comment
There was a problem hiding this comment.
Ahh, using build.rs for this is so much cleaner!
| // Patch the VERSIONS file to make `knot_extensions` available | ||
| if normalized_relative_path == "stdlib/VERSIONS" { | ||
| writeln!(&mut zip, "knot_extensions: 3.0-")?; | ||
| } |
There was a problem hiding this comment.
When I originally wrote this function, I intended to write it so that it could be used to zip up any directory into a zip archive -- that's why it's called zip_dir(), and that's why it accepts the typeshed directory path as a parameter rather than hardcoding it in the function. I think since I originally wrote it, it's already grown a few features that make it quite specific to its use-case here, such as the conditional logic a few lines above for deciding what the compression method should be. So I don't really mind much hardcoding things like "stdlib/VERSIONS" directly into the function body. But at this stage, it probably doesn't make much sense for the typeshed directory to be passed into the function as a parameter, and we should probably change the name of the function 😄
There was a problem hiding this comment.
Sorry about that, I figured it would be okay to modify in this way since it wasn't being used anywhere else and not in a place where it could be imported from anywhere else. See #15372
## Summary See #15370 (comment): - Rename `zip_dir` to `write_zipped_typeshed_to` to clarify it's not a generic function (anymore) - Hard-code `TYPESHED_SOURCE_DIR` instead of using a `directory_path` argument
Summary
The symlink-approach in the typeshed-sync workflow caused some problems on Windows, even though it seemed to work fine in CI.
Here, we rely on
build.rsto patch typeshed instead, which allows us to get rid of the modifications in the workflow (thank you @MichaReiser).Test Plan
knot_extensions.pyiresult in a recompile ofred_knot_vendored.