bcl opened a new pull-request against the project: `go-rpm-macros` that you are following: `` Add BUILDTAGS to %gobuildflags ``
To reply, visit the link below or just reply to this email https://pagure.io/go-rpm-macros/pull-request/34
obudai commented on the pull-request: `Add BUILDTAGS to %gobuildflags` that you are following: `` FTR I think this is a very useful addition for packages for which we need to specify a tag while building them in Fedora. ``
To reply, visit the link below or just reply to this email https://pagure.io/go-rpm-macros/pull-request/34
ngompa commented on the pull-request: `Add BUILDTAGS to %gobuildflags` that you are following: `` Please prefix it with a `GO_` namespace. ``
To reply, visit the link below or just reply to this email https://pagure.io/go-rpm-macros/pull-request/34
ngompa commented on the pull-request: `Add BUILDTAGS to %gobuildflags` that you are following: `` Or rather, use `GO` prefix instead... ``
To reply, visit the link below or just reply to this email https://pagure.io/go-rpm-macros/pull-request/34
bcl commented on the pull-request: `Add BUILDTAGS to %gobuildflags` that you are following: `` 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... ``
To reply, visit the link below or just reply to this email https://pagure.io/go-rpm-macros/pull-request/34
ngompa commented on the pull-request: `Add BUILDTAGS to %gobuildflags` that you are following: ``
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...
No, `c9s` needs to match what _we_ do. You have done this backwards. ``
To reply, visit the link below or just reply to this email https://pagure.io/go-rpm-macros/pull-request/34
bcl commented on the pull-request: `Add BUILDTAGS to %gobuildflags` that you are following: `` *I* have not done anything other than try to make things match so that I don't have to kludge my spec file.
``
To reply, visit the link below or just reply to this email https://pagure.io/go-rpm-macros/pull-request/34
ngompa commented on the pull-request: `Add BUILDTAGS to %gobuildflags` that you are following: `` :weary: ``
To reply, visit the link below or just reply to this email https://pagure.io/go-rpm-macros/pull-request/34
bcl commented on the pull-request: `Add BUILDTAGS to %gobuildflags` that you are following: `` https://bugzilla.redhat.com/show_bug.cgi?id=1999335
``
To reply, visit the link below or just reply to this email https://pagure.io/go-rpm-macros/pull-request/34
linkdupont commented on the pull-request: `Add BUILDTAGS to %gobuildflags` that you are following: `` +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`. ``
To reply, visit the link below or just reply to this email https://pagure.io/go-rpm-macros/pull-request/34
jcajka commented on the pull-request: `Add BUILDTAGS to %gobuildflags` that you are following: `` I second the @ngompa 's take here. ``
To reply, visit the link below or just reply to this email https://pagure.io/go-rpm-macros/pull-request/34
qulogic commented on the pull-request: `Add BUILDTAGS to %gobuildflags` that you are following: `` 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. ``
To reply, visit the link below or just reply to this email https://pagure.io/go-rpm-macros/pull-request/34
linkdupont commented on the pull-request: `Add BUILDTAGS to %gobuildflags` that you are following: ``
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. ``
To reply, visit the link below or just reply to this email https://pagure.io/go-rpm-macros/pull-request/34
linkdupont commented on the pull-request: `Add BUILDTAGS to %gobuildflags` that you are following: ``
+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-compile... ``
To reply, visit the link below or just reply to this email https://pagure.io/go-rpm-macros/pull-request/34
jcajka commented on the pull-request: `Add BUILDTAGS to %gobuildflags` that you are following: `` @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. ``
To reply, visit the link below or just reply to this email https://pagure.io/go-rpm-macros/pull-request/34
linkdupont commented on the pull-request: `Add BUILDTAGS to %gobuildflags` that you are following: `` 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?
``
To reply, visit the link below or just reply to this email https://pagure.io/go-rpm-macros/pull-request/34
rishi commented on the pull-request: `Add BUILDTAGS to %gobuildflags` that you are following: ``
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.
A bit of Git forensics [reveals](https://pagure.io/go-rpm-macros/c/67b4fbbbfce0986ac46cd1329bf85a18ea7a43d2): ``` commit 67b4fbbbfce0986ac46cd1329bf85a18ea7a43d2 Author: Daniel P. Berrangé berrange@redhat.com AuthorDate: Wed Sep 18 16:49:58 2019 +0100 Commit: Daniel P. Berrangé berrange@redhat.com CommitDate: Thu Sep 19 11:36:33 2019 +0100
macros: define a %gobuildflags macro
Using the %gobuild macro is fine for a project where the go code is the only thing being built, and can be built directly by invoking the Go toolchain from RPM.
In more complex cases though, the Go code is just a small part of the project and the Go toolchain is invoked by a build system such as make (possibly automake), or meson. In such a case we need to be able to tell this build system what flags to pass to the compiler.
The %gobuildflags macros services this purpose allowing a RPM spec todo
GOBUILDFLAGS="%gobuildflags" %configure
or
%make GOBUILDFLAGS="%gobuildflags"
Ideally the %gobuild macro would in turn reference the %gobuildflags macro, but that does not appear possible given the semantics around quote expansion and escaping across RPM and shell.
Signed-off-by: Daniel P. Berrangé berrange@redhat.com ``` ``
To reply, visit the link below or just reply to this email https://pagure.io/go-rpm-macros/pull-request/34
rishi commented on the pull-request: `Add BUILDTAGS to %gobuildflags` that you are following: `` Speaking as someone who maintains a code base where `go build` is invoked by Meson ...
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.
Maybe something has changed since then, and my memory is a bit foggy because it's been a while since my deep dive into the Go toolchain and the RPM macros, but it was definitely not an oversight to have `%gobuildflags` duplicate `%gobuild`. The [commit](https://pagure.io/go-rpm-macros/c/67b4fbbbfce0986ac46cd1329bf85a18ea7a43d2) says: ``` Ideally the %gobuild macro would in turn reference the %gobuildflags macro, but that does not appear possible given the semantics around quote expansion and escaping across RPM and shell. ``` ``
To reply, visit the link below or just reply to this email https://pagure.io/go-rpm-macros/pull-request/34
rishi commented on the pull-request: `Add BUILDTAGS to %gobuildflags` that you are following: `` Since commit [c593262127afd28d](https://pagure.io/go-rpm-macros/c/c593262127afd28d9f0b52a78a3cf0ef5b14673f) or PR [#35](https://pagure.io/go-rpm-macros/pull-request/35), `%gobuild` does use `%gobuildflags`. It's been a while since that change, so I suppose at least `%gobuild` still works. I am struggling to spot any relevant change to `%gobuildflags`, so I guess it's a happy ending. :) ``
To reply, visit the link below or just reply to this email https://pagure.io/go-rpm-macros/pull-request/34
rishi commented on the pull-request: `Add BUILDTAGS to %gobuildflags` that you are following: `` Anyway, I think this pull request should be closed because `BUILDTAGS` is already part of `%gobuildflags` these days. See commit [c593262127afd28d](https://pagure.io/go-rpm-macros/c/c593262127afd28d9f0b52a78a3cf0ef5b14673f) or PR [#35](https://pagure.io/go-rpm-macros/pull-request/35).
The open question seems to be whether `BUILDTAGS` should be renamed to `GOBUILDTAGS`, but that wasn't what this pull request was about. ``
To reply, visit the link below or just reply to this email https://pagure.io/go-rpm-macros/pull-request/34
golang@lists.fedoraproject.org