Bug 448717 (gnome-rdp)

Added macros to %files section

I don't see that the user janderson has been sponsored; setting NEEDSPONSOR.  If
that's not you, my apologies.

I'm doing a pre-review to become sponsored and these are my comments on your
package. 

* rpmlint gives errors and warnings. See
http://fedoraproject.org/wiki/Packaging/Guidelines#Use_rpmlint
* %defattr is placed a bit too late, should be before %files
* your program uses localized files. You must not use %{_datadir}/locale/ to
place them sonewhere. You should use findlang. See
http://fedoraproject.org/wiki/Packaging/Guidelines#Handling_Locale_Files
* BuildRequires: are you sure there are no redundant requirements? (glib2-devel
gtk2-devel gtk-sharp2-devel gnome-sharp-devel)
* if your package contains a GUI application, please follow
http://fedoraproject.org/wiki/Packaging/Guidelines#Desktop_files
* Group: I'd use Application/Internet rater than Applications/System. All
vnc/rdesktop/tsclient applications are there.
* Patch0: why do you call it gnome-rdp-fedora.patch? It is not fedora specific,
so you should call it different (distribution neutral).
* License: please include the license file as documentation. See
http://fedoraproject.org/wiki/Packaging/LicensingGuidelines#License_Text

That's all for now. Again: this is just my pre-review, not an official review. 

One really small last thing: don't add empty lines after the %description, and
please add a dot at the end of sentences. No dots after summary, but after
description.

I've made changes based on Guido Ledermann's comments, I believe most of them
should be fixed now.

Also since the package came with a .desktop file, I'm using
desktop-file-validate instead of desktop-file-install, which was suggested to me
in #fedora-devel.

(In reply to comment #5)
> Also since the package came with a .desktop file, I'm using
> desktop-file-validate instead of desktop-file-install, which was suggested to me
> in #fedora-devel.

Well, but if you used desktop-file-install you wouldn't need the
gnome-rdp-desktop-validate.patch any more. Simply use

  desktop-file-install --vendor=""                           \
    --remove-category="Internet"                             \
    --delete-original                                        \
    --dir=%{buildroot}%{_datadir}/applications               \
    %{buildroot}/%{_datadir}/applnk/Multimedia/foo.desktop

and drop the patch. _If_ you use desktop-file-install you need to check the file
before it is installed and not afterwards. At the moment you are running
desktop-file-validate after make install.

A few more comments:

- License tag in the spec is wrong. Please take a look at the headers of the
files inside src:
"... either version 2 of the License, or (at your option) any later version."
"or later" means GPLv2+ instead of GPLv2.

- You are using find_lang, but you forgot to build require gettext. You'll also
need perl(XML::Parser).

- %{_libdir}/gnome-rdp/ is unowned and will be left over if you uninstall
gnome-rdp. Change 
  %{_libdir}/gnome-rdp/gnome-rdp.exe to
  %{_libdir}/gnome-rdp/
so the whole directory is included.

%{_datadir}/man/man1/gnome-rdp.1.gz should be
%{_mandir}/man1/gnome-rdp.1.gz (or %{_mandir}/man1/%{name}.1.gz)

- Include AUTHORS, ChangeLog and README in %doc, but not INSTALL (because it's
generic) and NEWS (empty)

- mock builds fail:

Ausführung(%build): /bin/sh -e /var/tmp/rpm-tmp.pA6JFs
+ umask 022
+ cd /builddir/build/BUILD
+ cd gnome-rdp-0.2.2.3
+ LANG=C
+ export LANG
+ unset DISPLAY
+ autoconf
aclocal.m4:14: error: this file was generated for autoconf 2.61.
You have another version of autoconf.  If you want to use that,
you should regenerate the build system entirely.
aclocal.m4:14: the top level
autom4te: /usr/bin/m4 failed with exit status: 63
Fehler beim Bauen des RPMS:
Fehler: Fehler-Status beim Beenden von /var/tmp/rpm-tmp.pA6JFs (%build)
    Fehler-Status beim Beenden von /var/tmp/rpm-tmp.pA6JFs (%build)
Child returncode was: 1

Please install mock and try to build the package against rawhide. You'll
possibly need "autoreconf -f" instead of autoconf. 

And this time please increase the release, so the new package would be
0.2.2.3-2. This makes it easier for the reviewers to distinguish between
different packages. We need to be able to track your changes, so please include
proper information in %changelog.

If you have problems please let me know.

(In reply to comment #6)
> If_ you use desktop-file-install you need to check the file
> before it is installed and not afterwards. At the moment you are running
> desktop-file-validate after make install.
>

Posting a new version.

http://transfer.eragen.com/rpm/gnome-rdp.spec
http://transfer.eragen.com/rpm/gnome-rdp-0.2.2.3-2.fc9.src.rpm

I couldn't get it to work without errors unless it followed the make install.
Could you have a look and explain how it should different if it's wrong?
 
> Please install mock and try to build the package against rawhide. You'll
> possibly need "autoreconf -f" instead of autoconf. 
>

I tried to do a mock on rawhide, however I can't currently get libattr through
yum so it fails. I did however change to autoreconf.

I made a few changes to deps and added a patch to get correct problems with the
VTE libraries. It now builds against rawhide i386 in my mock.

http://transfer.eragen.com/rpm/gnome-rdp-0.2.2.3-3.fc10.src.rpm
http://transfer.eragen.com/rpm/gnome-rdp.spec

I've done some additional work.

The current version builds against fc10 for i386 and x86_64. I've tested the package on the live cd for the alpha and it seems to run correctly.

http://transfer.eragen.com/rpm/gnome-rdp.spec
http://transfer.eragen.com/rpm/gnome-rdp-0.2.2.3-5.fc10.src.rpm

http://koji.fedoraproject.org/koji/taskinfo?taskID=764772

Stay tuned for full review this weekend (most likely tomorrow night).

(In reply to comment #10)
> Stay tuned for full review this weekend (most likely tomorrow night).

I look forward to it.

There are still a lot of issues with this package so I don't think that a full review makes sense at this state.

1) rpmlint /var/lib/mock/fedora-rawhide-i386/result/gnome-rdp-*
gnome-rdp.i386: W: file-not-utf8 /usr/share/doc/gnome-rdp-0.2.2.3/AUTHORS
gnome-rdp.i386: W: file-not-utf8 /usr/share/doc/gnome-rdp-0.2.2.3/ChangeLog
- this is minor and can be fixed with iconv

gnome-rdp.i386: E: description-line-too-long gnome-rdp is a Remote Desktop Protocol client for the GNOME desktop environment.
- description needs a line wrap after 79 characters

gnome-rdp.i386: E: no-binary
gnome-rdp.i386: E: only-non-binary-in-usr-lib
- because it's mono, save to ignore

gnome-rdp-debuginfo.i386: E: empty-debuginfo-package
3 packages and 0 specfiles checked; 5 errors, 2 warnings.
- why is there no debuginfo? if there really is none you should disable building the debuginfo package

2) IMO the description could be a little more elaborate, e. g.:
"gnome-rdp is a Remote Desktop Protocol client for the GNOME desktop environment. It supports RDP, VNC and SSH. Configured sessions can be saved to the built in list." (Taken from the sf.net website)

3) You are not providing a URL for Source0, see
http://fedoraproject.org/wiki/Packaging/SourceURL

4) You are using ExcludeArch for PPC and PPC64, but you are not providing any details about the reasons. Please read the corresponding paragraph at 
http://fedoraproject.org/wiki/Packaging/ReviewGuidelines

5) Looks as if you are missing some (Build)Requires:
  checking for vncviewer... vncviewer
  checking for rdesktop... rdesktop
  checking for ssh... ssh
  checking for gnome-terminal... gnome-terminal

6) On F9 build fails with
  configure: error: Package requirements (vte-sharp-0.16 >= 1.9) were not met:
  No package 'vte-sharp-0.16' found
Can you tighten the BuildRequires so that RPM will refuse to install the SRPM if the requirements are not met?

