Bug 1258182 - Review Request: go-compilers - Go language compilers for various architectures
Summary: Review Request: go-compilers - Go language compilers for various architectures
Keywords:
Status: CLOSED CURRENTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Jakub Čajka
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On: 1258166
Blocks:
TreeView+ depends on / blocked
 
Reported: 2015-08-30 00:04 UTC by Jan Chaloupka
Modified: 2019-02-28 12:14 UTC (History)
7 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2019-02-28 12:14:37 UTC
Type: ---
jcajka: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Jan Chaloupka 2015-08-30 00:04:53 UTC
Spec URL: https://jchaloup.fedorapeople.org/reviews/go-compilers/go-compilers.spec

SRPM URL: https://jchaloup.fedorapeople.org/reviews/go-compilers/go-compilers-1-1.fc20.src.rpm

Description: Go language compilers for various architectures

Fedora Account System Username: jchaloup

koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=10883004
ppc-koji: http://ppc.koji.fedoraproject.org/koji/taskinfo?taskID=2726462
s390-koji: http://s390.koji.fedoraproject.org/koji/taskinfo?taskID=1939911

Notes:
- on architectures specified by golang_arches macro go-compilers-golang-compiler providing compiler(golang) and compiler(go-compiler) virtual macros and requires golang and go-srpm-macros packages
- on architectures specified by gccgo_arches macro go-compilers-gcc-go-compiler providing compiler(gcc-go) and compiler(go-compiler) virtual macros and requires gcc-go and go-srpm-macros packages
- all golang packages will BuildRequire compiler(go-compiler) virtual provides which based on an architectures installs golang or gcc-go (transparent to spec file)

This package is primarily targeted for f22 and higher. Epel6 can be included as well (not for secondary architectures).

Resulting spec file will contain the following two improvements:
1) Choice of the correct architecture (applied for main and unit-test):
ExclusiveArch:  %{go_arches}
BuildRequires:  compiler(go-compiler)

2) shorter %build and %check section (go-compilers packages provides shell library with gobuild and gotest functions definition):
source %{go_compilers_lib}
gobuild -o bin/go-md2man %{import_path}

Packages go-compilers and go-srpm-macros are meant to work together. go-srpm-macros provides only macros which are independent of a golang language compiler. go-compilers just takes some macros and glues golang/gcc-go compiler with them to provide unifying virtual provide for all architectures. At the same time it defines gobuild and gotest functions which can be used again transparent to a given architecture. Thus making spec file unware of used compiler.

At the moment go-srpm-macros is in a minimal buildroot (installed as a runtime dependency of redhat-rpm-config). Once it is removed from the minimal buildroot, it will be installed as a dependency of go-compilers subpackage. Thus it should be available when srpm is generated (via BuildRequires: compiler(go-compiler) in a main package). This should be tested (via mock or in copr at least).

Comment 1 Jan Chaloupka 2015-08-31 12:40:56 UTC
After discussion with Jakub, compile.sh removed, definition of gobuild and gotest moved to macros. macros.go-compilers now contains:

%gobuild() \
function gobuild { \
local LDFLAGS="${LDFLAGS:-} -B 0x$(head -c20 /dev/urandom|od -An -tx1|tr -d ' \\n')" \
go build -compiler gc    -ldflags "${LDFLAGS:-}" -a -v -x "$@"; \
} \
gobuild %{*}

%gotest() \
function gotest { go test -compiler gc    -ldflags "${LDFLAGS:-}" "$@"; } \
gotest %{*}

Use:
%{gobuild} -o bin/go-md2man %{import_path}
%{gotest} %{import_path}

%{gobuild} can be used multiple times as gobuild function is only redefined. The same holds for %{gotest}. At the moment, the package ships only macros.go-compilers file.

Spec URL: https://jchaloup.fedorapeople.org/reviews/go-compilers/go-compilers.spec

SRPM URL: https://jchaloup.fedorapeople.org/reviews/go-compilers/go-compilers-1-1.fc20.src.rpm

koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=10894598
ppc-koji: http://ppc.koji.fedoraproject.org/koji/taskinfo?taskID=2726756
s390-koji: http://s390.koji.fedoraproject.org/koji/taskinfo?taskID=1939913

Comment 2 Jan Chaloupka 2015-09-01 14:23:14 UTC
Updated definition of gobuild and gotest
- added 'undef -f gobuild' and 'undef -f gotest' so both functions are not redefined if %gobuild/%gotest macros are used multi times.

Besides, the correct use of both macros is without braces:
%gobuild -o bin/go-md2man %{import_path}.

With braces macro's parameters are skipped, so they have to be put inside:
%{gobuild -o bin/go-md2man %{import_path}}

The latter option looks ugly.

Comment 3 Jan Chaloupka 2015-09-01 14:23:49 UTC
Correction: it is 'unset -f', not 'undef -f'

Comment 4 Jan Chaloupka 2015-09-01 14:31:35 UTC
Plus gobuild and gotest macros takes only -o option. For other options rpm reports an error but the building phase continue without any problems. At this point only -o option is relevant as it is common for both golang and gcc-go compiler. Did not make a survey on which options are common. However, I don't want to rely on common options. Let's make the macro as simple and general as possible. For specific compiler options user can create his own %ifelse branches to cover his/her usecase.

Comment 5 Jan Chaloupka 2015-09-03 12:11:57 UTC
As it stands, go-srpm-macros can not be removed from minimal buildroot if %{go_arches} macro is about to be used with ExclusiveArch. Reasoning:

Building of rpm has three phases:
1) generate srpm on a random machine as noarch
2) choose architectures and regenerate noarch srpm as arch specific srpm
3) from arch specific srpm build rpm

At phase 1 and 2, 'rpmbuild -bs --target noarch/arch --nodeps ...' is used with --nodeps options. So when srpm is generated no package tagged as BuildRequires is installed. Srpm is generated in minimal buildroot. So the only way how to get go-srpm-macros into the buildroot where srpm is generated is via redhat-rpm-config and have it installed in the minimal buildroot.

I will update go-compilers and suggest new solution. After discussion with vondruch, one of possible solutions is to remove ExclusiveArch tag a thus no need for go_arches macro. At the same time assuming all architectures has a golang language compiler. Will checkout what is an impact of this decision.

Comment 6 Vít Ondruch 2015-09-04 10:01:21 UTC
Looking into the .spec file, wouldn't it be better to reverse the virtual provides, e.g.

compiler(go-compiler) -> go(compiler)
compiler(gcc-go) -> go(gcc-go)
compiler(golang) -> go(golang)

Or possibly go(comp9ler_gcc-go), go(compiler_golang)?

As far as I understand these are Go specific macros, so they should be in go() format similarly to every other language specific macros.

Comment 7 Vít Ondruch 2015-09-04 10:05:43 UTC
And why are you creating platform specific subpackages? They are never created both on single platform if I understand it correctly, so what is the point then?

Comment 8 Vít Ondruch 2015-09-04 10:07:49 UTC
I would also suggest to include the "macros.go-compilers" files as a SOURCE instead creating them on the fly. One could easier compare them with current state for example and you can save some escaping ...

Comment 9 Vít Ondruch 2015-09-04 10:11:14 UTC
(In reply to Vít Ondruch from comment #8)
The downside is that you have to replace/expand the %{golang_build}, %{gcc_go_build}, %{golang_test} and %{gcc_go_test} macros in the template, but that is still probably better

Comment 10 Jan Chaloupka 2015-09-04 11:33:29 UTC
> And why are you creating platform specific subpackages? They are never created
> both on single platform if I understand it correctly, so what is the point
> then?

So I can provide compiler(gcc-go) and compiler(golang) among others. Which could be on the other hand ifarched in one package. Good point. It is more like to have package named gcc-go for gcc-go compiler and golang for golang compiler. To have two semantically distinct packages.

Note: ifarch of macros.* files, compiler(gcc-go/golang), Requires and summary will work but it does not look so good as two distinct packages.

> I would also suggest to include the "macros.go-compilers" files as a SOURCE
> instead creating them on the fly. One could easier compare them with current
> state for example and you can save some escaping ...

Point taken. Created macros.golang-compiler and macros.gcc-go-compiler. At the same time I have moved some macros from go-srpm-macros to macros.*-compiler and reduced unnecessary lines in macros definition.

> The downside is that you have to replace/expand the %{golang_build},
> %{gcc_go_build}, %{golang_test} and %{gcc_go_test} macros in the template, but
> that is still probably better

Not needed anymore with the change above.

Comment 11 Jan Chaloupka 2015-09-04 11:42:04 UTC
> Looking into the .spec file, wouldn't it be better to reverse the virtual
> provides, e.g.
>
> compiler(go-compiler) -> go(compiler)
> compiler(gcc-go) -> go(gcc-go)
> compiler(golang) -> go(golang)
> 
> Or possibly go(comp9ler_gcc-go), go(compiler_golang)?
> 
> As far as I understand these are Go specific macros, so they should be in go()
> format similarly to every other language specific macros.>

This is another way of looking at it. I don't mine any of them. What about to provide both ways?

compiler(...) is general and can be provided by any package with a compiler. To get a list of all compiler provided by distribution, you can run 'dnf/yum provides "compiler(*)"' or similar command.

On the other hand go(...) is specific and meant only for golang. Which is more suitable for spec file. However, it is harder to search for it.

I have not found any mention of compiler(...) or similar that would be used in general. At least not for gcc not java.

Comment 12 Vít Ondruch 2015-09-04 12:44:56 UTC
(In reply to Jan Chaloupka from comment #11)
> compiler(...) is general and can be provided by any package with a compiler.
> To get a list of all compiler provided by distribution, you can run 'dnf/yum
> provides "compiler(*)"' or similar command.

While I agree with this, I don't see any immediate benefit. If that was useful, it should be probably discussed with wider audience. This review is not probably the best place to decide.

Comment 13 Jan Chaloupka 2015-09-04 13:12:39 UTC
What would the use of go-compilers and go-srpm-macros macros?

1)
%if 0%{?go_arches:1}
ExclusiveArch:  %{go_arches}
%else
ExclusiveArch:  %{ix86} x86_64 %{arm}
%endif

If not being optimistic, this will work without go-srpm-macros package as well. So no need for it.

2)
Unit-test subpackage and main package (if necessary) will use:
%if 0%{?fedora}
BuildRequires: compiler(golang)
%endif
...

%if ! 0%{?gobuild:1}
%global gobuild(o) go build %{?**};
%endif

And use would be:
%gobuild -o bin/go-md2man %{import_path}

So again if there is no go-srpm-macros package => no macros => gobuild get defined locally. Of course this functionality is valid only for non-Fedora distribution. As from f22 and higher, golang package will no longer provide its macros (so no gopath macro as well). However, it has to be made sure the macro is present on non-Fedora distros.

This is not the final use case. However, it does not depend on go-compilers package so it can be discussed independently. Just mentioning it.

Comment 14 Jan Chaloupka 2015-09-04 13:15:43 UTC
At the same time there will be no support for debug info with the current definition of gobuild. Of course, the definition can be updated and generate debuginfo no matter what (as defined and mentions in macros.golang-compiler). 

Once go-srpm-macros and go-compilers get into other distribution, the spec file can be further extended/updated. Which is not the case at the moment.

Comment 15 Jan Chaloupka 2015-09-06 14:27:35 UTC
Proposed form and use of ExclusiveArchs and BuildRequires for various architectures and distributions with or without go-srpm-macros packaged.

https://github.com/ingvagabund/gofed/issues/29

Comment 16 Jan Chaloupka 2015-09-06 15:30:19 UTC
Again, minimized number of macros in macros.golang/gcc-go-compilers and redefined gobuild/gotest:

macros.golang-compiler:
# Define commands for building
# BUILD_ID can be generated for golang build no matter of debuginfo
%gobuild(o:) go build -compiler gc -ldflags "${LDFLAGS:-} -B 0x$(head -c20 /dev/urandom|od -An -tx1|tr -d ' \\n')" -a -v -x %{?**};

# Define commands for testing
%gotest() go test -compiler gc -ldflags "${LDFLAGS:-}" %{?**};

macros.gcc-go-compiler is analogous.

Comment 17 Jan Chaloupka 2015-09-08 07:49:48 UTC
Added new macro go_compiler which if set to 1 means that virtual provide compiler(go-compiler) is available. So distributions where there is no such provide can BuildRequire implicit golang compiler.

Spec file is ready for review.

Spec URL: https://jchaloup.fedorapeople.org/reviews/go-compilers/go-compilers.spec

SRPM URL: https://jchaloup.fedorapeople.org/reviews/go-compilers/go-compilers-1-1.fc20.src.rpm

Comment 18 Jakub Čajka 2015-09-08 14:15:08 UTC
Package spec file seems fine and packages seems to work as intended, only issue is reported by fedora-review (coreutils BR) and it seems minor and easy to fix before/during import.

------

Package Review
==============

