Bug 1479027 - (golang-github-golang-image) Review Request: golang-github-golang-image - Go supplementary image libraries
Review Request: golang-github-golang-image - Go supplementary image libraries
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Fabio Valentini
Fedora Extras Quality Assurance
:
: 1480969 (view as bug list)
Depends On:
Blocks: DeepinDEPackageReview 1426972
  Show dependency treegraph
 
Reported: 2017-08-07 14:40 EDT by Athos Ribeiro
Modified: 2017-12-31 20:45 EST (History)
5 users (show)

See Also:
Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2017-08-21 12:40:06 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
decathorpe: fedora‑review+


Attachments (Terms of Use)

  None (edit)
Description Athos Ribeiro 2017-08-07 14:40:03 EDT
Spec URL: https://athoscr.fedorapeople.org/packaging/golang-github-golang-image.spec
SRPM URL: https://athoscr.fedorapeople.org/packaging/golang-github-golang-image-0-0.1.20170514.git426cfd8.fc26.src.rpm
Description: Go supplementary image libraries
Fedora Account System Username: athoscr

Note that the package does not build due to a failing test,

%gotest %{x_import_path}/font/sfnt

 which requires x/text/encoding/charmap EncodeRune method, from commit 65f4f820a7954b82e5c9325e1e088a4fda098f36 on golang.org/x/text.
Comment 1 Fabio Valentini 2017-08-08 05:55:03 EDT
Taking this review.

Since this package doesn't build yet, I suggest we block this review until golang(golang.org/x/text) gets updated to the necessary commit?
Comment 2 Athos Ribeiro 2017-08-08 21:11:47 EDT
Sure! I will send the pull requests for the necessary updates later this week, would do it today, but it seems part of our infra is down

Thanks for taking this review as well!
Comment 3 Athos Ribeiro 2017-08-14 15:29:34 EDT
Just created the PR in golang-googlecode-text [1].

[1] https://src.fedoraproject.org/rpms/golang-googlecode-text/pull-request/1
Comment 4 Jan Chaloupka 2017-08-15 01:54:29 EDT
golang-googlecode-text-0-0.20.git65f4f82.fc27 at https://koji.fedoraproject.org/koji/taskinfo?taskID=21229221
Comment 5 Fabio Valentini 2017-08-15 05:18:56 EDT
The package builds on rawhide now, but the tests fail on 50% of architectures.
see: https://koji.fedoraproject.org/koji/taskinfo?taskID=21239167
Comment 6 Athos Ribeiro 2017-08-15 10:21:45 EDT
We have 3 architectures with failing tests here:
- ppc64
- ppc64le
- s390x

The issue with ppc64 in known, since there's no compiler(go-compiler) for that anymore. Although I am not sure why the %{go_arches} is still including it.

ppc64le and s390x are failing in the same tests with the same unexpected results. It seems to be a precision issue, which needs further investigation.
Comment 8 Fabio Valentini 2017-08-15 15:39:49 EDT
I'll look at it again right now!

By the way, is this (https://bugzilla.redhat.com/show_bug.cgi?id=1480969) a review request for the same go library? If so, you might tell them that your request has been opened earlier and should take precendence.
Comment 9 Athos Ribeiro 2017-08-15 15:45:38 EDT
*** Bug 1480969 has been marked as a duplicate of this bug. ***
Comment 10 Fabio Valentini 2017-08-15 16:02:59 EDT
Initial comments:

1) There are some minor typos / spelling errors which you might want to correct (line 44: provite->provide, line 275: skiping-> skipping).

2) Since you are packaging this go library for two import paths, I guess that both are used by dependencies. In this case, using two -devel subpackages seems reasonable.

3) There are some empty conditionals leftover from the gofed template. You can leave them in with comments that they might be needed one day, or just remove them.

4) "%global devel_prefix x" this isn't needed anywhere.

5) It looks like most/all of the testdata is listed in %files sections multiple times ("warning: File listed twice:" warnings in the koji build logs). You might want to look at the %install script to find out how this happens.

By the way, you don't have to bump the Release tag for me if you fix minor things I pointed out above.


I will provide a more comprehensive review (with fedora-review's help) once the golang-googlecode-text package update will hit the rawhide repo mirrors.
Comment 11 Athos Ribeiro 2017-08-15 17:53:24 EDT
(In reply to Fabio Valentini from comment #10)

> 1) There are some minor typos / spelling errors which you might want to
> correct (line 44: provite->provide, line 275: skiping-> skipping).
> 

Fixed

> 2) Since you are packaging this go library for two import paths, I guess
> that both are used by dependencies. In this case, using two -devel
> subpackages seems reasonable.

OK

> 
> 3) There are some empty conditionals leftover from the gofed template. You
> can leave them in with comments that they might be needed one day, or just
> remove them.
> 

I learned it is better to keep those to reduce the diff if I ever need to re-generate the spec with gofed

> 4) "%global devel_prefix x" this isn't needed anywhere.
> 

Fixed: removed the global.

> 5) It looks like most/all of the testdata is listed in %files sections
> multiple times ("warning: File listed twice:" warnings in the koji build
> logs). You might want to look at the %install script to find out how this
> happens.

Fixed, thanks for catching that!


Spec URL: https://athoscr.fedorapeople.org/packaging/golang-github-golang-image.spec
SRPM URL: https://athoscr.fedorapeople.org/packaging/golang-github-golang-image-0-0.1.20170514.git426cfd8.fc26.src.rpm
Koji Build: https://koji.fedoraproject.org/koji/taskinfo?taskID=21250259
Comment 12 Robert-André Mauchin 2017-08-16 05:14:08 EDT
>The issue with ppc64 in known, since there's no compiler(go-compiler) for that anymore. Although I am not sure why the %{go_arches} is still including it.

You need to install the updated go-srpm-macros from rawhide, then regenerate your SRPM before you do a scratch build.
Comment 13 Athos Ribeiro 2017-08-16 10:35:25 EDT
(In reply to Robert-André Mauchin (afk until next Thu) from comment #12)

> You need to install the updated go-srpm-macros from rawhide, then regenerate
> your SRPM before you do a scratch build.

I did not know those needed to be expanded during srpm build time. Thanks for the explanation
Comment 14 Fabio Valentini 2017-08-16 14:17:44 EDT
Package Review
==============

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


Issues:
=======
- Package does not contain duplicates in %files.
  Note: warning: File listed twice:
  /usr/share/gocode/src/golang.org/x/image/font/testdata/CFFTest.otf
  See: http://fedoraproject.org/wiki/Packaging/Guidelines#DuplicateFiles
- Spec file as given by url is not the same as in SRPM.

No rpmlint messages.

Otherwise:

[x]: Package complies to the Packaging Guidelines


Please fix the last remaining duplicate in %files, and make sure to submit/link the right version of the package / .spec (with all the fixes from the first and! second round of improvements).

One question remains: Are the testdata (pictures? fonts?) distributed under an acceptable license? Some at least seem to be in the Public Domain. You should include a "License: SOMETHING" tag in the -unit-test-devel subpackage if the test data is under a different license than the code.
Comment 15 Athos Ribeiro 2017-08-17 16:49:43 EDT
Oh, sorry for that! I messed up the links.

Spec URL: https://athoscr.fedorapeople.org/packaging/golang-github-golang-image.spec
SRPM URL: https://athoscr.fedorapeople.org/packaging/golang-github-golang-image-0-0.2.20170514.git426cfd8.fc26.src.rpm
Koji Build: https://koji.fedoraproject.org/koji/taskinfo?taskID=21286115


About the license for the testdata files, it seems that they are covered by the same license of the package, as mentioned in [1]. We can always wait for the pending reply in [1] to make sure.

[1] https://groups.google.com/forum/#!topic/golang-nuts/ayvhNCVqE0Q
Comment 16 Fabio Valentini 2017-08-17 17:14:07 EDT
From my point of view, the package can be approved, as all packaging issues have been resolved.

Still, it would probably be wise to wait with importing the package until you get confirmation for the licensing of the testdata fonts and pictures.
Comment 17 Athos Ribeiro 2017-08-17 18:04:40 EDT
Thank you for the review, Fabio!

I will request the repository and wait for the confirmation on the licensing issie before importing the package. I will wait until then to document so in this bug and only then, close it.
Comment 18 Gwyn Ciesla 2017-08-17 18:09:59 EDT
(fedrepo-req-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/golang-github-golang-image
Comment 19 Robert-André Mauchin 2017-08-21 04:03:10 EDT
Hey Athos, could you build this package in Koji now that it is accepted, it is needed for building another package.
Comment 20 Athos Ribeiro 2017-08-21 12:07:12 EDT
I was waiting for a reply on the golang go-nuts mailing list [1] on the license of some files in the source package to proceed with the build. Fortunately, the reply came yesterday.

All the fonts and images in the package are licensed under either BSD, Creative Commons or are under the public domain.

Since we are not talking about source code, but content, I will not change the License tag on the package.

It seems that we are clear to go and I will fire the build as soon as I can.

[1] https://groups.google.com/forum/#!topic/golang-nuts/ayvhNCVqE0Q

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