Skip to content
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

Merged
merged 2 commits into from
Dec 15, 2017
Merged

Add GoContext #1140

merged 2 commits into from
Dec 15, 2017

Conversation

ianthehat
Copy link
Contributor

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.

)
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
Copy link
Contributor

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?

Copy link
Contributor Author

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

@@ -0,0 +1,219 @@
# Copyright 2014 The Bazel Authors. All rights reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

2017

if not stdlib:
fail("No matching standard library for "+mode_string(mode))

members = structs.to_dict(toolchain.actions)
Copy link
Contributor

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")),
Copy link
Contributor

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?

Copy link
Contributor Author

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.
@ianthehat ianthehat merged commit 9031d58 into bazelbuild:master Dec 15, 2017
@ianthehat ianthehat deleted the context branch December 15, 2017 18:16
Copy link
Contributor

@jayconrod jayconrod left a 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`_.
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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 |
Copy link
Contributor

Choose a reason for hiding this comment

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

for a ... ?

sluongng added a commit to sluongng/rules_go that referenced this pull request Jun 28, 2022
`_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 (bazelbuild#1267)
  9031d58 Add GoContext (bazelbuild#1140)
  1c4f6fd Fix aspect based proto builds (bazelbuild#1131)
  ```

Let's remove the unused resolver and replace the documentation with the
resolver that is actually being used.
linzhp pushed a commit that referenced this pull request Jul 17, 2022
`_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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants