Bug 1230161

Please note this is an informal review.

The spec looks ok to me, although I have found an issue: it is a GUI application, yet it does not provide a .desktop file (https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/Guidelines#desktop).

Checked with fedora-review, I did not find anything else worth commenting.

Okay, I've added a simple desktop file.

Spec URL: https://psabata.fedorapeople.org/pkgs/jumanji/jumanji.spec
SRPM URL: https://psabata.fedorapeople.org/pkgs/jumanji/jumanji-0-2.20111209git963b309.fc22.src.rpm

Thanks! It looks fine for me now.

Thank you, too.

Package Review
==============

Need Work:
1- You SHOULD add a appdata.xml file to show this app in Gnome Software
https://fedoraproject.org/wiki/Packaging:AppData

2- Test build passed:
http://koji.fedoraproject.org/koji/taskinfo?taskID=10874448
http://arm.koji.fedoraproject.org/koji/taskinfo?taskID=3147836
http://s390.koji.fedoraproject.org/koji/taskinfo?taskID=1939904
http://ppc.koji.fedoraproject.org/koji/taskinfo?taskID=2724943

3- This BR are not needed: coreutils make sed

4- This app do not have a icon, so I will recomend add this line
to the desktop file:

Icon=web-browser

5- [ ]: %build honors applicable compiler flags or justifies otherwise.
In build I will recomend to use make %{?_smp_mflags}

Please fix these issues to check:
[ ]: Package complies to the Packaging Guidelines

===== MUST items =====
C/C++:
Pass: Package does not contain kernel modules.
Pass: Package contains no static executables.
Pass: Package does not contain any libtool archives (.la)
Pass Rpath absent or only used for internal libs.

Generic:
Pass: Package is licensed with an open-source compatible license and meets
      other legal requirements as defined in the legal section of Packaging
      Guidelines.
Pass: License field in the package spec file matches the actual license.
Pass: Package contains no bundled libraries without FPC exception.
Pass: Changelog in prescribed format.
Pass: Sources contain only permissible code or content.
Pass: Development files must be in a -devel package
Pass: Package uses nothing in %doc for runtime.
Pass: Package consistently uses macros (instead of hard-coded directory
      names).
Pass: Package is named according to the Package Naming Guidelines.
Pass: Package does not generate any conflict.
Pass: Package obeys FHS, except libexecdir and /usr/target.
NA:   If the package is a rename of another package, proper Obsoletes and
      Provides are present.
Pass: Requires correct, justified where necessary.
Pass: Spec file is legible and written in American English.
Pass: Package contains systemd file(s) if in need.
Pass: Useful -debuginfo package or justification otherwise.
Pass: Package is not known to require an ExcludeArch tag.
Pass: 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.
Pass: Package successfully compiles and builds into binary rpms on at least
      one supported primary architecture.
Pass: Package installs properly.
Pass: Rpmlint is run on all rpms the build produces.
Pass: 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.
Pass: Package requires other packages for directories it uses.
Pass: Package must own all directories that it creates.
Pass: Package does not own files or directories owned by other packages.
Pass: Package uses either %{buildroot} or $RPM_BUILD_ROOT
Pass: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
      beginning of %install.
Pass: Macros in Summary, %description expandable at SRPM build time.
Pass: Package contains desktop file if it is a GUI application.
Pass: Package installs a %{name}.desktop using desktop-file-install or
      desktop-file-validate if there is such a file.
Pass: Dist tag is present.
Pass: Package does not contain duplicates in %files.
Pass: Permissions on files are set properly.
Pass: Package use %makeinstall only when make install DESTDIR=... doesn't
      work.
Pass: Package is named using only allowed ASCII characters.
Pass: Package does not use a name that already exists.
Pass: Package is not relocatable.
Pass: Sources used to build the package match the upstream source, as
      provided in the spec URL.
Pass: Spec file name must match the spec package %{name}, in the format
      %{name}.spec.
Pass: File names are valid UTF-8.
Pass: Packages must not store files under /srv, /opt or /usr/local

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

Generic:
NA:   If the source package does not include license text(s) as a separate
      file from upstream, the packager SHOULD query upstream to include it.
Pass: Final provides and requires are sane (see attachments).
Pass: Package functions as described.
Pass: Latest version is packaged.
Pass: Package does not include license text files separate from upstream.
Pass: SourceX tarball generation or download is documented.
Pass: Description and summary sections in the package spec file contains
      translations for supported Non-English languages, if available.
Pass: Package should compile and build into binary rpms on all supported
      architectures.
NA:   %check is present and all tests pass.
Pass: Packages should try to preserve timestamps of original installed
      files.
Pass: Packager, Vendor, PreReq, Copyright tags should not be in spec file
Pass: Reviewer should test that the package builds in mock.
Pass: Buildroot is not present
Pass: Package has no %clean section with rm -rf %{buildroot} (or
      $RPM_BUILD_ROOT)
Pass: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
Pass: SourceX is a working URL.
Pass: Spec use %global instead of %define unless justified.

===== EXTRA items =====
Generic:
Pass: Rpmlint is run on debuginfo package(s).
Pass: Rpmlint is run on all installed packages.
Pass: Large data in /usr/share should live in a noarch subpackage if package
      is arched.
Pass: Spec file according to URL is the same as in SRPM.

Rpmlint
-------
Checking: jumanji-0-2.20111209git963b309.fc21.x86_64.rpm
          jumanji-0-2.20111209git963b309.fc21.src.rpm
jumanji.x86_64: W: spelling-error Summary(en_US) customizable -> customization
jumanji.x86_64: W: spelling-error %description -l en_US customizable -> customization
jumanji.x86_64: W: spelling-error %description -l en_US vimperator -> impersonator, imperative
jumanji.src: W: spelling-error Summary(en_US) customizable -> customization
jumanji.src: W: spelling-error %description -l en_US customizable -> customization
jumanji.src: W: spelling-error %description -l en_US vimperator -> impersonator, imperative
jumanji.src: W: invalid-url Source0: jumanji-963b309.tar.gz
2 packages and 0 specfiles checked; 0 errors, 7 warnings.

Rpmlint (debuginfo)
-------------------
Checking: jumanji-debuginfo-0-2.20111209git963b309.fc21.x86_64.rpm
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

Rpmlint (installed packages)
----------------------------
jumanji-debuginfo.x86_64: W: invalid-url URL: https://pwmt.org/projects/jumanji <urlopen error [Errno -2] Name or service not known>
jumanji.x86_64: W: spelling-error Summary(en_US) customizable -> customization
jumanji.x86_64: W: spelling-error %description -l en_US customizable -> customization
jumanji.x86_64: W: spelling-error %description -l en_US vimperator -> impersonator, imperative
jumanji.x86_64: W: invalid-url URL: https://pwmt.org/projects/jumanji <urlopen error [Errno -2] Name or service not known>
2 packages and 0 specfiles checked; 0 errors, 5 warnings.

Requires
--------
jumanji (rpmlib, GLIBC filtered):
    /bin/sh
    desktop-file-utils
    libatk-1.0.so.0()(64bit)
    libc.so.6()(64bit)
    libcairo.so.2()(64bit)
    libfontconfig.so.1()(64bit)
    libfreetype.so.6()(64bit)
    libgdk-x11-2.0.so.0()(64bit)
    libgdk_pixbuf-2.0.so.0()(64bit)
    libgio-2.0.so.0()(64bit)
    libglib-2.0.so.0()(64bit)
    libgobject-2.0.so.0()(64bit)
    libgthread-2.0.so.0()(64bit)
    libgtk-x11-2.0.so.0()(64bit)
    libjavascriptcoregtk-1.0.so.0()(64bit)
    libm.so.6()(64bit)
    libpango-1.0.so.0()(64bit)
    libpangocairo-1.0.so.0()(64bit)
    libpangoft2-1.0.so.0()(64bit)
    libpthread.so.0()(64bit)
    libsoup-2.4.so.1()(64bit)
    libunique-1.0.so.0()(64bit)
    libwebkitgtk-1.0.so.0()(64bit)
    rtld(GNU_HASH)

Provides
--------
jumanji:
    application()
    application(jumanji.desktop)
    jumanji
    jumanji(x86-64)

(In reply to William Moreno from comment #5)
> 1- You SHOULD add a appdata.xml file to show this app in Gnome Software
> https://fedoraproject.org/wiki/Packaging:AppData

I don't think this browser aims at standard desktop users but I'll consider it.

> 3- This BR are not needed: coreutils make sed

They are.  Multiple utilities from coreutils are called in the Makefile, so is sed.  Additionally, sed is called in the spec file, too.

> 4- This app do not have a icon, so I will recomend add this line
> to the desktop file:
> 
> Icon=web-browser

This doesn't appear to be a universal name (few icon sets seem to have it) but I'm no expert on the matter.  I'll add it.

> 5- [ ]: %build honors applicable compiler flags or justifies otherwise.
> In build I will recomend to use make %{?_smp_mflags}

CFLAGS are set on line 45.  I'll add the flags for make.  Missed that, thanks.

b> Please fix these issues to check:
> [ ]: Package complies to the Packaging Guidelines

I hope it does :)

> Provides
> --------
> jumanji:
>     application()
>     application(jumanji.desktop)
>     jumanji
>     jumanji(x86-64)

Do you have an idea what the empty `application()' virtual provide means?

* make flags added
* Icon defined in the desktop file

Spec: https://psabata.fedorapeople.org/pkgs/jumanji/jumanji.spec
SRPM: https://psabata.fedorapeople.org/pkgs/jumanji/jumanji-0-3.20111209git963b309.fc22.src.rpm

Why are these hardcoded?

BuildRequires:    binutils
BuildRequires:    coreutils
BuildRequires:    make
BuildRequires:    pkgconfig
BuildRequires:    sed
BuildRequires:    glibc-devel

Because they're all needed for build of this package, Christopher.

(In reply to Petr Šabata from comment #9)
> Because they're all needed for build of this package, Christopher.

All these packages are (have been) part of the minimal build environment as stated in the Packaging:Guidelines, either directly or as dependency.

I tried to find recent documentation about this, but all I found is a change in the Packaging:Guidelines from 8. January 2015, where the list has been removed.
The description of the change is "Remove duplicate \"man pages\" section".
So the exceptions might still be valid.

True, there used to be a list of exceptions, packages that were guaranteed to be present in every buildroot.  This list was dropped for multiple reasons -- to simplify the guidelines and to achieve minimal smaller minimal buildroots, among others.  See the relevant discussions on devel@ and packaging@ mailing lists.

Nowadays, there is no guarantee for anything to be in the minimal buildroot and every package is expected to explicitly require what it uses.  There's nothing wrong with it, quite the contrary.

If you naively expect some of you dependencies to bring in some other of your dependencies for you, your package might break at any time in the future when the dependency chain changes.

Frankly, I don't understand this strong opposition against correct dependency lists.  It doesn't cause any harm, only helps.

There's nothing wrong with requiring what I need to build, of course.

I did not (yet) read the discussion, but one question pops up:
what's in the real actual minimal guaranteed buildroot ?
Obviously there must be something. Is there a list available somewhere ?

Or do I have to expect real nothing ?
But that would also include compilers (if needed), probably autofoo-stuff etc.

(In reply to Jens Lody from comment #12)
> There's nothing wrong with requiring what I need to build, of course.
> 
> I did not (yet) read the discussion, but one question pops up:
> what's in the real actual minimal guaranteed buildroot ?
> Obviously there must be something. Is there a list available somewhere ?
> 
> Or do I have to expect real nothing ?
> But that would also include compilers (if needed), probably autofoo-stuff
> etc.

Expect real nothing; that's the goal.  In practice you can, at the moment, expect dnf and rpm at least.  I can imagine that changing as well, though -- the buildroot could be populated from the outside.

There's no list.  If you need a compiler, buildrequire it.  If you need autotools et al., buildrequire them.  Personally, I would say this greatly simplifies the packaging work -- you don't have to remember any exceptions, check any lists.

Here's the relevant FPC ticket, by the way:
https://fedorahosted.org/fpc/ticket/497

(In reply to Petr Šabata from comment #14)
> Here's the relevant FPC ticket, by the way:
> https://fedorahosted.org/fpc/ticket/497

Thanks I will read this evening, if I'm back from work.

That means almost all packages lack BR, this included (it uses a c-compiler).

(In reply to Jens Lody from comment #15)
> (In reply to Petr Šabata from comment #14)
> > Here's the relevant FPC ticket, by the way:
> > https://fedorahosted.org/fpc/ticket/497
> 
> Thanks I will read this evening, if I'm back from work.
> 
> That means almost all packages lack BR, this included (it uses a c-compiler).

Yes, you're right, I'll add it right away :))

* added a build time dependency on gcc

Spec: https://psabata.fedorapeople.org/pkgs/jumanji/jumanji.spec
SRPM: https://psabata.fedorapeople.org/pkgs/jumanji/jumanji-0-4.20111209git963b309.fc22.src.rpm

I missed to set the Assigned to me .... any way the web-browser icon it is included in the hicolor-icon-theme package, So I will recommed to add Requires: hicolor-icon-theme.

I do not think than include some build depencies in the spec than are suposed to be part of the minimal build enviroment is a bloquer.

I will recomend to include the appdata.xml file in the package, more users will use the default firefox included, some user will use google chrome, advanced user can look for in the Gnome Sotware Center for others options available in Fedora repos and with out this file this app will be hiden.

> the web-browser icon it is included in the hicolor-icon-theme package,
> So I will recommed to add Requires: hicolor-icon-theme.

The hicolor-icon-theme doesn't contain any icons for a very long time. It contains and defines lots of directories:

$ rpmls hicolor-icon-theme|grep -v ^d
-rw-r--r--  /usr/share/doc/hicolor-icon-theme/README
-rw-r--r--  /usr/share/icons/hicolor/icon-theme.cache
-rw-r--r--  /usr/share/icons/hicolor/index.theme
-rw-r--r--  /usr/share/licenses/hicolor-icon-theme/COPYING

$ rpm -qR hicolor-icon-theme|grep -v ^rpm
/bin/sh
/bin/sh
/bin/sh
coreutils
coreutils

I dont' see any generic icon set that would provide that icon.  Also, people who use desktop environments with menus and icons probably have something with it already.  And people who don't would just get a package they have no need for.  I don't think requiring any icon set is a good idea.

You seem to believe appdata would be really useful, even in this niche case.  I shall add it then...

* added an appdata metadata file

Spec: https://psabata.fedorapeople.org/pkgs/jumanji/jumanji.spec
SRPM: https://psabata.fedorapeople.org/pkgs/jumanji/jumanji-0-5.20111209git963b309.fc22.src.rpm

Packaged Aproved
=================

Thank you.

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

jumanji-0-5.20111209git963b309.fc23 has been submitted as an update to Fedora 23. https://bodhi.fedoraproject.org/updates/FEDORA-2015-1f3a5f47cb

jumanji-0-5.20111209git963b309.fc22 has been pushed to the Fedora 22 testing repository. If problems still persist, please make note of it in this bug report.
If you want to test the update, you can install it with
$ su -c 'dnf --enablerepo=updates-testing update jumanji'
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2015-7b7388801e

jumanji-0-5.20111209git963b309.fc23 has been pushed to the Fedora 23 testing repository. If problems still persist, please make note of it in this bug report.
If you want to test the update, you can install it with
$ su -c 'dnf --enablerepo=updates-testing update jumanji'
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2015-1f3a5f47cb

jumanji-0-5.20111209git963b309.fc23 has been pushed to the Fedora 23 stable repository. If problems still persist, please make note of it in this bug report.

jumanji-0-5.20111209git963b309.fc22 has been pushed to the Fedora 22 stable repository. If problems still persist, please make note of it in this bug report.