Bug 770152

Just a cosmetic comment, because I don't currently have an f17 box for testing and the gtk3 package in f16 is too old to build this - you should be consistent in using one style of tag:

http://fedoraproject.org/wiki/Packaging:Guidelines#Using_.25.7Bbuildroot.7D_and_.25.7Boptflags.7D_vs_.24RPM_BUILD_ROOT_and_.24RPM_OPT_FLAGS

(In reply to comment #1)
> Just a cosmetic comment, because I don't currently have an f17 box for testing
> and the gtk3 package in f16 is too old to build this

Ah you might be interested in what is on http://teuf.fedorapeople.org/boxes/ then :) This is something I setup this afternoon and which saw very light testing, so no promises about running stuff from this repo. But if your main goal is to build the spec from this bug, you should find everything that you need here.

> you should be consistent in using one style of tag:
> 
> http://fedoraproject.org/wiki/Packaging:Guidelines#Using_.25.7Bbuildroot.7D_and_.25.7Boptflags.7D_vs_.24RPM_BUILD_ROOT_and_.24RPM_OPT_FLAGS

Ok thanks, I'll look into changing that.

The patch to lower the gtk+ version needs to be dropped, it's just a local hack and is unneeded in rawhide.
I looked at fixing the issue you pointed out, but only found occurrences of RPM_BUILD_ROOT, no %{buildroot}, and no %optflags/RPM_OPT_FLAGS. Did I miss something, or would you prefer that I use %{buildroot}?

Yes, just for consistency with the other tags.  It's a minor thing but does help readability.

I have uploaded a new spec and a new srpm to the URLs above.
Changes:
* s/$RPM_BUILD_ROOT/%{_buildroot}
* removed useless patch
* added fuseinfo dependency

Thanks for the new version.  Continuing the theme of pedantry, shouldn't it be
%{buildroot} rather than %{_buildroot}?

I know it's annoyingly inconsistent with the other tags...

Also, could you update the changelog with these latest updates?

Updated the .spec, and put an additional srpm at Spec URL: 
SRPM URL:
http://teuf.fedorapeople.org/reviews/gnome-boxes/gnome-boxes-3.3.3-2.fc16.src.rpm

Thanks for the update.  I've installed the requirements from your repo and tried building the SRPM.  It didn't work on F16, but I did manage to build, install and run it on F17 by manually building some of the dependencies.

Some comments:

- in the changelog, you need to escape the macro, so %%{buildroot}

- did you intend to put something in the %pre section?

- the address in the COPYING file is wrong - please use the most recent version

- it's not required for the review, but please consider including a manpage

(In reply to comment #8)
> 
> Some comments:
> 
> - in the changelog, you need to escape the macro, so %%{buildroot}
> 

Fixed (locally)

> - did you intend to put something in the %pre section?
> 

Nope, it's some copy and paste from totem, I removed it

> - the address in the COPYING file is wrong - please use the most recent version
> 

I sent a patch upstream to fix this

> - it's not required for the review, but please consider including a manpage

I opened a bug upstream about it: https://bugzilla.gnome.org/show_bug.cgi?id=667340

Updated SRPM at http://teuf.fedorapeople.org/reviews/gnome-boxes/gnome-boxes-3.3.3-3.fc16.src.rpm
The .spec is still http://teuf.fedorapeople.org/reviews/gnome-boxes/gnome-boxes.spec

FYI.

Looking at the source, you might want to add:
  mtools
as Requires: (used for the floppy handling)

Also, GNOME boxes starts, but doesn't do anything unless you have qemu or similar, so you might want some requires on that.

See https://mail.gnome.org/archives/gnome-boxes-list/2012-January/msg00009.html for the discussion on getting GNOME boxes working on Mageia.

The (latest) spec file Mageia is at:
http://svnweb.mageia.org/packages/cauldron/gnome-boxes/current/SPECS/gnome-boxes.spec?view=co

(In reply to comment #11)
> FYI.
> 
> Looking at the source, you might want to add:
>   mtools
> as Requires: (used for the floppy handling)
> 

Ah right, thanks! I've also added this locally:
# gnome-boxes uses a dark theme
Requires: gnome-icon-theme 


> Also, GNOME boxes starts, but doesn't do anything unless you have qemu or
> similar, so you might want some requires on that.

yeah I have those locally as well but I'm unsure whether they should be here as hard deps or not. For now I guess it makes sense to have them, though in the future we can imagine connecting to remote vms.

I updated the package to the latest release (3.3.4)
Spec URL: http://teuf.fedorapeople.org/reviews/gnome-boxes/gnome-boxes.spec
SRPM URL: http://teuf.fedorapeople.org/reviews/gnome-boxes/gnome-boxes-3.3.4-1.fc16.src.rpm

Trying to build with mock show me that spice-gtk3-vala is missing for now ( likely mirror lag, so I will try again later ).

Along various remark, it seems :
- that BuildRoot should be removed
- that %clean is not needed anymore
- rm -rf %{buildroot}  should also be removed
- and %defattr 

And the spec should use %global, not  %define.

All fixed in the -2 package at http://teuf.fedorapeople.org/reviews/gnome-boxes/gnome-boxes-3.3.4-2.fc16.src.rpm

I'll do a formal review later on today, unless you want to take it Michael.

Go for it, I never did a review, so someone would have to check in the end :)

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

Key:
- = N/A
x = Pass
! = Fail
? = Not evaluated



==== Generic ====
[ ]: MUST Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[!]: MUST Package successfully compiles and builds into binary rpms on at
     least one supported architecture.
[!]: MUST All build dependencies are listed in BuildRequires, except for any
     that are listed in the exceptions section of Packaging Guidelines.
     Note: The package did not built BR could therefore not be checked or the
     package failed to build because of missing BR
[x]: MUST Buildroot is not present
     Note: Unless packager wants to package for EPEL5 this is fine
[ ]: MUST Package contains no bundled libraries.
[ ]: MUST Changelog in prescribed format.
[x]: MUST Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
     Note: Clean would be needed if support for EPEL is required
[ ]: MUST Sources contain only permissible code or content.
[x]: MUST Each %files section contains %defattr if rpm < 4.4
     Note: Note: defattr macros not found. They would be needed for EPEL5
[ ]: MUST Macros in Summary, %description expandable at SRPM build time.
[ ]: MUST Package requires other packages for directories it uses.
[ ]: MUST Package uses nothing in %doc for runtime.
[ ]: MUST Package is not known to require ExcludeArch.
[x]: MUST Permissions on files are set properly.
[x]: MUST Package does not contain duplicates in %files.
[x]: MUST Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
     Note: rm -rf would be needed if support for EPEL5 is required
[ ]: MUST 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 %doc.
[ ]: MUST License field in the package spec file matches the actual license.
[!]: MUST Package consistently uses macros (instead of hard-coded directory
     names).
     Note: Using both %{buildroot} and $RPM_BUILD_ROOT
[ ]: MUST Package meets the Packaging Guidelines.
[x]: MUST Package is named according to the Package Naming Guidelines.
[ ]: MUST Package does not generates any conflict.
[ ]: MUST Package obeys FHS, except libexecdir and /usr/target.
[ ]: MUST Package must own all directories that it creates.
[ ]: MUST Package does not own files or directories owned by other packages.
[ ]: MUST Package installs properly.
[ ]: MUST Requires correct, justified where necessary.
[x]: MUST Rpmlint output is silent.
[!]: MUST Sources used to build the package match the upstream source, as
     provided in the spec URL.
/home/adam/Fedora/fedora-review/770152/gnome-boxes-3.3.4.tar.xz :
  MD5SUM this package     : 2e203398f1912ddc47ba86ea7514d12e
  MD5SUM upstream package : d41d8cd98f00b204e9800998ecf8427e

[ ]: MUST Spec file is legible and written in American English.
[x]: MUST Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[ ]: MUST Package contains a SysV-style init script if in need of one.
[x]: MUST File names are valid UTF-8.
[!]: SHOULD Reviewer should test that the package builds in mock.
[ ]: SHOULD 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]: SHOULD Dist tag is present.
[ ]: SHOULD No file requires outside of /etc, /bin, /sbin, /usr/bin,
     /usr/sbin.
[ ]: SHOULD Final provides and requires are sane (rpm -q --provides and rpm -q
     --requires).
[ ]: SHOULD Package functions as described.

[ ]: SHOULD Package does not include license text files separate from
     upstream.
[ ]: SHOULD Scriptlets must be sane, if used.
[x]: SHOULD SourceX is a working URL.
[ ]: SHOULD Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[ ]: SHOULD Package should compile and build into binary rpms on all supported
     architectures.
[ ]: SHOULD %check is present and all tests pass.
[ ]: SHOULD Packages should try to preserve timestamps of original installed
     files.
[!]: SHOULD Spec use %global instead of %define.
     Note: %define

Issues:
[!]: MUST Package successfully compiles and builds into binary rpms on at
     least one supported architecture.
[!]: MUST All build dependencies are listed in BuildRequires, except for any
     that are listed in the exceptions section of Packaging Guidelines.
     Note: The package did not built BR could therefore not be checked or the
     package failed to build because of missing BR
[!]: MUST Package consistently uses macros (instead of hard-coded directory
     names).
     Note: Using both %{buildroot} and $RPM_BUILD_ROOT
[!]: MUST Sources used to build the package match the upstream source, as
     provided in the spec URL.
/home/adam/Fedora/fedora-review/770152/gnome-boxes-3.3.4.tar.xz :
  MD5SUM this package     : 2e203398f1912ddc47ba86ea7514d12e
  MD5SUM upstream package : d41d8cd98f00b204e9800998ecf8427e

Could you check the tarball because of this checksum mismatch?

The mention of $RPM_BUILD_ROOT is a false alarm I think because it's in the changelog.

I'll see if I can build it in my F17 VM.

Yes, it builds and installs in the F17 VM, if I manually rebuild the updated dependencies.

(In reply to comment #18)
> Issues:
> [!]: MUST Package successfully compiles and builds into binary rpms on at
>      least one supported architecture.
> [!]: MUST All build dependencies are listed in BuildRequires, except for any
>      that are listed in the exceptions section of Packaging Guidelines.
>      Note: The package did not built BR could therefore not be checked or the
>      package failed to build because of missing BR

I wanted to push a rawhide scratch build with koji to test this, but rawhide seems to be broken atm :-/ 

DEBUG util.py:257:  Error: Package: clutter-gtk-1.1.2-2.fc17.x86_64 (build)
DEBUG util.py:257:             Requires: libcogl.so.6()(64bit)
DEBUG util.py:257:   You could try using --skip-broken to work around the problem
DEBUG util.py:257:   You could try running: rpm -Va --nofiles --nodigest

( http://koji.fedoraproject.org/koji/taskinfo?taskID=3714490 )

> [!]: MUST Package consistently uses macros (instead of hard-coded directory
>      names).
>      Note: Using both %{buildroot} and $RPM_BUILD_ROOT

Yup, I agree it's probably confused by $RPM_BUILD_ROOT appearing in the changelog

> [!]: MUST Sources used to build the package match the upstream source, as
>      provided in the spec URL.
> /home/adam/Fedora/fedora-review/770152/gnome-boxes-3.3.4.tar.xz :
>   MD5SUM this package     : 2e203398f1912ddc47ba86ea7514d12e
>   MD5SUM upstream package : d41d8cd98f00b204e9800998ecf8427e
> 
> Could you check the tarball because of this checksum mismatch?

Started looking into this, the tarball on gnome.org has the same md5sum as the one in my srpm (phew), I think what happens is that %global url_ver %(echo %{version}|cut -d. -f1,2 doesn't work as expected, and http://download.gnome.org/sources/%{name}/%{url_ver}/%{name}-%{version}.tar.xz gives a 404 page which gives this d41d8cd md5sum. I'll investigate more

The "[!]: SHOULD Spec use %global instead of %define" is probably also a false alarm due to %define being in the changelog.

This is caused by using %global to define url_ver, changing it back to %define fixes the expansion. No idea what the difference is :(

Alternatively, this works too:
-%global url_ver        %(echo %{version}|cut -d. -f1,2)
+%global url_ver        %%(echo %{version}|cut -d. -f1,2)

(In reply to comment #22)
> Alternatively, this works too:
> -%global url_ver        %(echo %{version}|cut -d. -f1,2)
> +%global url_ver        %%(echo %{version}|cut -d. -f1,2)

That's a little ugly but still better than hand-coding the version in the URL, I suppose.

Yes it's slightly better I think, but I don't mind dropping it if you prefer.

Uploaded a -3 package with this fix at http://teuf.fedorapeople.org/reviews/gnome-boxes/gnome-boxes-3.3.4-2.fc16.src.rpm , .spec at the usual URL has been updated too.

Assuming you meant http://teuf.fedorapeople.org/reviews/gnome-boxes/gnome-boxes-3.3.4-3.fc16.src.rpm (added so the review script picks it up)

Here's the updated review script output:

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

Key:
- = N/A
x = Pass
! = Fail
? = Not evaluated



==== Generic ====
[ ]: MUST Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[x]: MUST Package successfully compiles and builds into binary rpms on at
     least one supported architecture.
[x]: MUST All build dependencies are listed in BuildRequires, except for any
     that are listed in the exceptions section of Packaging Guidelines.
[x]: MUST Buildroot is not present
     Note: Unless packager wants to package for EPEL5 this is fine
[ ]: MUST Package contains no bundled libraries.
[ ]: MUST Changelog in prescribed format.
[x]: MUST Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
     Note: Clean would be needed if support for EPEL is required
[ ]: MUST Sources contain only permissible code or content.
[x]: MUST Each %files section contains %defattr if rpm < 4.4
     Note: Note: defattr macros not found. They would be needed for EPEL5
[ ]: MUST Macros in Summary, %description expandable at SRPM build time.
[!]: MUST Package contains a properly installed %{name}.desktop using desktop-
     file-install file if it is a GUI application.
[ ]: MUST Package requires other packages for directories it uses.
[ ]: MUST Package uses nothing in %doc for runtime.
[ ]: MUST Package is not known to require ExcludeArch.
[x]: MUST Permissions on files are set properly.
[x]: MUST Package does not contain duplicates in %files.
[x]: MUST Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
     Note: rm -rf would be needed if support for EPEL5 is required
[x]: MUST 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 %doc.
[ ]: MUST License field in the package spec file matches the actual license.
[ ]: MUST The spec file handles locales properly.
[!]: MUST Package consistently uses macros (instead of hard-coded directory
     names).
     Note: Using both %{buildroot} and $RPM_BUILD_ROOT
[ ]: MUST Package meets the Packaging Guidelines.
[x]: MUST Package is named according to the Package Naming Guidelines.
[ ]: MUST Package does not generates any conflict.
[ ]: MUST Package obeys FHS, except libexecdir and /usr/target.
[ ]: MUST Package must own all directories that it creates.
[ ]: MUST Package does not own files or directories owned by other packages.
[ ]: MUST Package installs properly.
[ ]: MUST Requires correct, justified where necessary.
[!]: MUST Rpmlint output is silent.

rpmlint gnome-boxes-debuginfo-3.3.4-3.fc17.x86_64.rpm

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


rpmlint gnome-boxes-3.3.4-3.fc17.x86_64.rpm

gnome-boxes.x86_64: W: no-manual-page-for-binary gnome-boxes
1 packages and 0 specfiles checked; 0 errors, 1 warnings.


rpmlint gnome-boxes-3.3.4-3.fc17.src.rpm

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


[x]: MUST Sources used to build the package match the upstream source, as
     provided in the spec URL.
/home/adam/Fedora/fedora-review/770152/gnome-boxes-3.3.4.tar.xz :
  MD5SUM this package     : 2e203398f1912ddc47ba86ea7514d12e
  MD5SUM upstream package : 2e203398f1912ddc47ba86ea7514d12e

[ ]: MUST Spec file is legible and written in American English.
[x]: MUST Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[ ]: MUST Package contains a SysV-style init script if in need of one.
[x]: MUST File names are valid UTF-8.
[x]: SHOULD Reviewer should test that the package builds in mock.
[ ]: SHOULD 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]: SHOULD Dist tag is present.
[ ]: SHOULD No file requires outside of /etc, /bin, /sbin, /usr/bin,
     /usr/sbin.
[ ]: SHOULD Final provides and requires are sane (rpm -q --provides and rpm -q
     --requires).
[ ]: SHOULD Package functions as described.
[ ]: SHOULD Package does not include license text files separate from
     upstream.
[ ]: SHOULD Scriptlets must be sane, if used.
[x]: SHOULD SourceX is a working URL.
[ ]: SHOULD Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[ ]: SHOULD Package should compile and build into binary rpms on all supported
     architectures.
[ ]: SHOULD %check is present and all tests pass.
[ ]: SHOULD Packages should try to preserve timestamps of original installed
     files.
[!]: SHOULD Spec use %global instead of %define.
     Note: %define

Issues:
[!]: MUST Package contains a properly installed %{name}.desktop using desktop-
     file-install file if it is a GUI application.
[!]: MUST Package consistently uses macros (instead of hard-coded directory
     names).
     Note: Using both %{buildroot} and $RPM_BUILD_ROOT
[!]: MUST Rpmlint output is silent.

rpmlint gnome-boxes-debuginfo-3.3.4-3.fc17.x86_64.rpm

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


rpmlint gnome-boxes-3.3.4-3.fc17.x86_64.rpm

gnome-boxes.x86_64: W: no-manual-page-for-binary gnome-boxes
1 packages and 0 specfiles checked; 0 errors, 1 warnings.


rpmlint gnome-boxes-3.3.4-3.fc17.src.rpm

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


Generated by fedora-review 0.1.1

- We've covered the lack of a man page already and the false positive re. macros
- The remaining error relates to the .desktop file - I think you need to add the snippet listed at https://fedoraproject.org/wiki/Packaging/Guidelines#desktop

http://teuf.fedorapeople.org/reviews/gnome-boxes/gnome-boxes-3.3.4-4.fc16.src.rpm

* Fri Jan 20 2012 Christophe Fergeau <cfergeau> - 3.3.4-4
- call desktop-file-validate in %%install. gnome-boxes upstream installs
  a .desktop file on its own so desktop-file-validate is enough, no need
  to call desktop-file-install.

Okay, good.

Thanks for your patience with the review.

APPROVED

New Package SCM Request
=======================
Package Name: gnome-boxes
Short Description: A simple GNOME 3 application to access remote or virtual systems
Owners: teuf elmarco zeenix
Branches:
InitialCC: teuf elmarco zeenix

Git done (by process-git-requests).