7) There must be one blank line between every changelog entry. This is important because the changelogs get parsed automatically by a couple of scripts.

8) This is not the latest version. 0.2.3 is out since 2008-06-27.

Unfortunately the new version adds gnome-keyring-sharp as a dependency, which is not yet in Fedora. I'll look at that library and see how easy it would be to package.

I see gnome-keyring-sharp is not easy to remove, so we have two options:
1. stick with the previous version but fix the issues I mentioned. In this case please ask upstream if they can make gnome-keyring-sharp optional.
2. submit a review request for gnome-keyring-sharp and wait until it is finished.

gnome-keyring-sharp has been in Rawhide for a couple of months; I was not aware that it is being used by anything other than gnome-do (which has some dependencies not in Fedora-9, thus it is currently Rawhide only).

I have just built gnome-keyring-sharp for F-8 and F-9:
https://admin.fedoraproject.org/updates/gnome-keyring-sharp-1.0.0-0.2.87622svn.fc8
https://admin.fedoraproject.org/updates/gnome-keyring-sharp-1.0.0-0.2.87622svn.fc9

Feel free to use it for gnome-rdp, and report any bugs (or vote it up if it works for you).

Michel thanks for the info on gonme-keyring-sharp.

I'll work on getting gnome-rdp-0.2.3 to build against this and post shortly.

Here is the new version, plus many fixes:

http://transfer.eragen.com/rpm/gnome-rdp.spec
http://transfer.eragen.com/rpm/gnome-rdp-0.2.3-1.fc10.src.rpm

It will only build against fc10 due to the gnome-desktop-sharp dependency.

Review for
1e1a7230912de6e6b287c3ce7215eac6  gnome-rdp-0.2.3-1.fc10.src.rpm

FIX - MUST: 
# rpmlint /var/lib/mock/fedora-rawhide-i386/result/gnome-rdp-0.2.3-1.fc10.*
gnome-rdp.i386: E: no-binary
gnome-rdp.i386: E: only-non-binary-in-usr-lib
gnome-rdp.src:53: W: rpm-buildroot-usage %build nant -D:DESTDIR=%{buildroot} -D:libdir=%{_libdir}
2 packages and 0 specfiles checked; 2 errors, 1 warnings.

The first two errors are because of mono and can be ignored, but the warning needs to be fixed for several reasons: $RPM_BUILD_ROOT/%{buildroot} should not be touched during %build or %prep stage, as it will break short circuiting.
DESTDIR is only needed for install, for compiling the package we need to set the prefix. So the correct nant call is
  nant -D:prefix=%{_prefix} -D:libdir=%{_libdir}

OK - MUST: The package is named according to the  Package Naming Guidelines
OK - MUST: The spec file name matches the base package %{name}, in the format %{name}.spec
OK - MUST: The package meets the Packaging Guidelines
FIX - MUST: The package is licensed with a Fedora approved license and meets the Licensing Guidelines, but the included license text and the sources don't match any longer. Text is GLPv3, but the headers still say it's GPLv2+. I guess upstream switched to GPLv3+, but I'd like you to contact them and ask for clarification
FIX - MUST: Please change the License field in the package spec file to match the actual license when you really know which one is correct
OK - MUST: The source package includes the text of the license and it is correctly included in %doc
OK - MUST: The spec file is written in American English
OK - MUST: The spec file for the package is legible
OK - MUST: The source used to build the package match the upstream source by md5 1297c536e8d84c05113bc744d6829b54
OK - MUST: The package successfully compiles and builds into binary rpms on Rawhide i386
OK - MUST: The package does not successfully compile on PPC(64), but those architectures are listed in the spec in ExcludeArch.
OK - MUST: All build dependencies are listed in BuildRequires, but
  gnome-desktop-sharp >= 2.20.1 can be removed since you already require 
  gnome-desktop-sharp-devel >= 2.20.1
