Bug 1411962 - Review Request: golang-github-milochristiansen-lua - A Lua 5.3 VM and compiler written in Go
Summary: Review Request: golang-github-milochristiansen-lua - A Lua 5.3 VM and compile...
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Fabio Valentini
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2017-01-10 20:50 UTC by Ben Rosser
Modified: 2017-05-17 05:55 UTC (History)
2 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2017-05-15 04:34:54 UTC
Type: ---
decathorpe: fedora-review+


Attachments (Terms of Use)

Description Ben Rosser 2017-01-10 20:50:34 UTC
Spec URL: https://tc01.fedorapeople.org/dwarffortress/golang-github-milochristiansen-lua.spec
SRPM URL: https://tc01.fedorapeople.org/dwarffortress/golang-github-milochristiansen-lua-1.0.2-1.fc25.src.rpm

Description: This is a Lua 5.3 VM and compiler written in Go. This is
intended to allow easy embedding into Go programs, with
minimal fuss and bother.

Fedora Account System Username: tc01

Comment 1 Fabio Valentini 2017-03-15 08:47:10 UTC
Taking this review.

Comment 2 Fabio Valentini 2017-03-15 09:24:06 UTC
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.

Comment 3 Fabio Valentini 2017-03-28 09:23:23 UTC
Pinging submitter.

Comment 4 Ben Rosser 2017-04-04 16:41:41 UTC
> 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.

Comment 5 Ben Rosser 2017-04-05 04:59:29 UTC
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.

Comment 6 Fabio Valentini 2017-04-06 07:19:45 UTC
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!

Comment 7 Fabio Valentini 2017-04-06 07:30:15 UTC
It seems tests (and builds) fail on 32 bit architectures:
https://koji.fedoraproject.org/koji/taskinfo?taskID=18810051

You should inform upstream about that.

Comment 8 Ben Rosser 2017-04-06 20:14:17 UTC
> 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?).

Comment 9 Fabio Valentini 2017-04-07 08:23:41 UTC
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.

Comment 10 Ben Rosser 2017-04-18 17:52:31 UTC
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.

Comment 11 Fabio Valentini 2017-04-18 18:34:25 UTC
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.

Comment 12 Gwyn Ciesla 2017-04-19 16:46:49 UTC
Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/rpms/golang-github-milochristiansen-lua

Comment 13 Ben Rosser 2017-04-19 18:01:43 UTC
Sure, I'll fix that on import. 

Thanks for the reviews (and the golang packaging advice)!

Comment 14 Fedora Update System 2017-04-19 18:24:29 UTC
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

Comment 15 Fedora Update System 2017-04-20 20:22:41 UTC
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

Comment 16 Fedora Update System 2017-05-02 17:15:23 UTC
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

Comment 17 Fedora Update System 2017-05-02 17:16:07 UTC
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

Comment 18 Fedora Update System 2017-05-04 13:29:45 UTC
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.

Comment 19 Fedora Update System 2017-05-04 20:01:16 UTC
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

Comment 20 Fedora Update System 2017-05-04 20:02:21 UTC
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

Comment 21 Fedora Update System 2017-05-15 04:34:54 UTC
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.

Comment 22 Fedora Update System 2017-05-17 05:55:56 UTC
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.


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