Bug 1411962
Summary: | Review Request: golang-github-milochristiansen-lua - A Lua 5.3 VM and compiler written in Go | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Ben Rosser <rosser.bjr> |
Component: | Package Review | Assignee: | Fabio Valentini <decathorpe> |
Status: | CLOSED ERRATA | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | decathorpe, package-review |
Target Milestone: | --- | Keywords: | Reopened |
Target Release: | --- | Flags: | decathorpe:
fedora-review+
|
Hardware: | All | ||
OS: | Linux | ||
Whiteboard: | |||
Fixed In Version: | Doc Type: | If docs needed, set a value | |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2017-05-15 04:34:54 UTC | Type: | --- |
Regression: | --- | Mount Type: | --- |
Documentation: | --- | CRM: | |
Verified Versions: | Category: | --- | |
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
Cloudforms Team: | --- | Target Upstream Version: | |
Embargoed: |
Description
Ben Rosser
2017-01-10 20:50:34 UTC
Taking this review. Initial comments: 1) Please update your .spec and .src.rpm for the newest upstream version (1.1.0). 2) Please drop the leading "A" from the Summary line. 3) Please change the target file name for your sources, for example you can use: "Source0: https://%{provider_prefix}/archive/v%{version}/%{project}-%{repo}-%{version}.tar.gz" to avoid name collisions with "official" lua package sources (otherwise your sources will be "lua-1.1.0.tar.gz", which looks like official lua sources ...). 4) Are you sure it's a good idea to provide the "vendored" go library? If not, just remove the "Provides: golang(%{import_path}/vendor/sliceutil) = %{version}-%{release}". 5) You can remove the empty %if-endif block at line 77. 6) You can use "%setup -q -n %{repo}-%{version}" in %prep. 7) Please correct the GOPATH definition on line 136, the Godeps directory isn't in the sources, so this seems to be there by accident. 8) Please drop the %{dist} tag from the changelog entry, it doesn't belong there. Pinging submitter. > 4) Are you sure it's a good idea to provide the "vendored" go library? If not, just remove the "Provides: golang(%{import_path}/vendor/sliceutil) = %{version}-%{release}".
I wasn't sure if I should do so or not; but I can remove this from the spec when I update it with all the other comments.
I'll post an updated version of the spec later today, sorry for the delay.
Updated to upstream release 1.1.1* and fixed the following: - Updated to latest upstream release. - Changed Source0 to use project prefix. - Remove vendored go library from Provides section. - Cleaned up setup macro in prep section. - Removed some empty/redundant template parts of the spec file and cleaned GOPATH definition. - General spec cleanup; removed dist tag from changelog section and removed leading article from summary. Spec URL: https://tc01.fedorapeople.org/dwarffortress/golang-github-milochristiansen-lua.spec SRPM URL: https://tc01.fedorapeople.org/dwarffortress/golang-github-milochristiansen-lua-1.1.1-1.fc25.src.rpm *There is a more recent commit referring to 1.1.2, but that release hasn't been tagged. 1) The new release provides a few new go subpackages: > Provides: golang(%{import_path}/supermeta) = %{version}-%{release} > Provides: golang(%{import_path}/testhelp) = %{version}-%{release} Please add them to the -devel subpackage's Provides: section. 2) Additionally, add > %gotest %{import_path}/supermeta after the > %gotest %{import_path} line, since that subpackage also has tests. 3) Please add the > %global commit cbee393f691f87139e9f21525610767e85ae0e33 > %global shortcommit %(c=%{commit}; echo ${c:0:7}) and maybe > # commit cbee393f691f87139e9f21525610767e85ae0e33 == version 1.1.1 to the .spec header (after the import_path definition, where you removed it from in the first place). This information is used by gofed to track versions of golang packages across all of fedora. Otherwise, the package looks good now! PS: I usually re-run gofed when I bump my golang packages to a new version to make sure I catch issues like [1] and [2]. Maybe that helps you in the future! It seems tests (and builds) fail on 32 bit architectures: https://koji.fedoraproject.org/koji/taskinfo?taskID=18810051 You should inform upstream about that. > PS: I usually re-run gofed when I bump my golang packages to a new version to make sure I catch issues like [1] and [2]. Maybe that helps you in the future! Thanks! This is a good suggestion; I'll be sure to bear it in mind. > It seems tests (and builds) fail on 32 bit architectures: https://koji.fedoraproject.org/koji/taskinfo?taskID=18810051 > You should inform upstream about that. I have done so here: https://github.com/milochristiansen/lua/issues/7. In the mean time, should I simply modify the ExclusiveArch tag to not build on the 32-bit architectures? (What would be a good way to do that, since it uses the go_arches macro?). Let's see how upstream responds. I had similar problems with some of my go packages, most of the time the issue was an integer overflow (caused by test cases, not by code) on 32bit arches that developers didn't think of. Indeed; it turned out to be an integer conversion issue. The bug has now been fixed in 1.1.3: Spec URL: https://tc01.fedorapeople.org/dwarffortress/golang-github-milochristiansen-lua.spec SRPM URL: https://tc01.fedorapeople.org/dwarffortress/golang-github-milochristiansen-lua-1.1.3-1.fc25.src.rpm I've also added the new provides, the supermeta tests, and the commit/shortcommit macros to the header. It's nice that is was possible to resolve the bug upstream! The tests pass now: https://koji.fedoraproject.org/koji/taskinfo?taskID=19065240 My final review uncovered one last minor issue: <snip> Note: No known owner of /usr/share/gocode/src/github.com/milochristiansen/lua/vendor <snip> You should add this as a %dir to the -devel subpackage before importing the package (no additional changelog entry is necessary from my point of view). For example, add something like the following at line 104 (similar to line 103): echo "%%dir %%{gopath}/src/%%{import_path}/vendor" >> devel.file-list But since it's only a very minor issue, the package is approved. Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/rpms/golang-github-milochristiansen-lua Sure, I'll fix that on import. Thanks for the reviews (and the golang packaging advice)! golang-github-milochristiansen-lua-1.1.3-1.fc26 has been submitted as an update to Fedora 26. https://bodhi.fedoraproject.org/updates/FEDORA-2017-e960574979 golang-github-milochristiansen-lua-1.1.3-1.fc26 has been pushed to the Fedora 26 testing repository. If problems still persist, please make note of it in this bug report. See https://fedoraproject.org/wiki/QA:Updates_Testing for instructions on how to install test updates. You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2017-e960574979 golang-github-milochristiansen-lua-1.1.3-1.fc25 has been submitted as an update to Fedora 25. https://bodhi.fedoraproject.org/updates/FEDORA-2017-6dd45a7b01 golang-github-milochristiansen-lua-1.1.3-1.fc24 has been submitted as an update to Fedora 24. https://bodhi.fedoraproject.org/updates/FEDORA-2017-17e1e108f9 golang-github-milochristiansen-lua-1.1.3-1.fc26 has been pushed to the Fedora 26 stable repository. If problems still persist, please make note of it in this bug report. golang-github-milochristiansen-lua-1.1.3-1.fc24 has been pushed to the Fedora 24 testing repository. If problems still persist, please make note of it in this bug report. See https://fedoraproject.org/wiki/QA:Updates_Testing for instructions on how to install test updates. You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2017-17e1e108f9 golang-github-milochristiansen-lua-1.1.3-1.fc25 has been pushed to the Fedora 25 testing repository. If problems still persist, please make note of it in this bug report. See https://fedoraproject.org/wiki/QA:Updates_Testing for instructions on how to install test updates. You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2017-6dd45a7b01 golang-github-milochristiansen-lua-1.1.3-1.fc25 has been pushed to the Fedora 25 stable repository. If problems still persist, please make note of it in this bug report. golang-github-milochristiansen-lua-1.1.3-1.fc24 has been pushed to the Fedora 24 stable repository. If problems still persist, please make note of it in this bug report. |