OK - MUST: The spec file handles locales properly with the %find_lang macro
OK - MUST: The package is not designed to be relocatable
OK - MUST: The package owns all directories that it creates
OK - MUST: The package does not contain any duplicate files in the %files listing
OK - MUST: Permissions on files are set properly, %files section includes a %defattr(...) line
OK - MUST: The package has a %clean section, which contains rm -rf $RPM_BUILD_ROOT
OK - MUST: The package consistently uses macros, as described in the macros section of Packaging Guidelines
OK - MUST: The package contains code, not content
OK - MUST: No large documentation files for a -doc subpackage
OK - MUST: Files in %doc don't affect the runtime of the application
OK - MUST: The package does not contain any .la libtool archives
OK - MUST: The package includes a %{name}.desktop file, which is properly installed with desktop-file-install in the %install section
OK - MUST: The package does not own files or directories already owned by other packages
OK - MUST: rm -rf %{buildroot} at the beginning of %install
OK - MUST: All filenames in the package are valid UTF-8
OK - SHOULD: The package builds in mock
OK - SHOULD: The package functions as described

NEEDSWORK, but looks good so far. The only thing that needs to be cleared it the license.

Two minor notes:
Changelog: If you mention you fixed something "per review" or that you fixed a bug you should also mention the bug #. This is usually done like this:
  - Fixed comments from Christoph Wickert and Guido Ledermann per review (#448717)

ExcludeArch:
Please don't forget to open a bug for the excluded archs after the CVS Admin procedure. You only need to write a short description why this package does not build (the one in the spec is enough) and then make the bug block bug # 179260 and bug # 238953. After that you can close the bug CANTFIX, but please add the # to the comment in the spec. For more info please read the corresponding paragraph at http://fedoraproject.org/wiki/Packaging/ReviewGuidelines
BTW: The new mono SIG is working on getting the PPC issues fixed, so I hope mono will be available there soon.

(In reply to comment #18)
> The only thing that needs to be cleared is the license.

Not to forget the rpmlint warning...

I've confirmed with upstream they're now on GPL3+ and adjusted the SPEC accordingly. Also fixed the warning.

> On Thu, Sep 11, 2008 at 9:53 AM, David Paleino <d.paleino> wrote:
> > I hereby confirm that Gnome-RDP is now being released under the terms of GNU
> > General Public License v3 or, at your option, any later version.

http://transfer.eragen.com/rpm/gnome-rdp.spec
http://transfer.eragen.com/rpm/gnome-rdp-0.2.3-2.fc10.src.rpm

All (In reply to comment #20)
> I've confirmed with upstream they're now on GPL3+ and adjusted the SPEC
> accordingly.

That was my guess as well.

gnome-rdp-0.2.3-2.fc10.src.rpm fixes all outstanding issues, so this package is APPROVED.

Now it's time for the CVSAdmin procedure and to request membership to the packager group in the accounts system, see
http://fedoraproject.org/wiki/PackageMaintainers/CVSAdminProcedure and
http://fedoraproject.org/wiki/PackageMaintainers/Join#Get_a_Fedora_Account

New Package CVS Request
=======================
Package Name: gnome-rdp
Short Description: Remote Desktop Protocol client for GNOME
Owners: janderson
Branches:
InitialCC:

Hey John. I don't see you in the packager group yet. 
Have you requested that group so that Christoph can sponsor you?

Hello Kevin,

I have requested the group in FAS.

Thanks,

John

John, do you have other packages or did you participate in any reviews? Please show me some more of your work before I sponsor you. TIA!

Clearing the cvs flag here for now. 
Please reset it when you are ready for cvs.

Removing NEEDSPONSOR as I will sponsor John. Also re-adding the fedora‑cvs? flag.

John, please follow the instructions from
http://fedoraproject.org/wiki/PackageMaintainers/Join#Get_a_Fedora_Account

Thanks for the sponsorship! I've applied to the packager group as janderson.

Ok, I see you already did that 6 weeks ago and I have picked up your request now.

cvs done

gnome-rdp-0.2.3-2.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/gnome-rdp-0.2.3-2.fc10

gnome-rdp-0.2.3-2.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.