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

feat: Saving the nogo fixes #4102

Merged
merged 12 commits into from
Jan 9, 2025

Conversation

peng3141
Copy link
Contributor

@peng3141 peng3141 commented Sep 12, 2024

What type of PR is this?

Feature

What does this PR do? Why is it needed?

nogo analyzers may produce fixes as analysis.Diagnostic. This PR allows rules_go to save the fixes as patches, one per target. The patch and the command to apply the patch is printed out to the terminal for users to manually apply. The patch is also available in the "nogo_fix" output group. This allows people to get patches for all targets without failing the build by passing --norun_validations --output_groups nogo_fix.

Example output:

Use --sandbox_debug to see verbose messages from the sandbox and retain the sandbox build root for debugging

nogo: errors found by nogo during build-time code analysis:
src/example.com/send_request.go:133:4: Add and Done should not both exist inside the same goroutine block (example_nogo_analyzer)

-------------------Suggested Fix-------------------
--- src/example.com/send_request.go
+++ src/example.com/send_request.go
@@ -123,6 +123,7 @@
 
 	for !isComplete {
 		<-ticker.C
+		concurrentJobs.Add(1)
 		go func() {
 			text, hasMore := <-line
 			if !hasMore {
@@ -130,7 +131,6 @@
 				return
 			}
 
-			concurrentJobs.Add(1)
 			defer concurrentJobs.Done()
 
 			atomic.AddInt32(&totalSend, 1)

-----------------------------------------------------

To apply the suggested fix, run the following command:
$ patch -p1 < bazel-out/k8-fastbuild/bin/src/example.com/send_request.nogo.patch

Target //src/example.com:send_request failed to build
Use --verbose_failures to see the command lines of failed build steps.

Other notes for review
An analyzer may suggest multiple alternative fixes to one issue. Only the first one is selected by default, unless it conflicts with other fixes, in which case it moves on to try the next alternative. If all alternatives are tried but still have conflicts, they will be skipped. In such case, the user will have to apply the patch first, and run nogo again to get the fix to the issue.

Copy link

google-cla bot commented Sep 12, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@peng3141 peng3141 marked this pull request as draft September 12, 2024 02:07
@peng3141 peng3141 force-pushed the rules_go_hack_for_dumping_fix branch from 7b03e4c to 29f650b Compare September 24, 2024 03:03
@peng3141 peng3141 changed the title [draft][do not review] hack to get nogo fix out of bazel sandbox rules_go improvement to externalize the nogo fix Sep 24, 2024
@peng3141 peng3141 force-pushed the rules_go_hack_for_dumping_fix branch 9 times, most recently from f506b28 to c079a7f Compare September 25, 2024 15:09
@peng3141 peng3141 marked this pull request as ready for review September 25, 2024 15:33
@peng3141 peng3141 force-pushed the rules_go_hack_for_dumping_fix branch 2 times, most recently from 3b49ecb to b57424d Compare September 25, 2024 16:54
@linzhp linzhp requested a review from fmeum September 29, 2024 04:51
go/private/actions/compilepkg.bzl Show resolved Hide resolved
go/private/actions/compilepkg.bzl Outdated Show resolved Hide resolved
go/tools/builders/nogo.go Outdated Show resolved Hide resolved
go/tools/builders/nogo.go Outdated Show resolved Hide resolved
go/tools/builders/nogo.go Outdated Show resolved Hide resolved
go/tools/builders/nogo.go Outdated Show resolved Hide resolved
go/private/rules/test.bzl Outdated Show resolved Hide resolved
go/tools/builders/nogo_change.go Outdated Show resolved Hide resolved
@peng3141
Copy link
Contributor Author

peng3141 commented Oct 1, 2024

thanks for the comments, i am in the process of addressing them.

@peng3141 peng3141 force-pushed the rules_go_hack_for_dumping_fix branch 2 times, most recently from 4f41cce to f6bab4d Compare October 1, 2024 21:54
@peng3141
Copy link
Contributor Author

peng3141 commented Oct 1, 2024

this shows the log of the latest version:
https://gist.github.com/peng3141/bdaefac333434cf2ecbef4edfd8d0200

@peng3141 peng3141 force-pushed the rules_go_hack_for_dumping_fix branch 3 times, most recently from 47d7bf7 to ee5bf11 Compare October 2, 2024 15:23
@peng3141
Copy link
Contributor Author

peng3141 commented Oct 3, 2024

@fmeum @linzhp the PR is ready for review. Could you take a look? thanks!

could you focus on the high-level design, once it is agreed upon, we can have one pass to address the readability nits.

Copy link
Contributor

@linzhp linzhp left a comment

Choose a reason for hiding this comment

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

Only half way through, posting some comments so far. I will continue later this week

go/tools/builders/nogo_validation.go Show resolved Hide resolved
go/tools/builders/nogo_main.go Outdated Show resolved Hide resolved
go/tools/builders/nogo_change.go Outdated Show resolved Hide resolved
go/tools/builders/nogo_change.go Outdated Show resolved Hide resolved
@peng3141 peng3141 force-pushed the rules_go_hack_for_dumping_fix branch from ee5bf11 to 23277ea Compare October 4, 2024 14:57
go/tools/builders/nogo_change.go Outdated Show resolved Hide resolved
go/tools/builders/nogo_change_serialization.go Outdated Show resolved Hide resolved
go/tools/builders/nogo_change.go Outdated Show resolved Hide resolved
go/tools/builders/nogo_main.go Outdated Show resolved Hide resolved
go/tools/builders/nogo_change.go Outdated Show resolved Hide resolved
@peng3141
Copy link
Contributor Author

half way, will handle the rest tmr.

@peng3141 peng3141 force-pushed the rules_go_hack_for_dumping_fix branch from b00b27a to d792536 Compare December 20, 2024 18:07
@linzhp linzhp changed the title rules_go improvement to externalize the nogo fix feat: Saving the nogo fixes Dec 22, 2024
go/private/rules/nogo.bzl Show resolved Hide resolved
go/private/rules/test.bzl Outdated Show resolved Hide resolved
go/tools/builders/nogo_fix.go Outdated Show resolved Hide resolved
go/tools/builders/nogo_fix.go Show resolved Hide resolved
go/tools/builders/nogo_main.go Outdated Show resolved Hide resolved
go/tools/builders/nogo_validation.go Outdated Show resolved Hide resolved
@linzhp linzhp force-pushed the rules_go_hack_for_dumping_fix branch 4 times, most recently from f4b8542 to 925f99d Compare January 2, 2025 23:28
@peng3141
Copy link
Contributor Author

peng3141 commented Jan 6, 2025

landing is blocked by the central CI infra issue: bazelbuild/continuous-integration#2153.

@linzhp linzhp force-pushed the rules_go_hack_for_dumping_fix branch from 625a4b3 to e4d5930 Compare January 9, 2025 04:46
@linzhp linzhp merged commit 9b4c2ab into bazel-contrib:master Jan 9, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants