[red-knot] Typeshed patching: use build.rs instead of workflow #15370
Conversation
sharkdp
added
internal
sharkdp
requested review from
carljm,
MichaReiser and
AlexWaygood
as code owners
sharkdp
force-pushed
the
david/typeshed_sync-copy-instead-of-symlink
branch
from
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!
| zip.finish() | ||
| } | ||
|
|
||
| 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.
|
sharkdp
changed the title
| // 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.