Bug 1479027 - Review Request: golang-github-golang-image - Go supplementary image libraries
Review Request: golang-github-golang-image - Go supplementary image libraries
Status: NEW
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: 1426972 DeepinDEPackageReview
  Show dependency treegraph
 
Reported: 2017-08-07 14:40 EDT by Athos Ribeiro
Modified: 2017-08-16 10:35 EDT (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:
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 (afk until next Thu) 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

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