Skip to content

Accept new #cgo directives introduced by go 1.24#23439

Merged
tdyas merged 4 commits into
pantsbuild:mainfrom
skirino:accept_new_cgo_directives_introduced_by_go1.24
Jul 2, 2026
Merged

Accept new #cgo directives introduced by go 1.24#23439
tdyas merged 4 commits into
pantsbuild:mainfrom
skirino:accept_new_cgo_directives_introduced_by_go1.24

Conversation

@skirino

@skirino skirino commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

(First of all thank you for the great build tool!)

Go 1.24 introduced new #cgo directives for performance improvements. Unfortunately the current pants go plugin does not understand the directives and results in the following error when building:

21:02:21.05 [ERROR] Failed to analyze github.com/pantsbuild/pants/testprojects/src/go/pants_test/cgo/directives for dependency inference:
invalid Go sources encountered
nocallback_noescape.go: cgo error: nocallback_noescape.go: invalid #cgo line: #cgo nocallback addNumbers

The error message was obtained by running the new test ./pants test testprojects/src/go/pants_test/cgo/directives: with the following diff (to recognize code under testprojects/src/go/).

diff --git a/pants.toml b/pants.toml
index fde22eaf8e..f3e4f56313 100644
--- a/pants.toml
+++ b/pants.toml
@@ -72,7 +72,7 @@ pants_ignore.add = [

 build_ignore.add = [
   # Disable Go targets by default so Pants developers do not need Go installed.
-  "testprojects/src/go/**",
+  # "testprojects/src/go/**",
 ]

 unmatched_build_file_globs = "error"

This PR fixes the issue by changing saveCgo function in go package analyzer. Specifically I updated saveCgo based on https://github.com/golang/go/blob/go1.26.4/src/go/build/build.go#L1710-L1713 (i.e. catch up with upstream) so that #cgo nocallback directive is not rejected.

I also added a simple cgo test case. Please let me know if you have a better idea how to test this feature.

@sureshjoshi

Copy link
Copy Markdown
Member

Thanks for the PR!

@tdyas would this be something you'd be able to review? Seems simple, but I know approximately nothing about Go.

Comment thread src/python/pants/backend/go/go_sources/analyze_package/main.go
@@ -0,0 +1,17 @@
package directives

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Source code under testprojects is not necessarily part of the Pants test suite by default. These would be better framed as part of a Python test file which sets up a Go project to be built. There should be existing CGo test files for this purpose.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

OK, thank you for the comment. I'm going to turn the test into a python test like the ones in https://github.com/pantsbuild/pants/blob/main/src/python/pants/backend/go/util_rules/cgo_test.py, so that we can have more fine-grained setup/assertions.

@skirino

skirino commented Jun 26, 2026

Copy link
Copy Markdown
Contributor Author

In 24f5353 I turned the test under testprojects/ into a python test. Also changed the CGo source code to violate "nocallback", and confirm in test that pants run causes go runtime panic as expected. Please take a look.

@skirino skirino requested a review from tdyas June 26, 2026 06:57
@tdyas tdyas added the category:bugfix Bug fixes for released features label Jul 2, 2026
@tdyas tdyas requested a review from benjyw July 2, 2026 03:40
@tdyas

tdyas commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Also needs a ./pants fmt src/python/pants/backend/go/util_rules/cgo_test.py.

@skirino skirino force-pushed the accept_new_cgo_directives_introduced_by_go1.24 branch from 24f5353 to 82a24e3 Compare July 2, 2026 07:40
@skirino

skirino commented Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

@tdyas Sorry I forgot to run the formatter. Formatting issue was resolved by 3f9745b and added a sentence to release note in 82a24e3 (I'm ready to resolve merge conflict with #23447...!)

@tdyas tdyas merged commit 9410e86 into pantsbuild:main Jul 2, 2026
25 checks passed
@skirino skirino deleted the accept_new_cgo_directives_introduced_by_go1.24 branch July 2, 2026 08:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

category:bugfix Bug fixes for released features

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants