Bug 1060419 - Review Request: gwakeonlan - A GTK+ utility to awake machines using the Wake on LAN
Summary: Review Request: gwakeonlan - A GTK+ utility to awake machines using the Wake ...
Keywords:
Status: CLOSED NOTABUG
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
unspecified
medium
Target Milestone: ---
Assignee: Nobody's working on this, feel free to take it
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: FE-NEEDSPONSOR
TreeView+ depends on / blocked
 
Reported: 2014-02-01 14:29 UTC by Alberto Chiusole
Modified: 2016-02-16 00:01 UTC (History)
5 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2016-02-16 00:01:04 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)

Description Alberto Chiusole 2014-02-01 14:29:34 UTC
Spec URL: rpm.bebosudo.tk/SPECS/gwakeonlan.spec
SRPM URL: rpm.bebosudo.tk/SRPMS/gwakeonlan-0.6-3.fc19.src.rpm
Description: gWakeOnLan is a GTK+ utility to awake turned off machines using
the Wake On LAN feature.
It allows to turn on machines in the local network or through
Internet using a destination host and a specified UDP port number.
The machines to turn on need to be shut off with the Wake on LAN
magic packet enabled.

Fedora Account System Username: bebosudo

I would like to say I don't already have any package in fedora repos, so I'm looking for a sponsor to review my package.
I also packaged gtkradiant (#836840) and requested a review in bugzilla, but then I moved it to rpmfusion (#2909), due to legal issues, and I'm already waiting for a review there.

Here is the output from rpmlint:
~$ rpmlint -i gwakeonlan.spec ../SRPMS/gwakeonlan-0.6-*fc19.src.rpm ../RPMS/noarch/gwakeonlan-0.6-*.fc19.noarch.rpm
2 packages and 1 specfiles checked; 0 errors, 0 warnings.
~$

Maybe there are some dependencies missing, I already have to test it in a mock session.

Greetings.

Comment 1 Antonio T. (sagitter) 2014-04-29 17:14:10 UTC
Your URLs seem down.

Comment 2 Volker Fröhlich 2014-05-01 10:06:41 UTC
They work, but fedora-review doesn't like them, due to the missing protocol, I suppose.

Spec URL: http://rpm.bebosudo.tk/SPECS/gwakeonlan.spec
SRPM URL: http://rpm.bebosudo.tk/SRPMS/gwakeonlan-0.6-3.fc19.src.rpm

Comment 3 Alberto Chiusole 2014-05-01 10:26:51 UTC
Do you mean the magic packet required by WOL?

Here there are the requirements:
 http://www.muflone.com/gwakeonlan/english/wol_requirements.html

Comment 4 Volker Fröhlich 2014-05-01 10:30:14 UTC
The build fails due to the xdg module missing.

You can remove " -n %{name}-%{version}", because that's the default.

The changelog must hold your real name. You may obfuscate your e-mail address in a human-readable way, but what you've got now I don't know how to decipher.

Icons placed in the hicolor dir require scriplets: http://fedoraproject.org/wiki/Packaging:ScriptletSnippets#Icon_Cache

Comment 5 Volker Fröhlich 2014-05-01 10:31:50 UTC
No, "http" was missing in your URLs.

Comment 6 Alberto Chiusole 2014-05-01 13:55:36 UTC
Ok, thanks for your tips.
I'll work on it.

Comment 7 nicolas.vieville 2014-10-08 22:48:14 UTC
Hello,

As a candidate packager for Fedora, and if you still want to produce this package, I could make an unofficial review for your proposed package.

If needed, I can provide a working spec file for python3 (builds and packages but for the moment untested application).

Let me know what you wonder on that question.

Cordially,


-- 
NVieville

Comment 8 Muflone 2014-10-19 12:18:06 UTC
Hi

The software gWakeOnLAN was written around Python 2.x and uses some pieces of code not compatible with Python 3:
- print instruction instead of print function
- module ConfigParser was renamed in configparser
- xrange was renamed in range (and range was removed)
- method sendto won't allow the use of strings

Apart the external GTK dependencies, some patches are needed in order to allow this software to run with Python 3.x

Muflone

Comment 9 Muflone 2014-10-19 12:24:43 UTC
@Alberto Chiusole

I've no great experience in Fedora packaging so I'm not entirely sure about this but the spec file contains:
Requires:	pygtk2
which seems to me a requirements for GTK+ 2.x bindings for Python.

Since the version 0.6, gWakeOnLAN was rewritten to use the GTK+ 3 libraries, instead of the GTK+ 2. As you can see in the README [1] file some runtime libraries are required (xdg, GTK+3 gobject for Python).

[1] https://github.com/muflone/gwakeonlan/blob/master/README.md

Comment 10 Alberto Chiusole 2014-10-19 19:14:59 UTC
Hi all,
sorry for the absence, I've completely forgotten to review the package after Volker's advice.
I've privately talked with Nicolas and I noted down some other recommendations, thank you.

I think it's pretty useless to re-write all the code in a way compatible with python3, because the two versions will go ahead living for many years IMHO.
And thanks for your direct intervention in the discussion, muflone. I will check those "requires" too (I think I've installed the right deps in my system, so the rpm builder hasn't complained).


Now I'm under exams so I've rather no time to spend in fedora or in other fields.
Around the first week of november I should have the time to work on it.

I'm sorry for the drawback and the lack of time to spend on it,
Alberto

Comment 11 nicolas.vieville 2014-10-20 10:21:32 UTC
Hello,

First, thank you to Muflone for his intervention and explanation about the proposition I made about making this application Python 3 compatible. When you want to make it compatible with Python 3, as I said in my first message, you know that all the dependencies will be already present in Fedora, and that it will be easy to build a package.

I'll make an unofficial review of your package.

In order to make it easy, some modifications are needed in the spec file you provided, before I make the complete unofficial review.

Please could you replace all the combined space/tabulation indentation by choosing one of this possibility: only tabulation indentation or only space indentation (I usually prefer the last one but it's only a matter of personal choice).

The description section can use line of 79 characters long (https://fedoraproject.org/wiki/Common_Rpmlint_issues#description-line-too-long). Yours is less than that, but as the previous state, it's your own personal choice.

Please upgrade the version to the last one available on the upstream web site (actually 0.6.2) and update the Version and Release field and the changelog section accordingly.

The Requires and BuildRequires should be set as follow:

BuildRequires:  python2-devel python-setuptools pyxdg
BuildRequires:  desktop-file-utils gettext
Requires:       pygobject3
Requires:       hicolor-icon-theme

The python-setuptools package is generally needed in order to help your package to be build.
The pyxdg package is a dependency of the build process of your package.
The desktop-file-utils gettext packages are needed in the process of building and installing the software provided by your package. You already set gettext, but desktop-file-utils will help you in installing or removing the .desktop file (see below comment about install section).

The pygobject3 package contains the equivalent of pygtk2 for GTK3 and will ask for all its dependencies.

The hicolor-icon-theme is needed in order to fix correctly the owner of the icons/hicolor directory in the post* sections (see below).

As the macro %oname will be used only in the description section (see below about Python eggs), this one can be removed, and replaced by "gWakeOnLan" in the description.

In the prep section, as the expression "%setup -q -n %{name}-%{version}" is already the default, you can just make it: "%setup -q"

In the install section, please use the macro %{buildroot} instead of $RPM_BUILD_ROOT variable (one of them should be used in all your spec file in order to avoid using both of them).

In this section, in order to correctly setup the unmodified .desktop file you should use this line:

desktop-file-validate %{buildroot}/%{_datadir}/applications/%{name}.desktop

If you want to modify this file, you should use the desktop-file-install tool. See https://fedoraproject.org/wiki/Packaging:Guidelines#Desktop_files for complete explanations about this.

As your package provides a complete set of icons you should add, always in the install section, the line described here:
https://fedoraproject.org/wiki/Packaging:ScriptletSnippets#Icon_Cache
in order to keep the icon cache up to date while installing/removing your package. I usually add the -f commutator to these commands in order to force operations. The lines needed look like that:

%post
# Update icon cache :
/bin/touch --no-create %{_datadir}/icons/hicolor &>/dev/null || :

%postun
# Update icon cache :
if [ $1 -eq 0 ] ; then
    /bin/touch --no-create %{_datadir}/icons/hicolor &>/dev/null
    /usr/bin/gtk-update-icon-cache -f %{_datadir}/icons/hicolor &>/dev/null || :
fi

%posttrans
# Update icon cache :
/usr/bin/gtk-update-icon-cache -f %{_datadir}/icons/hicolor &>/dev/null || :


In the files section, %{python_sitelib} is deprecated in favor of  %{python2_sitelib}.
As the python-setuptools does is job correctly, there seems that your package doesn't need to take special precaution of the "egg" part of it (to be checked after installation before validation). You can remove the two lines beginning with  %{python_sitelib} and replace them with this one: %{python2_sitelib}/*

So you can remove the definition and the use of the oname2 global variable from the entire spec file.

In this same section, as the package only embed one .desktop file, you can simplify the line:

%{_datadir}/applications/%{name}.desktop

by:

%{_datadir}/applications/*

In the changelog section, please use no special characters unreadable by some editors (in the case of this package the dot and the at characters). 
As it is stated in the changelogs section of the documentation (https://fedoraproject.org/wiki/Packaging:Guidelines#Changelogs), " If you wish to "scramble" or "obfuscate" your email address in the changelog, you may do so, provided that it is still understandable by humans. ". I would add, provided that it is still printable by an editor in order to be readable by humans :) .

To finish with this section, I usually prefer to add a minus ("-") between the version/release number of the package and the beginning of the line. Looks like that for example:

* Mon Jan 27 2014 bebo_sudo <your_printable_readable_email_address> - 0.6.2-1
- initial release from an existing package


As I'm candidate packager for Fedora, and as my skill in such things is not as high as proven packagers, any comments about this unofficial review are welcome.

I noticed that actually your time is dedicated to your exams (wish you to succeed), and knows that you will respond when you've got the time.

Feel free to make any comment you want and don't hesitate to ask for clarifications or explanations if needed.

Cordially,


-- 
NVieville

Comment 12 Alberto Chiusole 2014-11-01 22:28:03 UTC
Hi!
Thanks Nicolas for your whole review.

I've followed all your tips and I've uploaded the new version of SRPM, along with the new package (0.6.2). Here are the:
* SPEC: http://rpm.bebosudo.tk/SPECS/gwakeonlan.spec
* SRPM: http://rpm.bebosudo.tk/SRPMS/gwakeonlan-0.6.2-1.fc20.src.rpm


I've used tabs because I personally prefer those, and I followed all your other tips. thanks again.

rpmlint doesn't complaint about nothing, so I think we're doing quite good.
$ rpmlint -i gwakeonlan.spec ../SRPMS/gwakeonlan-0.6-*fc*.src.rpm ../RPMS/noarch/gwakeonlan-0.6-*.fc*.noarch.rpm
2 packages and 1 specfiles checked; 0 errors, 0 warnings.
$

What do we have to do now? There is the need of a proven packager, am I right?


Have a nice day!
Regards,
Alberto

Comment 13 nicolas.vieville 2014-11-03 20:55:41 UTC
(In reply to Alberto Chiusole from comment #12)

Hello,

> I've followed all your tips and I've uploaded the new version of SRPM, along
> with the new package (0.6.2). Here are the:
> * SPEC: http://rpm.bebosudo.tk/SPECS/gwakeonlan.spec
> * SRPM: http://rpm.bebosudo.tk/SRPMS/gwakeonlan-0.6.2-1.fc20.src.rpm

Thank you for these new files. Sorry for the delay.

> I've used tabs because I personally prefer those, and I followed all your
> other tips. thanks again.

Yes it's a matter of taste. Personally, I use spaces because I know that formatted text will be respected independently from the tabulation width settings in editors. But Fedora packaging rules let packagers do what they want on this point, so that's perfect.

> rpmlint doesn't complaint about nothing, so I think we're doing quite good.
> $ rpmlint -i gwakeonlan.spec ../SRPMS/gwakeonlan-0.6-*fc*.src.rpm
> ../RPMS/noarch/gwakeonlan-0.6-*.fc*.noarch.rpm
> 2 packages and 1 specfiles checked; 0 errors, 0 warnings.
> $

Strange, for my part I've got:

rpmlint gwakeonlan-0.6.2-1.fc20.src.rpm 
gwakeonlan.src: W: strange-permission gwakeonlan.spec 0777L
1 packages and 0 specfiles checked; 0 errors, 1 warnings.

Please could you modify gwakeonlan.spec file permissions to 0644?

I'll post the commented fedora-review result once you fix this little problem.

> What do we have to do now? There is the need of a proven packager, am I
> right?

Yes, you are right. And maybe a sponsor too :)

> Have a nice day!

Thank you. You too.

Regards,


-- 
NVieville

Comment 14 Alberto Chiusole 2014-11-03 22:21:10 UTC
Strange permissions I've set.
The links are the same as before (I didn't change the release).
Here are the:
* SPEC: http://rpm.bebosudo.tk/SPECS/gwakeonlan.spec
* SRPM: http://rpm.bebosudo.tk/SRPMS/gwakeonlan-0.6.2-1.fc20.src.rpm

(In the txt version on the browser the tabs before the buildrequires haven't been formatted well.. maybe on the next release I will change all tabs to spaces)


Hope it's good enough now ;)

Alberto

Comment 15 nicolas.vieville 2014-11-04 23:59:51 UTC
Hello,

Here is the commented fedora-review report for an unofficial review. Looks good to me, but must be approved by a proven Fedora packager.

Special notes about some points detailed below in the fedora-review report (see inserted notes in the report):
- Create-translations.sh file license isn't defined.
- Package naming is probably in conformance with Fedora python package naming 
  guidelines regarding its upstream name.
- Description and summary sections of spec file translations.
- %check section not present - tests not provided upstream.


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

Legend:
[x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated
[ ] = Manual review needed



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

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.
     Note: Checking patched sources after %prep for licenses. Licenses found:
     "GPL (v2 or later)", "Unknown or generated". 1 files have unknown
     license: create-translations.sh
     Note:
     This shell script seems to be a tool helpful for building translations 
     files, but unused and unuseful in building this package with python tools.
[x]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[-]: Development files must be in a -devel package
[x]: Package uses nothing in %doc for runtime.
[x]: The spec file handles locales properly.
[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]: gtk-update-icon-cache is invoked in %postun and %posttrans if package
     contains icons.
     Note: icons in gwakeonlan
[x]: Package is not known to require an ExcludeArch tag.
[x]: Large documentation must go in a -doc subpackage. Large could be size
     (~1MB) or number of files.
     Note: Documentation size is 40960 bytes in 7 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: No rpmlint messages.
[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 %doc.
[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]: All build dependencies are listed in BuildRequires, except for any that
     are listed in the exceptions section of Packaging Guidelines.
[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]: Package contains desktop file if it is a GUI application.
[x]: Package installs a %{name}.desktop using desktop-file-install or desktop-
     file-validate if there is such a file.
[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 do not use a name that already exist
[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

Python:
[x]: Python eggs must not download any dependencies during the build process.
[-]: A package which is used by another package via an egg interface should
     provide egg info.
[x]: Package meets the Packaging Guidelines::Python
     Note about naming convention: as the upstream software name is 
     "gwakeonlan" and doesn't mention in any form python, this is acceptable 
     (probably needs for my part acknowledgement from a proven python packager) 
[x]: Package contains BR: python2-devel or python3-devel
[x]: Binary eggs must be removed in %prep

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

Generic:
[x]: 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]: Package functions as described.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[x]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
     Note: we should probably try to provide translations for our natural 
     languages (it and fr).
[x]: Package should compile and build into binary rpms on all supported
     architectures.
[-]: %check is present and all tests pass.
     Note: tests are not provided upstream, so %check is not present.
[x]: Packages should try to preserve timestamps of original installed files.
[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: Sources can be downloaded from URI in Source: tag
[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]: Dist tag is present (not strictly required in GL).
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: SourceX is a working URL.
[x]: Spec use %global instead of %define unless justified.

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

Generic:
[x]: Rpmlint is run on all installed packages.
     Note: No rpmlint messages.
[x]: Spec file according to URL is the same as in SRPM.


Rpmlint
-------
Checking: gwakeonlan-0.6.2-1.fc22.noarch.rpm
          gwakeonlan-0.6.2-1.fc22.src.rpm
2 packages and 0 specfiles checked; 0 errors, 0 warnings.


Rpmlint (installed packages)
----------------------------
# rpmlint gwakeonlan
1 packages and 0 specfiles checked; 0 errors, 0 warnings.
# echo 'rpmlint-done:'


Requires
--------
gwakeonlan (rpmlib, GLIBC filtered):
    /bin/sh
    /usr/bin/python2
    hicolor-icon-theme
    pygobject3
    python(abi)


Provides
--------
gwakeonlan:
    application()
    application(gwakeonlan.desktop)
    gwakeonlan


Source checksums
----------------
http://www.muflone.com/resources/gwakeonlan/archive/0.6.2/gwakeonlan-0.6.2.tar.gz :
  CHECKSUM(SHA256) this package     : fa6a88351f5a437ca2171c012352f01a3cab147a09ac6c6b3278980f14eeb862
  CHECKSUM(SHA256) upstream package : fa6a88351f5a437ca2171c012352f01a3cab147a09ac6c6b3278980f14eeb862


Generated by fedora-review 0.5.2 (63c24cb) last change: 2014-07-14
Command line :/usr/bin/fedora-review -m fedora-rawhide-x86_64 -b 1060419
Buildroot used: fedora-rawhide-x86_64
Active plugins: Python, Generic, Shell-api
Disabled plugins: Java, C/C++, fonts, SugarActivity, Ocaml, Perl, Haskell, R, PHP, Ruby
Disabled flags: EXARCH, EPEL5, BATCH, DISTTAG


For my part, I think this package is on the good way to be accepted. Hope this unofficial review will help in that.
Feel free to make any comment you think necessary about this unofficial review.

Regards,


-- 
NVieville

Comment 16 Alberto Chiusole 2014-11-17 19:50:22 UTC
Ok, thank you Nicolas for your review.

How we should move along now? I have to look for an already approved packager that can submit a final review?


Thanks again,
Alberto

Comment 17 nicolas.vieville 2014-11-19 12:56:57 UTC
(In reply to Alberto Chiusole from comment #16)
Hello Alberto,

> How we should move along now? I have to look for an already approved
> packager that can submit a final review?

Exactly. As you already blocked FE-NEEDSPONSOR bug report, all the necessary is done. I suspect that proven packagers are a little bit busy actually because of the next coming F-21.
I'm in the same case too.
 
> Thanks again,

You're welcome.

Cordially,


-- 
NVieville

Comment 18 Volker Fröhlich 2015-04-21 15:23:19 UTC
Did you find a sponsor yet?

Comment 19 Alberto Chiusole 2015-04-21 18:54:37 UTC
Nope.

@Volker
Could you help me?

--
Alberto

Comment 20 Volker Fröhlich 2015-04-21 21:50:01 UTC
Did you take part in other reviews? If so, can you point me to them? What's your FAS name? Are you on IRC?

Comment 21 Alberto Chiusole 2015-04-23 13:52:20 UTC
thank you for the interest.

I've not already taken part in other reviews. Do you have some packages pending too? I could revise your's or someone else's packages, for what I'm able to do.
Do you have any suggestion?

--
Alberto

Comment 22 Volker Fröhlich 2016-02-16 00:01:04 UTC
Feel free to re-open this!


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