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.

Sign up for GitHub

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

Jump to bottom

runtime/cgo: should not set -Werror #63903

Open
williamh opened this issue Nov 2, 2023 · 12 comments
Open

runtime/cgo: should not set -Werror #63903

williamh opened this issue Nov 2, 2023 · 12 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone
Backlog

Comments

@williamh
Copy link

williamh commented Nov 2, 2023

What version of Go are you using (go version)?

$ go version
go version go1.21.3 linux/amd64

Does this issue reproduce with the latest release?

This was reported by a user a while back for go 1.19, but looking at the source for 1.21.3 as indicated in the gentoo bug, the issue still exists.

What operating system and processor architecture are you using (go env)?

go env Output
$ go env

GO111MODULE=''
GOARCH='amd64'
GOBIN=''
GOCACHE='/home/william/.cache/go-build'
GOENV='/home/william/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMODCACHE='/home/william/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='linux'
GOPATH='/home/william/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/usr/lib/go'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='local'
GOTOOLDIR='/usr/lib/go/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='go1.21.3'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='x86_64-pc-linux-gnu-gcc'
CXX='x86_64-pc-linux-gnu-g++'
CGO_ENABLED='1'
GOMOD='/dev/null'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build1224117578=/tmp/go-build -gno-record-gcc-switches'

What did you do?

The only thing I can provide is the comment in the gentoo bug linked above.
The distro policy is that we shouldn't be setting -Werror on any compilations.

What did you expect to see?

cgo seems to force -Werror into all compilations of c code.

What did you see instead?

This should not happen according to our distro policy linked in the bug because it causes compilations to break much more often than necessary.

I don't know if you are willing to make this change upstream or not, but if you aren't, can you please advise me as the maintainer on Gentoo with regard to making this change?

Thanks for your time,

William
@cherrymui cherrymui changed the title cgo runtime sets -Werror runtime/cgo: should not set -Werror Nov 2, 2023
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Nov 2, 2023
@cherrymui
Copy link
Member

cgo seems to force -Werror into all compilations of c code.

The -Werror in the runtime/cgo package only affects the C code in the runtime/cgo package. It is not adding -Werror to user C code. Are you seeing -Werror causing failures to building the Go standard library itself?

@cherrymui cherrymui added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Nov 2, 2023
@williamh
Copy link
Author

williamh commented Nov 2, 2023

@cherrymui I have not, but here is another bug that has been marked as a duplicate and seems to show that -Werror is added to user code.

@cherrymui
Copy link
Member

The build.log.xz from that bug seems to indicate that all -Werror are applied to the runtime/cgo package itself.

@williamh
Copy link
Author

williamh commented Nov 2, 2023

Ok, that makes sense.
If I set CGO_CFLAGS to some value, would that replace the CFLAGS that are being applied to this code? I have a pending proposal to do that.

@cherrymui
Copy link
Member

If I set CGO_CFLAGS to some value, would that replace the CFLAGS that are being applied to this code?

Yes and no. CGO_CFLAGS will be added to the C compilations, but not replacing the existing flags. I don't think we document/guarantee the order of flags that are passed. So if you set CGO_CFLAGS=-Wno-error, it may or may not cancel that -Werror.

Just to add, from the build.log.xz, the -Werror doesn't seem to cause any compilation failures.

@williamh
Copy link
Author

williamh commented Nov 2, 2023

I realize it isn't causing any compilation errors, but I'm getting flagged by our distro's qa just because it is there. Their documentation suggests dropping it for releases and keeping it for development.
Can you give me a way to disable the adding of -Werror, maybe an environment variable or a command line switch that the make scripts can use?

@cherrymui
Copy link
Member

I don't think we currently have such a way. One way that I can think of is to have a wrapper script for the C compiler that scans the flags and drops -Werror before invoking the actual C compiler, and set CC to the wrapper script.

Are you proposing we introduce a way to do that?

@williamh
Copy link
Author

williamh commented Nov 2, 2023

I'm not sure that would be a good way to do it since we already set CC, so I'm going to bring one of our toolchain guys into this bug.

@thesamesam What do you think?

@cherrymui
Copy link
Member

Policy-wise, I understand that some people may not agree with using -Werror. But I also don't really understand why it is a problem if one wants their own code to be compiled with -Werror (not other's code), or in general how their own code should be compiled. Thanks.

@thesamesam
Copy link

thesamesam commented Nov 3, 2023 •
edited

The issue is it makes building Go (or something using Go, given this came up when building e.g. Podman) a pain when testing new compilers which may introduce new warnings. Those things may either not be appropriate to report to Go upstream (e.g. trying out an option which is unlikely to be suitable for widespread deployment) or need investigation first.

@williamh
Copy link
Author

williamh commented Nov 8, 2023

@cherrymui Do you have any thoughts on this?

@cherrymui
Copy link
Member

Personally I don't have strong opinion about -Werror. But I think people frequently build Go with new versions of GCC and clang, so if a newer version of GCC or clang introduces new warnings, the failure is reported in a timely base. If you test a new version of GCC or clang and see such failures, you are also welcome to report. Or are you testing more customized C toolchains?

@cherrymui cherrymui added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. and removed WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. labels Nov 8, 2023
@mknyszek mknyszek added this to the Backlog milestone Nov 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
Go Compiler / Runtime
Status: Todo
Milestone
Backlog
Development

No branches or pull requests
5 participants
@williamh @mknyszek @gopherbot @thesamesam @cherrymui