-
Notifications
You must be signed in to change notification settings - Fork 402
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
Use the rust-src tool for fetching rust source #844
Conversation
rust/private/repository_utils.bzl
Outdated
Returns: | ||
str: The fully qualified url path for the specified tool. | ||
""" | ||
url = "" | ||
if iso_date: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no excuse for not having a unit test for this function :) I just uploaded #846.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(feel free to merge it and add a test for none in target_triple)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
rust/private/repository_utils.bzl
Outdated
return "{}-{}-{}".format(tool_name, version, target_triple) | ||
url += "{}/".format(iso_date) | ||
if tool_name: | ||
url += tool_name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tool is not an optional argument, let'sfail()
with an error message when None is passed. Same for version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
rust/private/repository_utils.bzl
Outdated
if tool_name: | ||
url += tool_name | ||
if version: | ||
url += "-{}".format(version) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about alternative code (not tested):
result = "-".join([e for e in [tool_name, target_triple,version] if e])
if iso_date:
return iso_date + "/" + result
else:
return result
Combining += and format is imho confusing and cumbersome.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
This tool only includes the relevant rust source files, significantly slimming the size of the download, and significantly decreasing the number of bazel targets generated. The concept of a "host tool" is introduced, which is targetless, to accommodate this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thank you!
CC @UebelAndre FYI. |
This tool only includes the relevant rust source files, significantly slimming the size of the download, and significantly decreasing the number of bazel targets generated. The concept of a "host tool" is introduced, which is targetless, to accommodate this change.
This partially fixes #833.