-
-
Notifications
You must be signed in to change notification settings - Fork 669
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
Add GoContext #1140
Add GoContext #1140
Conversation
) | ||
load("@io_bazel_rules_go//go/private:mode.bzl", | ||
"get_mode", | ||
load("@io_bazel_rules_go//go/private:context.bzl", #TODO: This ought to be def |
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.
Any reason not to change this?
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.
Causes a cycle, because we expose this through def.
If we are willing to break compatability, and not expose this from go:def.bzl we can fix it
go/private/context.bzl
Outdated
@@ -0,0 +1,219 @@ | |||
# Copyright 2014 The Bazel Authors. All rights reserved. |
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.
2017
go/private/context.bzl
Outdated
if not stdlib: | ||
fail("No matching standard library for "+mode_string(mode)) | ||
|
||
members = structs.to_dict(toolchain.actions) |
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.
Unused variable
"strip": attr.string(mandatory=True), | ||
# Hidden internal attributes | ||
"_stdlib_all": attr.label_list(default = _stdlib_all()), | ||
"_crosstool": attr.label(default=Label("//tools/defaults:crosstool")), |
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.
I'm surprised this works. Is this implicitly defined by Bazel?
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.
I think so, it's the same thing we were doing before, except we used to do it on the toolchain.
This collects up all dependancies that were on the toolchain, but shoudld be in target configuration (rather than host) and migrates them to a common go_context_data dependancy. It then adds a GoContext object, that holds the toolchain, build mode, standard library and all mode specific attributes. go_context(ctx) will build one of these and prepare to use it. This simplifies a lot of the logic in the rules, and helps enforce the isolation of action methods from ctx attributes.
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.
Docs LGTM, but there is a rendering problem with toolchains.rst.
@@ -56,6 +59,9 @@ repository before you call go_register_toolchains_. | |||
The toolchain | |||
~~~~~~~~~~~~~ | |||
|
|||
This a wrapper over the sdk that provides enough extras to match, target and work on a specific | |||
platforms. It should be considered an opaqute type, you only ever use it through `the context`_. |
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.
s/opaqute/opaque/
The context | ||
~~~~~~~~~~~ | ||
|
||
This is the type you use if you are writing custom rules that need |
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.
that need ... ?
@@ -12,6 +12,9 @@ Go toolchains | |||
.. _register_toolchains: https://docs.bazel.build/versions/master/skylark/lib/globals.html#register_toolchains | |||
.. _compilation modes: modes.rst#compilation-modes | |||
.. _go assembly: https://golang.org/doc/asm | |||
.. _GoLibrary: providers.rst#GoLibrary |
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.
I have no idea why, but GitHub is just showing the raw file instead of rendering it.
| :param:`resolver` | :type:`function` | :value:`None` | | ||
+--------------------------------+-----------------------------+-----------------------------------+ | ||
| This is the function that gets invoked when converting from a GoLibrary to a GoSource. | | ||
| See resolver_ for a | |
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.
for a ... ?
`_testmain_library_to_source` was introduced in 1c4f6fd and last usage of this resolver was in 56e5592. ``` > git log -S'_testmain_library_to_source' --oneline 56e5592 Move the test library rule to be go_test internal actions (bazel-contrib#1267) 9031d58 Add GoContext (bazel-contrib#1140) 1c4f6fd Fix aspect based proto builds (bazel-contrib#1131) ``` Let's remove the unused resolver and replace the documentation with the resolver that is actually being used.
`_testmain_library_to_source` was introduced in 1c4f6fd and last usage of this resolver was in 56e5592. ``` > git log -S'_testmain_library_to_source' --oneline 56e5592 Move the test library rule to be go_test internal actions (#1267) 9031d58 Add GoContext (#1140) 1c4f6fd Fix aspect based proto builds (#1131) ``` Let's remove the unused resolver and replace the documentation with the resolver that is actually being used.
This collects up all dependancies that were on the toolchain, but shoudld be in
target configuration (rather than host) and migrates them to a common
go_context_data dependancy.
It then adds a GoContext object, that holds the toolchain, build mode, standard
library and all mode specific attributes. go_context(ctx) will build one of
these and prepare to use it.
This simplifies a lot of the logic in the rules, and helps enforce the isolation
of action methods from ctx attributes.