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.

Sign up for GitHub

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

Jump to bottom

gazelle_binary: deprecate and remove mode attributes #803

Closed
jayconrod opened this issue Jun 2, 2020 · 7 comments · Fixed by #882
Closed

gazelle_binary: deprecate and remove mode attributes #803

jayconrod opened this issue Jun 2, 2020 · 7 comments · Fixed by #882
Labels
cleanup

Comments

@jayconrod
Copy link
Contributor

gazelle_binary currently supports the attributes pure, static, msan, race, goos, and goarch.

Supporting these attributes requires either go_archive_aspect or go_transition_binary, both of which are internal to rules_go. Using them in Gazelle makes it difficult to change go_transition_binary safely.

None of these attributes should be necessary. gazelle_binary should almost always be built for the host platform in the default configuration. If it's necessary to build for a different configuration, that can be done by setting the relevant build settings on the command line (for example, --@io_bazel_rules_go//go/config:static) or by targeting an appropriate platform.
jayconrod pushed a commit to jayconrod/bazel-gazelle that referenced this issue Jun 2, 2020
gazelle_binary: use transition instead of aspect
59e883b 
go_archive_aspect should no longer be used anywhere. It probably
should have been deleted before v0.21.0. gazelle_binary has been using
it for cross compilation though.

With this change, go_transition_rule is used for cross-compilation
instead.

Cross-compilation in gazelle_binary should be removed entirely in the
future though. It requires private functionality in rules_go, and it
doesn't seem useful.

