#34 Add BUILDTAGS to %gobuildflags
Opened 5 months ago by bcl. Modified 3 months ago
bcl/go-rpm-macros master-BUILDTAGS  into  master

@@ -29,7 +29,7 @@ 

  #

  #    %make GOBUILDFLAGS="%gobuildflags"

  #

- %gobuildflags() %{expand:%{gocompilerflags} -tags=\\"rpm_crashtraceback \\" -ldflags \\"${LDFLAGS:-} %{?currentgoldflags} -B 0x$(head -c20 /dev/urandom|od -An -tx1|tr -d ' \\n') -compressdwarf=false -extldflags '%__global_ldflags %{?__golang_extldflags}'\\" -a -v -x}

+ %gobuildflags() %{expand:%{gocompilerflags} -tags=\\"rpm_crashtraceback ${BUILDTAGS:-}\\" -ldflags \\"${LDFLAGS:-} %{?currentgoldflags} -B 0x$(head -c20 /dev/urandom|od -An -tx1|tr -d ' \\n') -compressdwarf=false -extldflags '%__global_ldflags %{?__golang_extldflags}'\\" -a -v -x}

Please prefix it with a GO_ namespace.

  

  # Turn off Go modules

  %gomodulesmode GO111MODULE=off

Otherwise any tags set by the user are clobbered by the macro's use of
-tags.

Resolves: rhbz#1997721

FTR I think this is a very useful addition for packages for which we need to specify a tag while building them in Fedora.

Please prefix it with a GO_ namespace.

Or rather, use GO prefix instead...

No -- this needs to match the go-rpm-macros used in c9s:

https://gitlab.com/redhat/centos-stream/rpms/go-rpm-macros/-/blob/c9s/remove-fedora-dependency-automation.patch#L215

No, c9s needs to match what we do. You have done this backwards.

I have not done anything other than try to make things match so that I don't have to kludge my spec file.

+1 to GOBUILDTAGS. The macros already make use of GOBUILDFLAGS, so this follows convention. It also avoids the (albeit narrow) situation where BUILDTAGS is already assumed to mean "C build tags" and a spec file is attempting to modify both CGo build configurations and Go build configurations. It's trivial to adjust existing spec files to use GOBUILDTAGS instead of BUILDTAGS.

I second the @ngompa 's take here.

I'm actually confused why %gobuildflags exists, because %gobuild below that doesn't use it. In fact, %gobuild uses an exact copy of gobuildflags with BUILDTAGS already there.

I guess gobuildflags is there if you need to somehow call go build manually? But in that case adding BUILDTAGS to gobuildflags is only halfway to making things consistent. gobuild should also be changed to use gobuildflags, or things will fall out of sync again.

I'm actually confused why %gobuildflags exists, because %gobuild below that doesn't use it. In fact, %gobuild uses an exact copy of gobuildflags with BUILDTAGS already there.

I guess gobuildflags is there if you need to somehow call go build manually? But in that case adding BUILDTAGS to gobuildflags is only halfway to making things consistent. gobuild should also be changed to use gobuildflags, or things will fall out of sync again.

This does look like an optimization that could be made to tidy up the macro a bit. Feel free to PR this if you feel like it's a good idea. I traced through the commit history and found the commit[1] where the %gobuildflags macro was created. It looks like it might have been simply an oversight to not update the %gobuild macro to use the new %gobuildflags macro.

This is the last we should say on this subject on this PR though; this is not actually related to this PR and should be discussed on a separate PR or issue.

1: https://pagure.io/go-rpm-macros/c/67b4fbbbfce0986ac46cd1329bf85a18ea7a43d2

+1 to GOBUILDTAGS. The macros already make use of GOBUILDFLAGS, so this follows convention. It also avoids the (albeit narrow) situation where BUILDTAGS is already assumed to mean "C build tags" and a spec file is attempting to modify both CGo build configurations and Go build configurations. It's trivial to adjust existing spec files to use GOBUILDTAGS instead of BUILDTAGS.

I think I might be changing my opinion on this. Looking at the macros again, internally, they look for variables such as BUILDTAGS and LDFLAGS[1]. The GOBUILDFLAGS mention only actually occurs in a comment, proposing an example usage of declaring a make or autoconf variable called GOBUILDFLAGS (the assumption being that the Makefile or configure.ac is looking for the variable GOBUILDFLAGS in order to pass it to a call to the compiler directly). The %gobuildflags macro and the %gobuild macro both look for LDFLAGS internally to expand the value it passed to the -ldflags option. Therefore, updating the %gobuildflags macro to use BUILDTAGS, just like the %gobuild macro is already doing, makes a lot of sense.

1: https://pagure.io/go-rpm-macros/blob/master/f/rpm/macros.d/macros.go-compilers-golang#_43

@linkdupont looking at the macros. LDFLAGS should have been passed in to the -ldflags='-extldflags "$LDFLAGS"'. I belive this is a bug in macros as generic "C" ldflags shouldn't really work well with the GC's internal linker. AFAIK at least there is no guarantee that they will. I don't know how I have missed that.

Would a value in the environment variable LDFLAGS get pulled in by the -extldflags '%__global_ldflags %{?__golang_extldflags}' part? Does %__global_ldflags or %__global_extldflags resolve to including $LDFLAGS?

Metadata