Bug 1479027

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?

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!

Just created the PR in golang-googlecode-text [1].

[1] https://src.fedoraproject.org/rpms/golang-googlecode-text/pull-request/1

golang-googlecode-text-0-0.20.git65f4f82.fc27 at https://koji.fedoraproject.org/koji/taskinfo?taskID=21229221

The package builds on rawhide now, but the tests fail on 50% of architectures.
see: https://koji.fedoraproject.org/koji/taskinfo?taskID=21239167

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.

I opened an issue upstream [1] and am skipping the failing tests for now.

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=21248420

[1] https://github.com/golang/go/issues/21460

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.

*** Bug 1480969 has been marked as a duplicate of this bug. ***

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.

(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

>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.

(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

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.

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

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.

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.

(fedrepo-req-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/golang-github-golang-image

Hey Athos, could you build this package in Koji now that it is accepted, it is needed for building another package.

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

*** Bug 1246593 has been marked as a duplicate of this bug. ***