Skip to content

Python: upgrade clap #18797

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

Merged
merged 5 commits into from
Feb 19, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
15 changes: 6 additions & 9 deletions MODULE.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -58,15 +58,12 @@ register_toolchains("@rust_toolchains//:all")
py_deps = use_extension("//misc/bazel/3rdparty:py_deps_extension.bzl", "p")
use_repo(
py_deps,
"vendor__anyhow-1.0.44",
"vendor__cc-1.0.70",
"vendor__clap-2.33.3",
"vendor__regex-1.5.5",
"vendor__smallvec-1.6.1",
"vendor__string-interner-0.12.2",
"vendor__thiserror-1.0.29",
"vendor__tree-sitter-0.20.4",
"vendor__tree-sitter-graph-0.7.0",
"vendor_py__anyhow-1.0.95",
"vendor_py__cc-1.2.14",
"vendor_py__clap-4.5.30",
"vendor_py__regex-1.11.1",
"vendor_py__tree-sitter-0.20.4",
"vendor_py__tree-sitter-graph-0.7.0",
)

# deps for ruby+rust
Expand Down
2 changes: 1 addition & 1 deletion misc/bazel/3rdparty/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ crates_vendor(
"//python/extractor/tsg-python/tsp:Cargo.toml",
],
mode = "remote",
repository_name = "vendor",
repository_name = "vendor_py",
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure I understand why this is a problem now and not before?

Copy link
Contributor Author

@redsun82 redsun82 Feb 18, 2025

Choose a reason for hiding this comment

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

The short answer is sheer coincidence 😄

The slightly longer answer is: this is what bazel uses to prefix names of external rust dependencies of the python extractor, which then get pulled in in MODULE.bazel. The names are of the form <repository_name>__<crate name>_<version>, but it turns out they need to be disjoint from the dependencies of the other tree-sitter extractors, because then you end up defining something twice in MODULE.bazel (which in hindsight is not that weird).

Until now it just so happened that the versions of the common dependencies in the two lock files were never overlapping, until I had anyhow hit this condition after updating clap.

This does mean there is no sharing of dependencies between the two families of tree-sitter extractor, which does mean potentially rebuilding some of those when building the whole dist. In order to do share those we would need to put everything in the same workspace, and we saw that was causing an increased build cost for building just one of the two (i.e. building python was incurring overhead coming from ruby/rust). It might be that is not the case any more with this vendoring thing and the newer version of rules_rust, we might need to retest that (cc @criemen).

If you wonder why I did not rename the other crates_vendor rule, it's just because I did not want to conflict with #18789. I will probably also rename that there.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you wonder why I did not rename the other crates_vendor rule, it's just because I did not want to conflict with #18789. I will probably also rename that there.

Yes I did wonder that, thanks for the detailed answer! :-)

tags = ["manual"],
vendor_path = "py_deps",
)
4 changes: 2 additions & 2 deletions misc/bazel/3rdparty/patch_defs.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,15 @@
import re
import pathlib

label_re = re.compile(r'"@vendor//:(.+)-([\d.]+)"')
label_re = re.compile(r'"@(vendor.*)//:(.+)-([\d.]+)"')

file = pathlib.Path(sys.argv[1])
temp = file.with_suffix(f'{file.suffix}.tmp')


with open(file) as input, open(temp, "w") as output:
for line in input:
line = label_re.sub(lambda m: f'"@vendor__{m[1]}-{m[2]}//:{m[1].replace("-", "_")}"', line)
line = label_re.sub(lambda m: f'"@{m[1]}__{m[2]}-{m[3]}//:{m[2].replace("-", "_")}"', line)
output.write(line)

temp.rename(file)

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

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

107 changes: 107 additions & 0 deletions misc/bazel/3rdparty/py_deps/BUILD.anstream-0.6.18.bazel

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

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

90 changes: 90 additions & 0 deletions misc/bazel/3rdparty/py_deps/BUILD.anstyle-parse-0.2.6.bazel

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

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

Loading
Loading