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

Allow nogo to depend on go_library #2922

Merged
merged 4 commits into from
Sep 23, 2021

Conversation

thomas-wk
Copy link
Contributor

This is a limited version of a previous PR that added a config transition to
the nogo rule that enabled it to build itself without depending on itself.

Unfortunately that PR had to be rolled back because of complexities around
host configuration. Host configuration is terminal (specifically user defined
transitions are skipped) and therefore the transition in the nogo rule that
avoids dependency cycles was skipped. This prevented all host mode tools from
building.

This PR works around host mode limitations via using a config_setting with a
default value of noop nogo. [Making host mode buildable - although without nogo].
This config_setting is controlled via 2 boolean settings - 1 indicating that
we are currently bootstrapping nogo (and therefore cannot depend on it) and
another indicating that we are requesting the nogo binary thru go_context_data rule
to execute a build. This allows target, exec and other non-hostmode configs
to use nogo (as long as we're not bootstrapping nogo).

This PR replaces go_tool_librarys in org_golang_x_tools with aliases to the
go_default_library. go_tool_library cannot be removed 100% because it is used
by other parts of go_rules (eg: coverdata) to avoid circular deps.

The original reverted PR added the capabilities to define nogo via a label_flag - and had some more extensive cleanup of the go_rules repo. PLMK how you'd like to proceed.

What type of PR is this?

Feature

What does this PR do? Why is it needed?

Creating parallel build graphs of go_tool_library inhibits the usability and extension of nogo. It is a great tool - this makes it easier to extend.

Which issues(s) does this PR fix?

Fixes #2374

Other notes for review

…tion to

the nogo rule that enabled it to build itself without depending on itself.

Unfortunately that PR had to be rolled back because of complexities around
host configuration. Host configuration is terminal (specifically user defined
transitions are skipped) and therefore the transition in the nogo rule that
avoids dependency cycles was skipped. This prevented all host mode tools from
building.

This PR works around host mode limitations via using a config_setting with a
default value of noop nogo. [Making host mode buildable - although without nogo].
This config_setting is controlled via 2 boolean settings - 1 indicating that
we are currently bootstrapping nogo (and therefore cannot depend on it) and
another indicating that we are requesting the nogo binary thru go_context_data rule
to execute a build. This allows target, exec and other non-hostmode configs
to use nogo (as long as we're not bootstrapping nogo).

This PR replaces go_tool_librarys in org_golang_x_tools with aliases to the
go_default_library. go_tool_library cannot be removed 100% because it is used
by other parts of go_rules (eg: coverdata) to avoid circular deps.
@google-cla
Copy link

google-cla bot commented Jul 13, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added the cla: no label Jul 13, 2021
@thomas-wk-cb
Copy link
Contributor

@googlebot I consent.

@google-cla google-cla bot added cla: yes and removed cla: no labels Jul 13, 2021
Copy link
Contributor

@robfig robfig left a comment

Choose a reason for hiding this comment

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

Thank you for the PR, and I apologize for the length of time it's taken to review.

I just tested this using this simple repo which pulls in staticcheck using nogo:
https://github.com/robfig/staticcheck-nogo-example

It works as expected. This is great! This unlocks staticcheck usage for Bazel repos, which we were wanting but didn't have before.

The code looks fine to me, but I am not very knowledgeable about this area so would like someone else in @bazelbuild/go-maintainers to sign off as well.

@@ -2009,8 +2009,8 @@ diff -urN b/cover/BUILD.bazel c/cover/BUILD.bazel
diff -urN b/go/analysis/BUILD.bazel c/go/analysis/BUILD.bazel
Copy link
Contributor

Choose a reason for hiding this comment

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

To confirm, these patches can't be deleted entirely because they are needed to bootstrap Gazelle, even though Gazelle now likely creates correct BUILD files now without go_tool_library?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah gazelle's default build file generation logic works fine now with nogo; so the segments of the patch where I replaced the go_tool_library targets with aliases are not strictly needed.

I left aliases to avoid any breakages in folks nogo rules at upgrade time (eg - rules_go's documentation has examples depending directly on these targets - so I assume folks have copied them into their repo's setup).

This patch file is a beast and has a lot of BUILD definitions that have nothing to do with go_tool_library or nogo, so I'm unsure if it can be entirely deleted.

@robfig
Copy link
Contributor

robfig commented Sep 22, 2021

@linzhp also tested this PR on the Uber Go repo and found it worked as expected, so we feel pretty good about this. Thanks again!

We should also remove go_tool_library documentation from go/core.rst.. do you mind adding that to the PR?

thomas-wk and others added 3 commits September 22, 2021 11:40
…tion to

the nogo rule that enabled it to build itself without depending on itself.

Unfortunately that PR had to be rolled back because of complexities around
host configuration. Host configuration is terminal (specifically user defined
transitions are skipped) and therefore the transition in the nogo rule that
avoids dependency cycles was skipped. This prevented all host mode tools from
building.

This PR works around host mode limitations via using a config_setting with a
default value of noop nogo. [Making host mode buildable - although without nogo].
This config_setting is controlled via 2 boolean settings - 1 indicating that
we are currently bootstrapping nogo (and therefore cannot depend on it) and
another indicating that we are requesting the nogo binary thru go_context_data rule
to execute a build. This allows target, exec and other non-hostmode configs
to use nogo (as long as we're not bootstrapping nogo).

This PR replaces go_tool_librarys in org_golang_x_tools with aliases to the
go_default_library. go_tool_library cannot be removed 100% because it is used
by other parts of go_rules (eg: coverdata) to avoid circular deps.
@thomas-wk
Copy link
Contributor Author

@linzhp also tested this PR on the Uber Go repo and found it worked as expected, so we feel pretty good about this. Thanks again!

We should also remove go_tool_library documentation from go/core.rst.. do you mind adding that to the PR?

done

@robfig
Copy link
Contributor

robfig commented Sep 23, 2021

Closing and reopening to re-run buildkite

@robfig
Copy link
Contributor

robfig commented Sep 23, 2021

The build failure can't possibly be related, but I don't yet have permission to re-run, so I will merge over the BuildKite complaint. Thank you!

@robfig robfig merged commit 63dfd99 into bazel-contrib:master Sep 23, 2021
@robfig robfig mentioned this pull request Oct 5, 2021
orelmisan added a commit to orelmisan/kubevirt that referenced this pull request Dec 26, 2021
For more details please see bazel-contrib/rules_go#2922
Disable nogo dep "ineffassign".

Signed-off-by: Orel Misan <omisan@redhat.com>
orelmisan added a commit to orelmisan/kubevirt that referenced this pull request Dec 26, 2021
For more details please see bazel-contrib/rules_go#2922
Disable nogo dep "ineffassign"

Signed-off-by: Orel Misan <omisan@redhat.com>
orelmisan added a commit to orelmisan/kubevirt that referenced this pull request Jan 4, 2022
For more details please see bazel-contrib/rules_go#2922
Disable nogo dep "ineffassign"

Signed-off-by: Orel Misan <omisan@redhat.com>
orelmisan added a commit to orelmisan/kubevirt that referenced this pull request Jan 6, 2022
For more details please see bazel-contrib/rules_go#2922
Disable nogo dep "ineffassign"

Signed-off-by: Orel Misan <omisan@redhat.com>
fossedihelm pushed a commit to fossedihelm/kubevirt that referenced this pull request Jan 11, 2022
For more details please see bazel-contrib/rules_go#2922
Disable nogo dep "ineffassign"

Signed-off-by: Orel Misan <omisan@redhat.com>
fossedihelm pushed a commit to fossedihelm/kubevirt that referenced this pull request Jan 12, 2022
For more details please see bazel-contrib/rules_go#2922
Disable nogo dep "ineffassign"

Signed-off-by: Orel Misan <omisan@redhat.com>
fossedihelm pushed a commit to fossedihelm/kubevirt that referenced this pull request Jan 13, 2022
For more details please see bazel-contrib/rules_go#2922
Disable nogo dep "ineffassign"

Signed-off-by: Orel Misan <omisan@redhat.com>
fossedihelm pushed a commit to fossedihelm/kubevirt that referenced this pull request Jan 17, 2022
For more details please see bazel-contrib/rules_go#2922
Disable nogo dep "ineffassign"

Signed-off-by: Orel Misan <omisan@redhat.com>
fossedihelm pushed a commit to fossedihelm/kubevirt that referenced this pull request Jan 18, 2022
For more details please see bazel-contrib/rules_go#2922
Disable nogo dep "ineffassign"

Signed-off-by: Orel Misan <omisan@redhat.com>
fossedihelm pushed a commit to fossedihelm/kubevirt that referenced this pull request Jan 27, 2022
For more details please see bazel-contrib/rules_go#2922
Disable nogo dep "ineffassign"

Signed-off-by: Orel Misan <omisan@redhat.com>
fossedihelm pushed a commit to fossedihelm/kubevirt that referenced this pull request Jan 28, 2022
For more details please see bazel-contrib/rules_go#2922
Disable nogo dep "ineffassign"

Signed-off-by: Orel Misan <omisan@redhat.com>
fossedihelm pushed a commit to fossedihelm/kubevirt that referenced this pull request Mar 9, 2022
For more details please see bazel-contrib/rules_go#2922
Disable nogo dep "ineffassign"

Signed-off-by: Orel Misan <omisan@redhat.com>
fossedihelm pushed a commit to fossedihelm/kubevirt that referenced this pull request Mar 9, 2022
For more details please see bazel-contrib/rules_go#2922
Disable nogo dep "ineffassign"

Signed-off-by: Orel Misan <omisan@redhat.com>
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.

Depend on nogo with a transition and delete go_tool_library
3 participants