Legend:
[x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated


Issues:
=======
- All build dependencies are listed in BuildRequires, except for any that
  are listed in the exceptions section of Packaging Guidelines.
  Note: These BR are not needed: coreutils
  See: http://fedoraproject.org/wiki/Packaging/Guidelines#Exceptions_2


===== MUST items =====

Generic:
[x]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[-]: If (and only if) the source package includes the text of the
     license(s) in its own file, then that file, containing the text of the
     license(s) for the package is included in %license.
[x]: License field in the package spec file matches the actual license.
[x]: Package must own all directories that it creates.
     Note: Directories without known owners: /usr/lib/rpm/macros.d,
     /usr/lib/rpm
[x]: %build honors applicable compiler flags or justifies otherwise.
[x]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[-]: Package contains desktop file if it is a GUI application.
[-]: Development files must be in a -devel package
[x]: Package uses nothing in %doc for runtime.
[x]: Package consistently uses macros (instead of hard-coded directory
     names).
[x]: Package is named according to the Package Naming Guidelines.
[x]: Package does not generate any conflict.
[x]: Package obeys FHS, except libexecdir and /usr/target.
[-]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[x]: Requires correct, justified where necessary.
[x]: Spec file is legible and written in American English.
[-]: Package contains systemd file(s) if in need.
[-]: Useful -debuginfo package or justification otherwise.
[x]: Package is not known to require an ExcludeArch tag.
[x]: Package complies to the Packaging Guidelines
[x]: Package successfully compiles and builds into binary rpms on at least
     one supported primary architecture.
[x]: Package installs properly.
[x]: Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).
[x]: Package requires other packages for directories it uses.
[x]: Package does not own files or directories owned by other packages.
[x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[x]: Macros in Summary, %description expandable at SRPM build time.
[x]: Dist tag is present.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Package use %makeinstall only when make install DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[x]: Package does not use a name that already exists.
[x]: Package is not relocatable.
[x]: Sources used to build the package match the upstream source, as
     provided in the spec URL.
[x]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: File names are valid UTF-8.
[x]: Large documentation must go in a -doc subpackage. Large could be size
     (~1MB) or number of files.
     Note: Documentation size is 0 bytes in 0 files.
[x]: Packages must not store files under /srv, /opt or /usr/local

===== SHOULD items =====

Generic:
[-]: If the source package does not include license text(s) as a separate
     file from upstream, the packager SHOULD query upstream to include it.
[x]: Final provides and requires are sane (see attachments).
[x]: Package functions as described.
[-]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[-]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[-]: %check is present and all tests pass.
[x]: Packages should try to preserve timestamps of original installed
     files.
[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: SourceX is a working URL.
[x]: Package should compile and build into binary rpms on all supported
     architectures.
[x]: Spec use %global instead of %define unless justified.

===== EXTRA items =====

Generic:
[x]: Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).
[x]: Large data in /usr/share should live in a noarch subpackage if package
     is arched.
[x]: Spec file according to URL is the same as in SRPM.


Rpmlint
-------
Checking: go-compilers-golang-compiler-1-1.fc22.x86_64.rpm
          go-compilers-1-1.fc22.src.rpm
go-compilers-golang-compiler.x86_64: W: summary-not-capitalized C compiler for golang
go-compilers-golang-compiler.x86_64: W: no-url-tag
go-compilers-golang-compiler.x86_64: W: only-non-binary-in-usr-lib
go-compilers-golang-compiler.x86_64: W: no-documentation
go-compilers.src: W: spelling-error %description -l en_US golang -> Angolan, Golan, Angola
go-compilers.src: W: no-url-tag
go-compilers.src:27: W: unversioned-explicit-provides compiler(go-compiler)
go-compilers.src:28: W: unversioned-explicit-provides compiler(golang)
go-compilers.src:40: W: unversioned-explicit-provides compiler(go-compiler)
go-compilers.src:41: W: unversioned-explicit-provides compiler(gcc-go)
2 packages and 0 specfiles checked; 0 errors, 10 warnings.




Rpmlint (installed packages)
----------------------------
go-compilers-golang-compiler.x86_64: W: summary-not-capitalized C compiler for golang
go-compilers-golang-compiler.x86_64: W: no-url-tag
go-compilers-golang-compiler.x86_64: W: only-non-binary-in-usr-lib
go-compilers-golang-compiler.x86_64: W: no-documentation
1 packages and 0 specfiles checked; 0 errors, 4 warnings.



Requires
--------
go-compilers-golang-compiler (rpmlib, GLIBC filtered):
    golang



Provides
--------
go-compilers-golang-compiler:
    compiler(go-compiler)
    compiler(golang)
    go-compilers-golang-compiler
    go-compilers-golang-compiler(x86-64)

Comment 19 Jan Chaloupka 2015-09-08 14:19:42 UTC
Thanks Jakub.

These could be fixed:
go-compilers.src:27: W: unversioned-explicit-provides compiler(go-compiler)
go-compilers.src:28: W: unversioned-explicit-provides compiler(golang)
go-compilers.src:40: W: unversioned-explicit-provides compiler(go-compiler)
go-compilers.src:41: W: unversioned-explicit-provides compiler(gcc-go)

Will update the spec file in the repository.

Comment 20 Jan Chaloupka 2015-09-08 14:22:59 UTC
New Package SCM Request
=======================
Package Name: go-compilers
Short Description: Go language compilers for various architectures
Upstream URL: 
Owners: jchaloup jcajka fpokorny
Branches: f23 f22 f21 el6
InitialCC: golang-sig

Comment 21 Gwyn Ciesla 2015-09-09 12:26:08 UTC
Git done (by process-git-requests).

Comment 22 Fedora Update System 2015-09-09 14:21:30 UTC
go-compilers-1-1.fc22 has been submitted as an update to Fedora 22. https://bodhi.fedoraproject.org/updates/FEDORA-2015-15464

Comment 23 Fedora Update System 2015-09-09 14:36:55 UTC
go-compilers-1-1.fc23 has been submitted as an update to Fedora 23. https://bodhi.fedoraproject.org/updates/FEDORA-2015-15466

Comment 24 Fedora Update System 2015-09-10 02:07:26 UTC
go-compilers-1-1.fc22 has been pushed to the Fedora 22 testing repository. If problems still persist, please make note of it in this bug report.\nIf you want to test the update, you can install it with \n su -c 'yum --enablerepo=updates-testing update go-compilers'. You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2015-15464

Comment 25 Fedora Update System 2015-09-10 05:51:22 UTC
go-compilers-1-1.fc23 has been pushed to the Fedora 23 testing repository. If problems still persist, please make note of it in this bug report.\nIf you want to test the update, you can install it with \n su -c 'yum --enablerepo=updates-testing update go-compilers'. You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2015-15466

Comment 26 Fedora Update System 2015-09-10 13:15:11 UTC
go-compilers-1-2.fc23 has been submitted as an update to Fedora 23. https://bodhi.fedoraproject.org/updates/FEDORA-2015-15532

Comment 27 Fedora Update System 2015-09-10 13:17:08 UTC
go-compilers-1-2.fc22 has been submitted as an update to Fedora 22. https://bodhi.fedoraproject.org/updates/FEDORA-2015-15533

Comment 28 Fedora Update System 2015-09-11 03:49:38 UTC
go-compilers-1-2.fc23 has been pushed to the Fedora 23 testing repository. If problems still persist, please make note of it in this bug report.\nIf you want to test the update, you can install it with \n su -c 'yum --enablerepo=updates-testing update go-compilers'. You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2015-15532

Comment 29 Fedora Update System 2015-09-11 19:54:19 UTC
go-compilers-1-2.fc22 has been pushed to the Fedora 22 testing repository. If problems still persist, please make note of it in this bug report.\nIf you want to test the update, you can install it with \n su -c 'yum --enablerepo=updates-testing update go-compilers'. You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2015-15533

Comment 30 Fedora Update System 2015-09-25 07:58:34 UTC
go-compilers-1-2.fc23 has been pushed to the Fedora 23 stable repository. If problems still persist, please make note of it in this bug report.

Comment 31 Fedora Update System 2015-10-04 22:51:57 UTC
go-compilers-1-2.fc22 has been pushed to the Fedora 22 stable repository. If problems still persist, please make note of it in this bug report.

Comment 32 Fedora Update System 2015-10-20 08:00:21 UTC
go-compilers-1-2.fc21 has been submitted as an update to Fedora 21. https://bodhi.fedoraproject.org/updates/FEDORA-2015-b9da8834ba

Comment 33 Fedora Update System 2015-10-26 10:29:16 UTC
go-compilers-1-2.fc21 has been pushed to the Fedora 21 testing repository. If problems still persist, please make note of it in this bug report.
If you want to test the update, you can install it with
$ su -c 'dnf --enablerepo=updates-testing update go-compilers'
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2015-b9da8834ba

Comment 34 Upstream Release Monitoring 2016-02-08 13:43:47 UTC
jchaloup's scratch build of go-memdb-9ea975b.tar.gz for cli-build/1454938967.533693.wYGVMBzT/go-memdb-9ea975b.tar.gz and f22-candidate failed http://koji.fedoraproject.org/koji/taskinfo?taskID=12902531

Comment 35 Kamil Páral 2019-02-28 12:14:37 UTC
The package is in Fedora, I believe this should be closed.


Note You need to log in before you can comment on or make changes to this bug.