Bug 841418 - Review Request: prey - Open-source anti-theft solution
Review Request: prey - Open-source anti-theft solution
Status: ASSIGNED
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Peter Lemenkov
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-18 18:59 EDT by Patrick Uiterwijk
Modified: 2013-11-21 10:20 EST (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed:
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
lemenkov: fedora‑review+


Attachments (Terms of Use)

  None (edit)
Description Patrick Uiterwijk 2012-07-18 18:59:04 EDT
Spec URL: http://puiterwijk.fedorapeople.org/packages/prey/prey-1.spec
SRPM URL: http://puiterwijk.fedorapeople.org/packages/prey/prey-0.5.3-1.fc17.src.rpm
Koji URL: http://koji.fedoraproject.org/koji/taskinfo?taskID=4253552
Fedora Account System Username: puiterwijk
Description: 
Prey lets you keep track of your laptop, phone and tablet
whenever stolen or missing -- easily and all in one place.
It's lightweight, open source software that gives you full
and remote control, 24/7.
Comment 1 Gregor Tätzner 2012-07-21 16:06:30 EDT
- drop BuildRoot and defattr (below %files)
- the use of %attr seems a little bit over the top. is it really necessary to specify all those?
- make sure to use cp -a whenever possible
- rename spec file to prey.spec
Comment 2 Patrick Uiterwijk 2012-07-23 18:53:43 EDT
- The BuildRoot is because I want to support el5. I removed defattr
- The use of %attr is because the source archive has incorrect permissions, and a too broad %attr (for the while dir) is not possible either, because the version "scripts" cannot have executable permissions
- cp -a fixed
- I put it as prey.spec

new Spec URL: http://puiterwijk.fedorapeople.org/packages/prey/prey.spec
new SRPM URL: http://puiterwijk.fedorapeople.org/packages/prey/prey-0.5.3-2.fc17.src.rpm
new Koji URL: http://koji.fedoraproject.org/koji/taskinfo?taskID=4324586
Comment 3 Peter Lemenkov 2012-07-26 08:06:27 EDT
I'll review it.
Comment 4 Peter Lemenkov 2012-07-26 08:23:42 EDT
REVIEW:

Legend: + = PASSED, - = FAILED, 0 = Not Applicable

+ rpmlint is silent

work ~/Desktop: rpmlint prey-0.5.3-2.fc18.noarch.rpm prey-0.5.3-2.fc18.src.rpm
2 packages and 0 specfiles checked; 0 errors, 0 warnings.
work ~/Desktop: 

+ The package is named according to the  Package Naming Guidelines.
+ The spec file name matches the base package %{name}, in the format %{name}.spec.
+ The package meets the Packaging Guidelines.
+ The package is licensed with a Fedora approved license and meets the Licensing Guidelines.
+ The License field in the package spec file matches the actual license (strict GPLv3).
+ The file, containing the text of the license(s) for the package, is included in %doc.
+ The spec file is written in American English.
+ The spec file for the package is legible.
+ The sources used to build the package, match the upstream source, as provided in the spec URL.

work ~/Desktop: sha256sum prey-0.5.3-linux.zip*
fdeed1987d30b4ca585e19ac9884194bc8a108bd56112d2913206fc85be3bd9b  prey-0.5.3-linux.zip
fdeed1987d30b4ca585e19ac9884194bc8a108bd56112d2913206fc85be3bd9b  prey-0.5.3-linux.zip.1
work ~/Desktop: 

+ The package successfully compiles and builds into binary rpms on at least one primary architecture.
+ All build dependencies are listed in BuildRequires.
0 No need to handle locales.
0 No shared library files.
+ The package does NOT bundle copies of system libraries.
+ The package is not designed to be relocatable.
+ The package owns all directories that it creates.
+ The package does not list a file more than once in the spec file's %files listings.
+ Permissions on files are set properly.

-/0 The package DOESN'T have a %clean section, which contains rm -rf %{buildroot} (or $RPM_BUILD_ROOT). That's a problem if you plan to build for EL5.

+ The package consistently uses macros.
+ The package contains code, or permissible content.
0 No extremely large documentation files.
+ Anything, the package includes as %doc, does not affect the runtime of the application.
0 No header files.
0 No static libraries.
0 No pkgconfig(.pc) files.
0 The package doesn't contain library files with a suffix (e.g. libfoo.so.1.1).
0 No devel sub-package.
+ The package does NOT contain any .la libtool archives.
0 Not a GUI application.
+ The package does not own files or directories already owned by other packages.

-/0 At the beginning of %install, the package DOESN'T run rm -rf %{buildroot} (or $RPM_BUILD_ROOT). That's a problem if you plan to build for EL5.

+ All filenames in rpm packages are valid UTF-8.

Please, consider adding %clean section and add rm -rf %{buildroot} if you plan to build for EL5.

I don't see any other issues so this package is 

APPROVED.
Comment 5 Ralf Corsepius 2012-07-26 08:27:46 EDT
(In reply to comment #4)

> I don't see any other issues so this package is 

But I do. This package is _far_, _far_ from being FHS compliant. It installs scripts, config files and OS depended files into ${datadir}.
Comment 6 Paul Wouters 2012-07-26 12:49:56 EDT
I wonder how many false positives you will get behind hotspots. perhaps it should first check fedoraproject.org/static/hotspot.txt ?
Comment 7 Patrick Uiterwijk 2012-07-26 13:48:10 EDT
(In reply to comment #5)
> (In reply to comment #4)
> 
> > I don't see any other issues so this package is 
> 
> But I do. This package is _far_, _far_ from being FHS compliant. It installs
> scripts, config files and OS depended files into ${datadir}.
I will look into the locations, but the config file which should override all others is placed as /etc/sysconfig/prey, and I wonder which OS dependant files you mean, as it is just a bunch of bash scripts?

(In reply to comment #6)
> I wonder how many false positives you will get behind hotspots. perhaps it 
> should first check fedoraproject.org/static/hotspot.txt ?
It is not activated by IP, but by the user registering the device as stolen on the control panel, or removing a file on a web server.
Comment 8 Patrick Uiterwijk 2012-07-26 19:48:25 EDT
Thanks for all the comments, I am sorry if my answers were harsh and non-welcoming.


I have looked into the locations, and after re-reading the FHS, I have placed the scripts in libexecdir/prey, and all of the config files in /etc/sysconfig/prey.

I also added the check for hotspot.txt.


new Spec URL: http://puiterwijk.fedorapeople.org/packages/prey/prey.spec
new SRPM URL: http://puiterwijk.fedorapeople.org/packages/prey/prey-0.5.3-3.fc17.src.rpm
new Koji URL: http://koji.fedoraproject.org/koji/taskinfo?taskID=4332846
Comment 9 Ralf Corsepius 2012-07-27 00:10:42 EDT
(In reply to comment #7)
> and I wonder which OS dependant
> files you mean, as it is just a bunch of bash scripts?

The FHS defines datadir as "read-only architecture-independent data"

Note the "data" and "architecture-independent":

=> These files are arguably "non-arch-independent":
%attr(755, -, -) %{_datadir}/%{name}/platform/linux/functions
%attr(644, -, -) %{_datadir}/%{name}/platform/linux/prey-config.glade
%attr(755, -, -) %{_datadir}/%{name}/platform/linux/prey-config.py*
%attr(755, -, -) %{_datadir}/%{name}/platform/linux/settings
%attr(755, -, -) %{_datadir}/%{name}/platform/linux/bin/sendEmail

(The idea behind datadir (/usr/share) is them begin sharable between different OSes/architectures, e.g. through nfs mounts)


=> Theses files are executable:
%attr(755, -, -) %{_datadir}/%{name}/platform/linux/functions
%attr(755, -, -) %{_datadir}/%{name}/platform/linux/prey-config.py*
%attr(755, -, -) %{_datadir}/%{name}/platform/linux/settings
%attr(755, -, -) %{_datadir}/%{name}/platform/linux/bin/sendEmail

They aren't data, they are executables and therefore do not belong into %{_datadir} but should be placed into %{_libdir}/%{name} or %{_libexecdir}/%{name}


That said, a simple solution would be to install all these files into %{_libdir}/%{name} instead of %{_datadir}/%{name}
Comment 10 Gregor Tätzner 2012-07-27 05:00:53 EDT
(In reply to comment #9)
> (In reply to comment #7)
> > and I wonder which OS dependant
> > files you mean, as it is just a bunch of bash scripts?
> 
> The FHS defines datadir as "read-only architecture-independent data"
> 
> Note the "data" and "architecture-independent":
> 
> => These files are arguably "non-arch-independent":
> %attr(755, -, -) %{_datadir}/%{name}/platform/linux/functions
> %attr(644, -, -) %{_datadir}/%{name}/platform/linux/prey-config.glade
> %attr(755, -, -) %{_datadir}/%{name}/platform/linux/prey-config.py*
> %attr(755, -, -) %{_datadir}/%{name}/platform/linux/settings
> %attr(755, -, -) %{_datadir}/%{name}/platform/linux/bin/sendEmail

Actually those python, bash and perl scripts are arch independent. They don't must be OS independent.
Comment 11 Patrick Uiterwijk 2012-07-27 07:20:15 EDT
Well, they are arch independant, but I agree they are not "data".
That's why I placed them in libexec, according to the packaging guidelines: "Libexecdir (aka, /usr/libexec on Fedora systems) should only be used as the directory for executable programs that are designed primarily to be run by other programs rather than by users. ", as they are executable, but the user will not execute them (crond will).

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