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

go: deprecate the go_rule wrapper #2302

Open
jayconrod opened this issue Dec 2, 2019 · 2 comments
Open

go: deprecate the go_rule wrapper #2302

jayconrod opened this issue Dec 2, 2019 · 2 comments

Comments

@jayconrod
Copy link
Contributor

See bazel-contrib/bazel-gazelle#676.

A Bazel incompatibility flag (possibly --incompatible_remap_main_repo, see bazelbuild/bazel#7130) causes default labels in attributes to be resolved in @io_bazel_rules_go//go/private (where go_rule is defined) instead of wherever the rule should actually have been defined.

We use go_rule as a wrapper around rule to add hidden attributes and a toolchain dependency. Instead, we should move hidden attributes to the toolchain or provide a set of common attributes. The toolchain dependency can be explicit.

jayconrod pushed a commit to jayconrod/bazel-gazelle that referenced this issue Dec 2, 2019
This is a workaround for a visibility issue, possibly introduced by
the --incompatible_remap_main_repo flag or something similar.

We should stop using go_rule though.

Fixes bazel-contrib#676
Updates bazel-contrib/rules_go#2302
jayconrod pushed a commit to bazel-contrib/bazel-gazelle that referenced this issue Dec 2, 2019
This is a workaround for a visibility issue, possibly introduced by
the --incompatible_remap_main_repo flag or something similar.

We should stop using go_rule though.

Fixes #676
Updates bazel-contrib/rules_go#2302
jayconrod pushed a commit to jayconrod/rules_go that referenced this issue Jan 8, 2020
jayconrod pushed a commit that referenced this issue Jan 8, 2020
@jmhodges
Copy link
Contributor

I see the deprecation commit made it in and would love to hear what the migration should look like! (bazel_gomock is still using go_rule, so it's relevant to my interests)

@jayconrod
Copy link
Contributor Author

@jmhodges I apologize for creating a classic Google "old thing is deprecated, new thing is not ready yet" dilemma. I thought it was important to mark this is deprecated, given that there are potential problems caused by Bazel.

The change I would like to make is for a skeleton Go rule to look something like this:

go_example = rule(
    implementation = _go_example_impl,
    attrs = {
        "_go_context": attr.label(default = "@io_bazel_rules_go//:go_context_data"),
    },
    toolchains = ["@io_bazel_rules_go//go:toolchain"],
)

The rule would get nearly all of the information it needs from the toolchain. There are a few things that must be configured for the target platform (C toolchain, flags), and toolchains are always configured for the host platform, so we still need go_context_data to provide those.

After this is ready and rules_go is migrated, go_rule would continue to be available for at least a full release cycle (about a quarter). It may be reduced to just adding the _go_context attribute and the toolchain.

jayconrod pushed a commit that referenced this issue Mar 26, 2020
This changes how the goos, goarch, race, msan, static, and pure
attributes of go_binary and go_test are implemented.

Previously, the go_binary and go_test rules used an aspect on the deps
and embed attributes. If any of the mode attributes listed above were
set, the aspect declared additional output files and actions to
generate them for the specified configuration. This approach had two
significant problems. First, the aspect caused the analysis to be
performed twice, wasting time and memory even in the case
where no cross-compilation was needed. Second, the aspect was not
compatible with conditional dependencies specified with select
expressions: aspects run on the configured target graph, but the mode
attributes could not affect Bazel's configuration.

This change deletes the aspect and implements these attributes using a
different mechanism: build settings and configuration transitions.

A new package is added //go/config with a build setting for each
attribute. Most settings are flags that may be set on the
command-line. This is an alternative to --feature flags, which will be
deprecated and eventually removed.

The target //:go_config gathers build settings relevant to Go and
provides a GoContextInfo (private).

The target //:go_context_data depends on //:go_config, //:stdlib,
//:nogo, and a few other standard dependencies. go_binary and go_test
rules no longer need to depend on these targets directly. Go rules
should now only need to depend on //:go_context_data and the Go
toolchain. Unfortunately, neither //:go_context_data nor //:go_config
can be part of the toolchain because the toolchain is always analyzed
for the execution configuration, not the target configuration.

Because of the simplified dependencies, the go_rule function is no
longer needed by most rules. This change makes it possible to
deprecate it.

Fixes #1351
Fixes #2219
Updates #2302
jayconrod pushed a commit that referenced this issue Apr 17, 2020
This changes how the goos, goarch, race, msan, static, and pure
attributes of go_binary and go_test are implemented.

Previously, the go_binary and go_test rules used an aspect on the deps
and embed attributes. If any of the mode attributes listed above were
set, the aspect declared additional output files and actions to
generate them for the specified configuration. This approach had two
significant problems. First, the aspect caused the analysis to be
performed twice, wasting time and memory even in the case
where no cross-compilation was needed. Second, the aspect was not
compatible with conditional dependencies specified with select
expressions: aspects run on the configured target graph, but the mode
attributes could not affect Bazel's configuration.

This change deletes the aspect and implements these attributes using a
different mechanism: build settings and configuration transitions.

A new package is added //go/config with a build setting for each
attribute. Most settings are flags that may be set on the
command-line. This is an alternative to --feature flags, which will be
deprecated and eventually removed.

The target //:go_config gathers build settings relevant to Go and
provides a GoContextInfo (private).

The target //:go_context_data depends on //:go_config, //:stdlib,
//:nogo, and a few other standard dependencies. go_binary and go_test
rules no longer need to depend on these targets directly. Go rules
should now only need to depend on //:go_context_data and the Go
toolchain. Unfortunately, neither //:go_context_data nor //:go_config
can be part of the toolchain because the toolchain is always analyzed
for the execution configuration, not the target configuration.

Because of the simplified dependencies, the go_rule function is no
longer needed by most rules. This change makes it possible to
deprecate it.

Fixes #1351
Fixes #2219
Updates #2302
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants