Bug 1428974

Thanks for getting involved in Fedora.

I will review this and hopefully eventually sponsor you, thus lifting FE-NEEDSPONSOR.

Except some feedback soon.

s/Except/Expect/

The License tag says: DWPL and ISC and MIT

From reading the LICENSE [0], I'd say it shall be: DWPL and ISC and MIT and BSD

Adding an explanation comment (Such as "See LICENSE for details") would be nice.

I'd suggest you remove the commented out "#Requires:       ".

Also, the Q=(nothing) in %build is confusing, maybe change it to Q=""?

Why is the debuginfo package disabled?

[0]: https://github.com/rednex/rgbds/blob/v0.2.4/LICENSE

Also, the description could be more verbose. What does it do?

I'm curious whether this belongs to:

https://fedoraproject.org/wiki/Packaging:Guidelines#Packages_which_are_not_useful_without_external_bits

"Some software is not functional or useful without the presence of external code dependencies in the runtime operating system environment. When those external code dependencies are non-free, legally unacceptable, or binary-only (with the exception of permissible firmware), then the dependent software is not acceptable for inclusion in Fedora."


Also, is this an emulator?

The summary and description includes "Game Boy". Be specially careful with that, see https://fedoraproject.org/wiki/Packaging:Guidelines#Trademarks_in_Summary_or_Description

The description says "for the Game Boy and Game Boy Color" and that s fine.
However the summary simply says "A Game Boy development package, including an assembler" and that may not be OK. What about "A development package for Game Boy, including an assembler"?

As I understand it, this allows you to create games for the Game Boy? That should be fine. Does it contain any binary blobs or such that might be problematic?


I'd suggest to add the following section fo the README to the description as well:

It consists of:

* rgbasm (assembler)
* rgblink (linker)
* rgbfix (checksum/header fixer)
* rgbgfx (PNG‐to‐Game Boy graphics converter)

>The License tag says: DWPL and ISC and MIT
>From reading the LICENSE [0], I'd say it shall be: DWPL and ISC and MIT and BSD
I will correct this, good catch.

>Adding an explanation comment (Such as "See LICENSE for details") would be nice.
Where would this be?  I'm under the impression the License tag should be machine-readable.

>I'd suggest you remove the commented out "#Requires:       ".
OK

>Also, the Q=(nothing) in %build is confusing, maybe change it to Q=""?
OK

>Why is the debuginfo package disabled?
I am getting the following error when I enable debuginfo:

   error: Empty %files file /builddir/build/BUILD/rgbds-0.2.4/debugfiles.list
       Empty %files file /builddir/build/BUILD/rgbds-0.2.4/debugfiles.list

I'm assuming the build process isn't producing sufficient debug information.  I'll be looking into it further.

>I'm curious whether this belongs to:
>https://fedoraproject.org/wiki/Packaging:Guidelines#Packages_which_are_not_useful_without_external_bits
>Also, is this an emulator?
It is not an emulator.  This software is an assembler package - it transforms source code into binaries.  It can be used to build free software for the game console, and indeed such software can be found on the internet.  Therefore, it does not have non-free dependencies.

>The description says "for the Game Boy and Game Boy Color" and that s fine.
>However the summary simply says "A Game Boy development package, including an assembler" and that may not be OK. What about "A development package for Game Boy, including an assembler"?
Agreed, I shall change it.

>As I understand it, this allows you to create games for the Game Boy? That should be fine. Does it contain any binary blobs or such that might be problematic?
Games and utilities, correct.  I am aware the rgbfix utility contains a small bitmap of the Nintendo logo[0].

This bitmap must be present in the ROM in order for the console to boot into the game.  It is my understanding the presence of the logo, when it's required to pass hardware restrictions, is not a trademark violation, rather, it deemed fair use in the US (see [1]), but IANAL.

>I'd suggest to add the following section fo the README to the description as well:
I'll add it.  Note that rgbgfx is not provided by the version I am packaging (0.2.4).

[0] https://github.com/rednex/rgbds/blob/v0.2.4/src/fix/main.c#L212
[1] https://en.wikipedia.org/wiki/Sega_v._Accolade