For bazelbuild/rules_go#2523
Updates bazelbuild#803
@jayconrod jayconrod mentioned this issue Jun 2, 2020
Merged
jayconrod pushed a commit that referenced this issue Jun 2, 2020
gazelle_binary: use transition instead of aspect (#804)
25317b3 
go_archive_aspect should no longer be used anywhere. It probably
should have been deleted before v0.21.0. gazelle_binary has been using
it for cross compilation though.

With this change, go_transition_rule is used for cross-compilation
instead.

Cross-compilation in gazelle_binary should be removed entirely in the
future though. It requires private functionality in rules_go, and it
doesn't seem useful.

For bazelbuild/rules_go#2523
Updates #803
@jayconrod jayconrod mentioned this issue Jun 8, 2020
Merged
@dududko
Copy link
Contributor

dududko commented Jun 17, 2020

I think that it would not be an issue if gazelle_binary was split on two parts:

  1. go_library
  2. go_binary

(1) contains all the magic with go_context(ctx); (2) is a direct use of go_binary rule (with a proper use of goos/goarch).

Please correct me if I am wrong.

@jayconrod
Copy link
Contributor Author

Maybe... whatever depends on the go_binary would need to reset all the build settings to default values though, and that causes problems like bazelbuild/rules_go#2538. In any case, I'd rather not wire this together with genrules and macros though.

Why not remove these attributes? gazelle_binary should really only be built for the host configuration. I can't think of why it would need race instrumentation for example.

@dududko
Copy link
Contributor

dududko commented Jun 17, 2020

In out team we use executable rule to push build artifacts to s3 (with executable binary written in go) and we build artifacts for both linux and darwin. goarch/goos allow us to both build and push all artifacts with a single bazel run command, which is very convenient because id does not require to have some side scripts for this job.

Since we extended gazelle with some custom logic for typescript, we also do the release management for gazelle binary for both linux and darwin.

@jayconrod
Copy link
Contributor Author

That seems like a pretty customized situation. I don't think that justifies keeping these attributes for everyone, since they're causing a maintenance problem.

achew22 added a commit to achew22/rules_go that referenced this issue Aug 24, 2020
@achew22
Create bzl_library entries using gazelle-skylark
97ba01f 
There are a few minor manual modifications that needed to be performed.
For example, some `.bzl` files depend on `.bzl` files that are generated
outside of this repo and do not presently exist. `@bazel_tools` is a
great example of this.

Additionally this effort has revealed that bazel gazelle has an explicit
dependency on the private parts of rules_go in
internal/gazelle_binary.bzl on line 21 where it imports from
`go/private:rules/transitions.bzl` which is no longer the path to import
from once I created the BUILD file in `go/private/rules`.

As a temporary fix for this, I'm included a patch that changes that to
point to the new path. I will provide a follow up patch once rules_go
has been released to fix it properly in the gazelle repo and then remove
the patch from rules_go.

Fixed: bazelbuild#2619
Bug: bazelbuild/bazel-skylib#250
Bug: bazelbuild/bazel-gazelle#803
@achew22 achew22 mentioned this issue Aug 24, 2020
Merged
@jayconrod
Copy link
Contributor Author

This is blocking bazelbuild/rules_go#2621.

@achew22
Copy link
Member

achew22 commented Aug 24, 2020

@jayconrod would you like me to take a look into this? aka is this a blocker AI for me? It's going to take me quite a while to get context on it unfortunately. I've never poked the transitions part of rules_go before.

@dududko, it seems to me the standard cross compilation stuff for rules_go is sufficient to meet the use-case you've laid out here. I expect that I'm missing something though. Could you speak about why --platforms=@io_bazel_rules_go//go/toolchain:linux_amd64_cgo (and the associated one for darwin) wouldn't be sufficient?

@jayconrod
Copy link
Contributor Author

@achew22 I'll try and fix this today. I'd like to get it into the next Gazelle minor release this week.

@jayconrod jayconrod mentioned this issue Aug 25, 2020
Closed
jayconrod pushed a commit to jayconrod/bazel-gazelle that referenced this issue Aug 26, 2020
gazelle_binary: remove mode attributes
05f4e25 
This PR removes mode attributes (goos, goarch, race, etc.) from the
gazelle_binary rule. These attributes were implemented using private
files in rules_go, so supporting them is blocking changes in the
rules_go implementation.

These attributes should probably never be used. gazelle_binary should
generally be built for the host configuration (by 'bazel run' or
'bazel build' without configuration flags). If a binary is still
needed in a different configuration, that may still be accomplished
with command-line flags like --platforms or
--@io_bazel_rules_go//go/config:race.

Fixes bazelbuild#803
@jayconrod jayconrod mentioned this issue Aug 26, 2020
Merged
jayconrod pushed a commit that referenced this issue Aug 26, 2020
gazelle_binary: remove mode attributes (#882)
7033f2c 
This PR removes mode attributes (goos, goarch, race, etc.) from the
gazelle_binary rule. These attributes were implemented using private
files in rules_go, so supporting them is blocking changes in the
rules_go implementation.

These attributes should probably never be used. gazelle_binary should
generally be built for the host configuration (by 'bazel run' or
'bazel build' without configuration flags). If a binary is still
needed in a different configuration, that may still be accomplished
with command-line flags like --platforms or
--@io_bazel_rules_go//go/config:race.

Fixes #803
achew22 added a commit to achew22/rules_go that referenced this issue Aug 27, 2020
@achew22 @aherrmann
Create bzl_library entries using gazelle-skylark
5536b1e 
There are a few minor manual modifications that needed to be performed.
For example, some `.bzl` files depend on `.bzl` files that are generated
outside of this repo and do not presently exist. `@bazel_tools` is a
great example of this.

Additionally this effort has revealed that bazel gazelle has an explicit
dependency on the private parts of rules_go in
internal/gazelle_binary.bzl on line 21 where it imports from
`go/private:rules/transitions.bzl` which is no longer the path to import
from once I created the BUILD file in `go/private/rules`.

As a temporary fix for this, I'm included a patch that changes that to
point to the new path. I will provide a follow up patch once rules_go
has been released to fix it properly in the gazelle repo and then remove
the patch from rules_go.

Fixed: bazelbuild#2619
Bug: bazelbuild/bazel-skylib#250
Bug: bazelbuild/bazel-gazelle#803
Co-authored-by: Andreas Herrmann <andreas.herrmann@tweag.io>
achew22 added a commit to achew22/rules_go that referenced this issue Aug 27, 2020
@achew22 @aherrmann
Create bzl_library entries using gazelle-skylark
8e545b0 
There are a few minor manual modifications that needed to be performed.
For example, some `.bzl` files depend on `.bzl` files that are generated
outside of this repo and do not presently exist. `@bazel_tools` is a
great example of this.

Additionally this effort has revealed that bazel gazelle has an explicit
dependency on the private parts of rules_go in
internal/gazelle_binary.bzl on line 21 where it imports from
`go/private:rules/transitions.bzl` which is no longer the path to import
from once I created the BUILD file in `go/private/rules`.

As a temporary fix for this, I'm included a patch that changes that to
point to the new path. I will provide a follow up patch once rules_go
has been released to fix it properly in the gazelle repo and then remove
the patch from rules_go.

Fixed: bazelbuild#2619
Bug: bazelbuild/bazel-skylib#250
Bug: bazelbuild/bazel-gazelle#803
Co-authored-by: Andreas Herrmann <andreas.herrmann@tweag.io>
achew22 added a commit to achew22/rules_go that referenced this issue Aug 27, 2020
@achew22 @aherrmann
Create bzl_library entries using gazelle-skylark
bb49af0 
There are a few minor manual modifications that needed to be performed.
For example, some `.bzl` files depend on `.bzl` files that are generated
outside of this repo and do not presently exist. `@bazel_tools` is a
great example of this.

Additionally this effort has revealed that bazel gazelle has an explicit
dependency on the private parts of rules_go in
internal/gazelle_binary.bzl on line 21 where it imports from
`go/private:rules/transitions.bzl` which is no longer the path to import
from once I created the BUILD file in `go/private/rules`.

As a temporary fix for this, I'm included a patch that changes that to
point to the new path. I will provide a follow up patch once rules_go
has been released to fix it properly in the gazelle repo and then remove
the patch from rules_go.

Fixed: bazelbuild#2619
Bug: bazelbuild/bazel-skylib#250
Bug: bazelbuild/bazel-gazelle#803
Co-authored-by: Andreas Herrmann <andreas.herrmann@tweag.io>
achew22 added a commit to achew22/rules_go that referenced this issue Sep 22, 2020
@achew22 @aherrmann
Create bzl_library entries using gazelle-skylark
4b4348d 
There are a few minor manual modifications that needed to be performed.
For example, some `.bzl` files depend on `.bzl` files that are generated
outside of this repo and do not presently exist. `@bazel_tools` is a
great example of this.

Additionally this effort has revealed that bazel gazelle has an explicit
dependency on the private parts of rules_go in
internal/gazelle_binary.bzl on line 21 where it imports from
`go/private:rules/transitions.bzl` which is no longer the path to import
from once I created the BUILD file in `go/private/rules`.

As a temporary fix for this, I'm included a patch that changes that to
point to the new path. I will provide a follow up patch once rules_go
has been released to fix it properly in the gazelle repo and then remove
the patch from rules_go.

Fixed: bazelbuild#2619
Bug: bazelbuild/bazel-skylib#250
Bug: bazelbuild/bazel-gazelle#803
Co-authored-by: Andreas Herrmann <andreas.herrmann@tweag.io>
achew22 added a commit to achew22/rules_go that referenced this issue Oct 22, 2020
@achew22 @aherrmann
Create bzl_library entries using gazelle-skylark
7621757 
There are a few minor manual modifications that needed to be performed.
For example, some `.bzl` files depend on `.bzl` files that are generated
outside of this repo and do not presently exist. `@bazel_tools` is a
great example of this.

Additionally this effort has revealed that bazel gazelle has an explicit
dependency on the private parts of rules_go in
internal/gazelle_binary.bzl on line 21 where it imports from
`go/private:rules/transitions.bzl` which is no longer the path to import
from once I created the BUILD file in `go/private/rules`.

As a temporary fix for this, I'm included a patch that changes that to
point to the new path. I will provide a follow up patch once rules_go
has been released to fix it properly in the gazelle repo and then remove
the patch from rules_go.

Fixed: bazelbuild#2619
Bug: bazelbuild/bazel-skylib#250
Bug: bazelbuild/bazel-gazelle#803
Co-authored-by: Andreas Herrmann <andreas.herrmann@tweag.io>
jayconrod pushed a commit to bazelbuild/rules_go that referenced this issue Oct 23, 2020
@achew22
Create bzl_library entries using gazelle-skylark (#2621)
2bada59 
There are a few minor manual modifications that needed to be performed.
For example, some .bzl files depend on .bzl files that are generated
outside of this repo and do not presently exist. @bazel_tools is a
great example of this.

Additionally this effort has revealed that bazel gazelle has an explicit
dependency on the private parts of rules_go in
internal/gazelle_binary.bzl on line 21 where it imports from
go/private:rules/transitions.bzl which is no longer the path to import
from once I created the BUILD file in go/private/rules.

As a temporary fix for this, I'm included a patch that changes that to
point to the new path. I will provide a follow up patch once rules_go
has been released to fix it properly in the gazelle repo and then remove
the patch from rules_go.

Fixed: #2619
Bug: bazelbuild/bazel-skylib#250
Bug: bazelbuild/bazel-gazelle#803
@dududko dududko mentioned this issue Sep 27, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
cleanup
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

gazelle_binary: remove mode attributes jayconrod/bazel-gazelle
3 participants
@achew22 @jayconrod @dududko