-
-
Notifications
You must be signed in to change notification settings - Fork 668
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
Avoid involving a toolchain for the other build platform #72
Conversation
_toolchain_map = { | ||
'linux': struct( | ||
url = "https://storage.googleapis.com/golang/go1.7.linux-amd64.tar.gz", | ||
sha256 = ("702ad90f705365227e902b42d91dd1a4" + |
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.
Can you put the whole sha256 on line line? I don't think there is a strict 80 char line limit on .bzl files and the ability to grep by the sha256 without having to remember that it is just the first 32 characters is something I personally enjoy having.
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.
Nice patch, LGTM |
Resolve a go toolchain for the build platform earlier because it does not make sense to download toolchains for other build platforms. * this makes it easier to support even more platform * this makes it easier to support locally installed toolchain, c.f. #46 * this prevents "bazel query" from forcing users to download a toolchain for the other platform
68237ee
to
6fbe082
Compare
url = "https://storage.googleapis.com/golang/go1.7.linux-amd64.tar.gz", | ||
sha256 = "702ad90f705365227e902b42d91dd1a40e48ca7f67a2f4b2fd052aaa4295cd95", | ||
), | ||
'mac os x': struct( |
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.
The bazel-standard name is "darwin", so maybe just use that?
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.
It must be consistent to ctx.os.name
and it is actually "mac os x".
LGTM |
Jenkins, test this please. |
Resolve a go toolchain for the build platform earlier because it does not make sense to download toolchains for other build platforms.