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

CLI: explicit integration with cgo #7377

Closed
wants to merge 1 commit into from
Closed

Conversation

andrewrk
Copy link
Member

@andrewrk andrewrk commented Dec 10, 2020

This is just a proof-of-concept. Maybe it could be nice for Go
developers, but also maybe we don't put additional complexity into the
zig codebase when CLI flags work just fine.

See the discussion in #7342

@andrewrk andrewrk added the proposal This issue suggests modifications. If it also has the "accepted" label then it is planned. label Dec 10, 2020
This is just a proof-of-concept. Maybe it could be nice for Go
developers, but also maybe we don't put additional complexity into the
zig codebase when CLI flags work just fine.
Comment on lines +386 to +387
if (std.mem.eql(u8, go_arch, "386")) {
cpu_arch = null;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (std.mem.eql(u8, go_arch, "386")) {
cpu_arch = null;
if (std.mem.eql(u8, go_arch, "native")) {
cpu_arch = null;

@lu4p
Copy link

lu4p commented Dec 10, 2020

I think this would be great, to onboard members of the go community to the zig project.

return error.UnsupportedOperatingSystem;
} else if (std.mem.eql(u8, go_os, "linux")) {
os_tag = .linux;
abi = .musl; // go wants static linking on linux
Copy link

Choose a reason for hiding this comment

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

I think this is an ok default behaviour, but this should be configurable via a flag or env var.

@LemonBoy
Copy link
Contributor

I don't think that writing a shell script to perform the argument translation is that hard.
On the other hand maintaining this set of extra switches and making sure they don't regress is much harder.

Comment on lines +154 to +156
// Workaround for https://github.com/golang/go/issues/43078, here we fix the order of
// command line arguments. Dear Go developers please let the Zig project know when
// we can remove this!
Copy link

Choose a reason for hiding this comment

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

https://go-review.googlesource.com/c/go/+/276412/ which is under review may fix this.

@jonjohnsonjr
Copy link

I don't think that writing a shell script to perform the argument translation is that hard.
On the other hand maintaining this set of extra switches and making sure they don't regress is much harder.

I tend to agree, though I don't personally want to be responsible for maintaining this mapping (and it seems silly to have everyone individually reimplement it), but it's not clear where something like that should live.

I'm not sure what the minimum bar is for creating a repository is under the ziglang org, but something like a github.com/ziglang/zig-cgo (or cgo-zig) that hosts the canonical translation layer would be a very nice thing to be able to reference. There seems to be precedent for this with docker-zig, zig.vim, vscode-zig, zig-mode, etc...

@andrewrk
Copy link
Member Author

The more I think about it, the more I think there should not be a translation layer at all - typically when you are creating a cross build, you have in mind the target that you want to specify. You will then have the task of choosing a -target / -mcpu flags for Zig and GOOS/GOARCH env vars for go. It's possible that the target you want to give to Zig can't even be described by GOOS/GOARCH. For example OS version ranges, CPU features, dynamic linker path, C ABI, glibc version. In this case you must manually choose the Zig target configuration. So I think we may actually be doing a disservice by leading users to configure the much more detailed Zig target configuration via the coarse-grained go env vars.

Based on this, and @LemonBoy's point, I'm going to close this. I do, however, want to solve the problem of having zig choose a more effective local cache directory. (See golang/go#43078 (comment) for more details on that.)

@andrewrk andrewrk closed this Dec 10, 2020
@andrewrk andrewrk deleted the cgo-integration branch December 10, 2020 18:53
@andrewrk andrewrk restored the cgo-integration branch December 10, 2020 18:54
@andrewrk andrewrk deleted the cgo-integration branch December 29, 2021 19:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
proposal This issue suggests modifications. If it also has the "accepted" label then it is planned.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants