-
-
Notifications
You must be signed in to change notification settings - Fork 636
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: add basic support for cgo #16413
Conversation
bdca274
to
86a08f6
Compare
45bbcab
to
bf82a68
Compare
This should be complete for basic cgo support. Parts of it should probably be extracted into separate PRs: for example, the inclusion of |
bf7dc63
to
5473b39
Compare
Resource files in packages extracted to #16688. |
Extract the package name returned by the package analyzer and make it available to Go rules. This will be needed for the Cgo support to be added by #16413. [ci skip-rust]
…g) (#16688) Support placing resource files into packages just as `.go` files are put into packages (and not just embedding them in the compiled package via `//go:embed` directives). This is necessary to allow placing `.c` files into Go packages for the Cgo support to be added by #16413. [ci skip-rust]
5473b39
to
e5a02b3
Compare
|
Add `replace_sandbox_root_in_args` field to `GoSdkProcess` to teach the Go backend how to replace the string `__PANTS_SANDBOX_ROOT__` in args passed into `GoSdkProcess` with the absolute path to the sandbox root. This will be used by the forthcoming cgo support to be added by #16413. [ci skip-rust] [ci skip-build-wheels]
Fix a comment and a analysis JSON extraction nit. Extracted from cgo changes in #16413. [ci skip-build-wheels]
3b661c3
to
228b4fc
Compare
help="Name of the tool to use to compile fortran code included via CGo in a Go package.", | ||
) | ||
|
||
cgo_default_cflags = StrListOption( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bikeshed: In other Subsystems, we don't tend to use the term "default" in the option name. Is this intended to indicate something specific to cgo
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With go
, it does not use the -g -O2
default for cflags if CGO_CFLAGS
is set. But for Pants, we won't be looking at CGO_CFLAGS
so I agree makes more sense to just call this cgo_cflags
(and similar change to the other options).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍🏽
help="Default options used for FFLAGS when compiling cgo code. Equivalent to CGO_FFLAGS passed to `go`.", | ||
) | ||
|
||
cgo_default_ldflags = StrListOption( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using the term ldflags
here is done intentionally to line up with CGO's args, correct?
In lieu of something like link_flags
or link_options
- this is to be clear that this option directly passes into CGO_LDFLAGS
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I chose ldflags
to match with its use with CGO_LDFLAGS
. I assume go developers will be familiar with the go
terminology here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For reference, Bazel rules_go naming for fields on targets related to compile/link options:
copts
== CGO_CFLAGScppopts
== CGO_CPPFLAGScxxopts
== CGO_CXXFLAGSclinkopts
== CGO_LDFLAGS
Thus there is some precedent in not using cflags
/ ldflags
naming. Although I'm not particularly in favor of clinkopts
as any better than ldflags
. Going verbose here and having it be similar to the cc
backend's naming choices could lead to a more consistent experience for Pants users.
So I might consider renaming the options. Especially since I would want to add fields on the go_binary
target in a follow-on PR to allow configuring these options.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tangentially, I note that the cc
backend is using "cpp" to refer to C++ while Cgo uses "cpp" to refer to the C/C++ preprocessor and "cxx" to refer to C++. (Go will prepend the "cpp" flags to both "cflags" and "cxxflags.")
@sureshjoshi : Thoughts on cpp versus cxx naming?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally rather dislike how Bazel names options (in the CC backend especially - copts
, hdrs
, etc). Using word short-forms, no spacing, ugh - feels very 90's or 00's coding.
Anyways, I agree that keeping cgo
in line with cc
will be useful.
I'm finishing up some unit tests, then I'll push a preliminary version for a biiiiig bikeshed on naming. I'm using CPP, only because what's what I've always used for CPlusPlus.
I'm guessing Bazel is using more of a Makefile convention (https://www.gnu.org/software/make/manual/html_node/Implicit-Variables.html).
CMake uses CXX as well - https://cmake.org/cmake/help/latest/prop_tgt/CXX_STANDARD.html
I've got no opposition to changing and aligning with other tooling, I ran with CPP just so I wouldn't waste time on naming when there was a whole backend to write, and the CXX vs CPP decision would be bigger than just my opinion anyways.
In general, verbosely following existing conventions are my preference - plain English reading where possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤦🏽
Completely forgot that gcc
uses CPP as C pre-processor in docs.
I almost never touch those options directly, so I don't even think about it anymore. Probably a good reason to use cxx
and never use cpp
... Even though all my C++ files are named .cpp
) | ||
|
||
cgo_default_ldflags = StrListOption( | ||
default=lambda _: list(_DEFAULT_COMPILER_FLAGS), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a correct default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe so given the go
source here: https://github.com/golang/go/blob/202b7e7e76128f524142ac7d328fe8458a096dbf/src/cmd/go/internal/work/exec.go#L2724
I would like a second opinion though on the defaults for all of those variables being -g -O2
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, okay, I'm totally fine passing through the cgo
defaults following my principle-of-least-surprise methodology.
I was a bit surprised at -g
being passed through to ld
as I thought ld
ignores it by default, but I guess it's harmless enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, my opinion is predominantly embedded, where I almost exclusively use -Og
and -Ofast
, but I wouldn't propose those here.
353ff3f
to
c761762
Compare
[ci skip-build-wheels]
[ci skip-rust] [ci skip-build-wheels]
[ci skip-rust] [ci skip-build-wheels]
[ci skip-rust] [ci skip-build-wheels]
[ci skip-rust] [ci skip-build-wheels]
[ci skip-rust] [ci skip-build-wheels]
[ci skip-rust] [ci skip-build-wheels]
[ci skip-rust] [ci skip-build-wheels]
[ci skip-rust] [ci skip-build-wheels]
c761762
to
73e7380
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a big deal, really nice! One thing I was unclear on is why we need to replicate so much logic from exec.go rather than invoke that logic via the go tool?
Because this logic is internal to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
As opposed to what? That is, what is still missing for full Cgo support? Should those be tracked in issues? |
[ci skip-rust] [ci skip-build-wheels]
Some stuff is missing (like the "syso" support) plus I haven't ported the extensive This PR is intended as a base for that future work and allows the future work to consist of much smaller changes, which should be easier to review. Will open issues as needed. |
[ci skip-rust] [ci skip-build-wheels]
[ci skip-rust] [ci skip-build-wheels]
[ci skip-rust] [ci skip-build-wheels]
[ci skip-rust] [ci skip-build-wheels]
Add support for packages that use cgo, which allows interfacing with C code from Go.
[ci skip-rust]
[ci skip-build-wheels]