(In reply to David "Sanqui" Labský from comment #7)
> >Adding an explanation comment (Such as "See LICENSE for details") would be nice.
> Where would this be?  I'm under the impression the License tag should be
> machine-readable.
I meant a # comment above the License tag.


> >Why is the debuginfo package disabled?
> I am getting the following error when I enable debuginfo:
> 
>    error: Empty %files file /builddir/build/BUILD/rgbds-0.2.4/debugfiles.list
>        Empty %files file /builddir/build/BUILD/rgbds-0.2.4/debugfiles.list
> 
> I'm assuming the build process isn't producing sufficient debug information.
> I'll be looking into it further.

Try setting the compiler flags https://fedoraproject.org/wiki/Packaging:Guidelines#Compiler_flags


> >As I understand it, this allows you to create games for the Game Boy? That should be fine. Does it contain any binary blobs or such that might be problematic?
> Games and utilities, correct.  I am aware the rgbfix utility contains a
> small bitmap of the Nintendo logo[0].
> 
> This bitmap must be present in the ROM in order for the console to boot into
> the game.  It is my understanding the presence of the logo, when it's
> required to pass hardware restrictions, is not a trademark violation,
> rather, it deemed fair use in the US (see [1]), but IANAL.
> 
> [0] https://github.com/rednex/rgbds/blob/v0.2.4/src/fix/main.c#L212
> [1] https://en.wikipedia.org/wiki/Sega_v._Accolade

I'm not capable of deciding that. Setting to block FE-Legal and will ask on the mailing list.

And in the meantime, please better remove this from Copr, before we know it's (not) OK.

The use of the Nintendo logo in this specific scenario is fair use in the United States, and is permissible. Please be careful not to expose it or use it in any other situations.

Lifting FE-Legal.

Thanks spot.

Thank you!

New version rgbds-0.2.5-1:

Spec URL: https://sanqui.net/etc/fedora/rgbds-0.2.5-1/rgbds.spec
SRPM URL: https://sanqui.net/etc/fedora/rgbds-0.2.5-1/rgbds-0.2.5-1.fc25.src.rpm
Copr URL: https://copr.fedorainfracloud.org/coprs/sanqui/rgbds/build/532649/
Scratch build: https://koji.fedoraproject.org/koji/taskinfo?taskID=18594180

I have corrected all the issues with the specfile.  I had to patch the Makefile in order to allow for producing correct debuginfo builds.  The patch was submitted upstream.

At the same time, I have updated the package to 0.2.5, released recently.

Issues:
=======
- Aesthetically, I'd add an empty line between %changelog entries.

- I see a test directory on upstream GitHub. Is there a testsuite you can run?

===== MUST items =====

C/C++:
[x]: Package does not contain kernel modules.
[x]: Package contains no static executables.
[x]: Header files in -devel subpackage, if present.
[x]: Package does not contain any libtool archives (.la)
[x]: Rpath absent or only used for internal libs.

Generic:
[x]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[x]: License field in the package spec file matches the actual license.
[x]: License file installed when any subpackage combination is installed.
[x]: If the package is under multiple licenses, the licensing breakdown
     must be documented in the spec.
Note: As it's complicated, only a "link" to LICENSE is provided.
[x]: %build honors applicable compiler flags or justifies otherwise.
[x]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[-]: Package contains desktop file if it is a GUI application.
[-]: Development files must be in a -devel package
[x]: Package uses nothing in %doc for runtime.
[x]: Package consistently uses macros (instead of hard-coded directory
     names).
[x]: Package is named according to the Package Naming Guidelines.
[x]: Package does not generate any conflict.
[x]: Package obeys FHS, except libexecdir and /usr/target.
[-]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[x]: Requires correct, justified where necessary.
[x]: Spec file is legible and written in American English.
[-]: Package contains systemd file(s) if in need.
[x]: Useful -debuginfo package or justification otherwise.
[x]: Package is not known to require an ExcludeArch tag.
[-]: Large documentation must go in a -doc subpackage. Large could be size
     (~1MB) or number of files.
     Note: Documentation size is 10240 bytes in 1 files.
[x]: Package complies to the Packaging Guidelines
[x]: Package successfully compiles and builds into binary rpms on at least
     one supported primary architecture.
[x]: Package installs properly.
[x]: Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).
[x]: If (and only if) the source package includes the text of the
     license(s) in its own file, then that file, containing the text of the
     license(s) for the package is included in %license.
[x]: Package requires other packages for directories it uses.
[x]: Package must own all directories that it creates.
[x]: Package does not own files or directories owned by other packages.
[x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[x]: Macros in Summary, %description expandable at SRPM build time.
[x]: Dist tag is present.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Package use %makeinstall only when make install DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[x]: Package does not use a name that already exists.
[x]: Package is not relocatable.
[x]: Sources used to build the package match the upstream source, as
     provided in the spec URL.
[x]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: File names are valid UTF-8.
[x]: Packages must not store files under /srv, /opt or /usr/local

===== SHOULD items =====

Generic:
[-]: If the source package does not include license text(s) as a separate
     file from upstream, the packager SHOULD query upstream to include it.
[x]: Final provides and requires are sane (see attachments).
[x]: Fully versioned dependency in subpackages if applicable.
     Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in rgbds-
     debuginfo - that's automatic, nothing we can do about it.
[?]: Package functions as described.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[x]: Patches link to upstream bugs/comments/lists or are otherwise
     justified.
[-]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[?]: Package should compile and build into binary rpms on all supported
     architectures.
[!]: %check is present and all tests pass.
[ ]: Packages should try to preserve timestamps of original installed
     files.
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: Sources can be downloaded from URI in Source: tag
[x]: SourceX is a working URL.
[x]: Spec use %global instead of %define unless justified.

===== EXTRA items =====

Generic:
[x]: Rpmlint is run on debuginfo package(s).
     Note: There are rpmlint messages (see attachment).
[x]: Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).
[-]: Large data in /usr/share should live in a noarch subpackage if package
     is arched.
[x]: Spec file according to URL is the same as in SRPM.


Rpmlint
-------
Checking: rgbds-0.2.5-1.fc27.x86_64.rpm
          rgbds-debuginfo-0.2.5-1.fc27.x86_64.rpm
          rgbds-0.2.5-1.fc27.src.rpm

rgbds.x86_64: W: spelling-error %description -l en_US checksum -> check sum, check-sum, checks um
Checksum is valid.

rgbds.x86_64: W: invalid-license DWPL
Has been added to the list.

rgbds.x86_64: W: only-non-binary-in-usr-lib
rgbds.x86_64: W: hidden-file-or-dir /usr/lib/.build-id
rgbds.x86_64: W: hidden-file-or-dir /usr/lib/.build-id
This is known issue with rpm, not a problem of this specific package.

rgbds-debuginfo.x86_64: W: invalid-license DWPL
See above.

rgbds.src: W: spelling-error %description -l en_US rgbasm -> orgasm
rgbds.src: W: spelling-error %description -l en_US rgblink -> blink
rgbds.src: W: spelling-error %description -l en_US rgbfix 
rgbds.src: W: spelling-error %description -l en_US rgbgfx 
Those are the names of the binaries, so it's not a spelling error.

rgbds.src: W: spelling-error %description -l en_US checksum -> check sum, check-sum, checks um
See above.

rgbds.src: W: invalid-license DWPL
See above.
3 packages and 0 specfiles checked; 0 errors, 12 warnings.




Rpmlint (debuginfo)
-------------------
Checking: rgbds-debuginfo-0.2.5-1.fc27.x86_64.rpm
rgbds-debuginfo.x86_64: W: invalid-license DWPL
See above.

1 packages and 0 specfiles checked; 0 errors, 1 warnings.

Requires
--------
rgbds-debuginfo (rpmlib, GLIBC filtered):

rgbds (rpmlib, GLIBC filtered):
    libc.so.6()(64bit)
    libm.so.6()(64bit)
    libpng16.so.16()(64bit)
    libpng16.so.16(PNG16_0)(64bit)
    rtld(GNU_HASH)



Provides
--------
rgbds-debuginfo:
    rgbds-debuginfo
    rgbds-debuginfo(x86-64)

rgbds:
    rgbds
    rgbds(x86-64)



Source checksums
----------------
https://github.com/rednex/rgbds/archive/v0.2.5.tar.gz :
  CHECKSUM(SHA256) this package     : 4bf10cbdd1f8f528c36d8ce92cc072d21e582fd75b5c21d13194b09d5c60ab25
  CHECKSUM(SHA256) upstream package : 4bf10cbdd1f8f528c36d8ce92cc072d21e582fd75b5c21d13194b09d5c60ab25

Spec URL: https://sanqui.net/etc/fedora/rgbds-0.2.5-2/rgbds.spec
SRPM URL: https://sanqui.net/etc/fedora/rgbds-0.2.5-2/rgbds-0.2.5-1.fc25.src.rpm
Scratch build: https://koji.fedoraproject.org/koji/taskinfo?taskID=18607028

>- Aesthetically, I'd add an empty line between %changelog entries.

I have added newlines between changelog entries.

>- I see a test directory on upstream GitHub. Is there a testsuite you can run?
Sadly, the tests are extremely primitive[1] and currently failing[2].  Hopefully in the future.

[1] https://github.com/rednex/rgbds/blob/v0.2.4/test/asm/test.sh
[2] https://paste.fedoraproject.org/paste/wHpYP9kkBOVVXDNpfoL2gV5M1UNdIGYhyRLivL9gydE=

Spec URL correction: https://sanqui.net/etc/fedora/rgbds-0.2.5-2/rgbds-0.2.5-2.fc25.src.rpm

Note: There are automatic tools (such as FedoraReview [0]) parsing the comments in a prescribed format. So if you ever want to correct a spec or srpm URL, please post it again:

Spec URL: https://sanqui.net/etc/fedora/rgbds-0.2.5-2/rgbds.spec
SRPM URL: https://sanqui.net/etc/fedora/rgbds-0.2.5-2/rgbds-0.2.5-2.fc25.src.rpm

---------------


Will you report the failing tests upstream?

---------------

Package is approved (informally, as you have not yet been sponsored).

---------------

About the sponsorship: Could you do a couple of informal reviews? Indicate that you are not yet sponsored and thus cannot do a proper review (and set no flags, do not assign to you). Please add me to CC when you do so.

There are dozens of open reviews for python packages at [1], if you want to go that way.

I also have a few open, if you want to do some reviews for me, see [2] (right column, "Packages" section, "Without reviewer"). (I'm not posting direct links, as they are very ugly and long.)

The review process is described at [3] and [4] - [0] will help you a lot.

---------------

[0] https://github.com/timlau/FedoraReview
[1] https://fedoraproject.org/wiki/SIGs/Python#Review_Python_packages
[2] https://fedoraproject.org/wiki/User:Churchyard
[3] https://fedoraproject.org/wiki/Package_Review_Process
[4] https://fedoraproject.org/wiki/Packaging:ReviewGuidelines

Thank you for help and approving the package!

I have reported failing tests upstream and we'll talk about how to improve them for the future: https://github.com/rednex/rgbds/issues/144

I will look into attempting some reviews soon.

Sponsored. Approved.

Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/rpms/rgbds

rgbds-0.2.5-2.fc26 has been submitted as an update to Fedora 26. https://bodhi.fedoraproject.org/updates/FEDORA-2017-4c1c2c0bb4

rgbds-0.2.5-2.fc25 has been submitted as an update to Fedora 25. https://bodhi.fedoraproject.org/updates/FEDORA-2017-cee91e573e

rgbds-0.2.5-2.fc24 has been submitted as an update to Fedora 24. https://bodhi.fedoraproject.org/updates/FEDORA-2017-de5a8e1a90

rgbds-0.2.5-2.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-de5a8e1a90

rgbds-0.2.5-2.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-cee91e573e

rgbds-0.2.5-2.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-4c1c2c0bb4

rgbds-0.2.5-2.fc26 has been pushed to the Fedora 26 stable repository. If problems still persist, please make note of it in this bug report.

rgbds-0.2.5-2.fc24 has been pushed to the Fedora 24 stable repository. If problems still persist, please make note of it in this bug report.

rgbds-0.2.5-2.fc25 has been pushed to the Fedora 25 stable repository. If problems still persist, please make note of it in this bug report.