https://bugzilla.redhat.com/show_bug.cgi?id=1317939
Bug ID: 1317939 Summary: Review Request: dump1090 - Decode ADS-B messages from RTL-SDR Product: Fedora Version: rawhide Component: Package Review Severity: medium Assignee: nobody@fedoraproject.org Reporter: benjiwiebe14@gmail.com QA Contact: extras-qa@fedoraproject.org CC: package-review@lists.fedoraproject.org
Spec URL: https://www.benjiwiebe.com/packages/dump1090.spec SRPM URL: https://www.benjiwiebe.com/packages/dump1090-85aa200-1.fc23.src.rpm Description: Program to decode ADS-B messages from an RTL-SDR Fedora Account System Username: BenjiWiebe
This is another dependency of piaware, which I will submit for review soon. A successful Koji scratch build: http://koji.fedoraproject.org/koji/taskinfo?taskID=13354578
https://bugzilla.redhat.com/show_bug.cgi?id=1317939
Benji Wiebe benjiwiebe14@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks| |177841 (FE-NEEDSPONSOR)
Referenced Bugs:
https://bugzilla.redhat.com/show_bug.cgi?id=177841 [Bug 177841] Tracker: Review requests from new Fedora packagers who need a sponsor
https://bugzilla.redhat.com/show_bug.cgi?id=1317939
Zbigniew Jędrzejewski-Szmek zbyszek@in.waw.pl changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |zbyszek@in.waw.pl
--- Comment #1 from Zbigniew Jędrzejewski-Szmek zbyszek@in.waw.pl --- Too many acronyms. Please spell out what ADS-B messages are and what RTL-SDR is, and what this package is good for (in the Description).
You cannot use the git commit directly as versions, because git commits are "random" and versions must always 'grow". See https://fedoraproject.org/wiki/Packaging:Guidelines#Version_and_Release.
Please put BuildRequires and similar each in a single line (for readability and git-diffibility).
make %{?_smp_mflags} → %make_build
If Patch1 simply adds a service file, you can just include the service file as %{SOURCE1} and install it directly. It's easier to inspect and maintain this way.
systemd-units is long gone, please just use %systemd_requires. Also see https://fedoraproject.org/wiki/Packaging:Scriptlets#Systemd for scriptlets that need to be added.
%license should be used, see https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#License_Text.
Who owns %{_datarootdir}/%{name}/ and %{_datarootdir}/%{name}/public_html ?
https://bugzilla.redhat.com/show_bug.cgi?id=1317939
--- Comment #2 from Benji Wiebe benjiwiebe14@gmail.com --- Thanks for taking the time to look at this...
Description: See if you like the new description better. The acronyms are what most users would recognize, so I left them in, but added some more words to make it plainer for those who aren't familiar with it.
Version: Using the format YYYYMMDDgitSHORTCOMMIT now.
BuildRequires/Requires are now split out onto separate lines. What you said about diffability makes good sense.
I wasn't aware of %make_build, fixed.
Patch1 and Patch2 are both add-one-file patches, so I moved both to Source1 and Source2 respectively.
For Systemd, I'm not sure I've got it right yet. I had been going off https://fedoraproject.org/wiki/Packaging:Systemd#Packaging but apparently that is out-of-date information?
Switched to %license for the COPYING file.
The default ownership of %{_datarootdir}/%{name} and %{_datarootdir}/%{name}/public_html looks correct to me. It show be owned by root:root and everyone should have read permissions but not write permissions.
Also I removed the two Requires:, as they were unnecessary. Good ol' rpmlint.
https://bugzilla.redhat.com/show_bug.cgi?id=1317939
--- Comment #3 from Zbigniew Jędrzejewski-Szmek zbyszek@in.waw.pl --- (In reply to Benji Wiebe from comment #2)
Thanks for taking the time to look at this...
Description: See if you like the new description better. The acronyms are what most users would recognize, so I left them in, but added some more words to make it plainer for those who aren't familiar with it.
The Summary is great: concise yet understandable (for somebody like me who has no idea what this package does).
But %description should be a bit longer: what can you do with this package, what is the output, is special hardware required, etc. Doesn't have to be exhaustive, one paragraph is enough.
Consider that users who have no idea are sometimes looking for something in a list of hundreds and hundreds packages and you cannot assume that they have any area knowledge (the description is not only to let people use your package, quite often it is to let people know that your package is not useful for them).
Version: Using the format YYYYMMDDgitSHORTCOMMIT now.
Yep, that looks good.
BuildRequires/Requires are now split out onto separate lines. What you said about diffability makes good sense.
During review it is customary to bump the revision after major changes, add stuff to %changelog, and upload the SRPM under a new name. The spec file is updated in place.
This way it's possible for the review to go back and look at the previous version.
For Systemd, I'm not sure I've got it right yet. I had been going off https://fedoraproject.org/wiki/Packaging:Systemd#Packaging but apparently that is out-of-date information?
I don't think it's out-of-date, just slightly dated ;). There are parts which talk about old Fedora, but it correctly mentions what applies to newer versions.
The default ownership of %{_datarootdir}/%{name} and %{_datarootdir}/%{name}/public_html looks correct to me. It show be owned by root:root and everyone should have read permissions but not write permissions.
I meant rpm package ownership. Your package creates those directories so it should own them.
Also I removed the two Requires:, as they were unnecessary. Good ol' rpmlint.
Hm, I don't remember what those were. That's why it's helpful to keep old srpms accessible.
https://bugzilla.redhat.com/show_bug.cgi?id=1317939
--- Comment #4 from Benji Wiebe benjiwiebe14@gmail.com --- (In reply to Zbigniew Jędrzejewski-Szmek from comment #3)
But %description should be a bit longer: what can you do with this package, what is the output, is special hardware required, etc. Doesn't have to be exhaustive, one paragraph is enough.
It is a *bit* longer...I hope that's good enough.
During review it is customary to bump the revision after major changes, add stuff to %changelog, and upload the SRPM under a new name. The spec file is updated in place.
OK, I'll do that from now on.
I meant rpm package ownership. Your package creates those directories so it should own them.
Aha, I caught on about RPM ownership. I uninstalled dump1090 on my PC, and the directories are still there. I fixed that now.
Hm, I don't remember what those were. That's why it's helpful to keep old srpms accessible.
They were rtl-sdr and libusb. Both are picked up automatically by RPM.
https://bugzilla.redhat.com/show_bug.cgi?id=1317939
Zbigniew Jędrzejewski-Szmek zbyszek@in.waw.pl changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED Blocks|177841 (FE-NEEDSPONSOR) | Assignee|nobody@fedoraproject.org |zbyszek@in.waw.pl Flags| |fedora-review?
--- Comment #5 from Zbigniew Jędrzejewski-Szmek zbyszek@in.waw.pl --- Permissions will be wrong on the unit file, you should add -m644. (/usr/bin/install is stupid).
Also, if you use -D, you can get rid of the separate mkdir... It's often a bit nicer this way.
Use 'cp -a', to preserve timestamps on files.
'make faup1090' → %make_build faup1090 to have the -j flag.
%description is much better. But maybe you could add another sentence like "It provides a daemon that serves ...".
--
I can sponsor you into the packagers group.
Please do two or three reviews of packages from http://fedoraproject.org/PackageReviewStatus/NEW.html, and paste the links here. Please pick packages that are in the area you are interested in, and that other people haven't picked up, so that you can finalize the review after you get the packager privs. Also don't pick things that are overly complicated, we don't want to get bogged down in details.
Running fedora-review is a good first step, but please note that the automatically generated template needs to be filled in in various places, and trimmed in others. Not everything the tools say is always correct. Sometimes they are outdated, sometimes they are plain wrong. It's always best to link to the relevant part of the guidelines. If you have any questions or issues, I'd always be happy to help (zbyszek at in waw pl, zbyszek on #fedora-devel).
Also, you should update your bugzilla mail to match your FAS info, or the other way around.
Referenced Bugs:
https://bugzilla.redhat.com/show_bug.cgi?id=177841 [Bug 177841] Tracker: Review requests from new Fedora packagers who need a sponsor
https://bugzilla.redhat.com/show_bug.cgi?id=1317939
--- Comment #6 from Benji Wiebe benjiwiebe@fedoraproject.org --- Corrected permissions on unit file.
Using install -D now.
Using cp -a now.
Switched to %make_build faup1090.
I reworded the first few words of the Description. Was: dump1090 is software that can utilize an RTL-SDR dongle... Is: dump1090 is a daemon that utilizes an RTL-SDR dongle...
Is that better?
As for the sponsoring, thanks for being willing to help me along. And I updated my bugzilla email address.
https://bugzilla.redhat.com/show_bug.cgi?id=1317939
Zbigniew Jędrzejewski-Szmek zbyszek@in.waw.pl changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags|fedora-review? |fedora-review+
--- Comment #7 from Zbigniew Jędrzejewski-Szmek zbyszek@in.waw.pl --- + license is acceptable (GPLv2+) + license file is present, %license is used + latest version (git snapshot) + provides/requires look OK + scriptlets looks sane
- please add empty lines between changelog entries (Release-engineering scripts and other automatic tools will add entries to the changelog with an empty line, and things would look inconsistent. It's also more readable that way.)
- please post the srpm and add new "Spec URL, SRPM URL" lines. In general during review you'd do that every time. (I'd have to unpack your original src rpm, overwrite the spec file, repack, and then build. Too much work :))
- with the description: I was looking for a sentence like "The daemon listens on a configurable address (default localhost:8080) and serves a page with continously updated list of beacons". (Or something, I didn't actually run it).
https://bugzilla.redhat.com/show_bug.cgi?id=1317939
Zbigniew Jędrzejewski-Szmek zbyszek@in.waw.pl changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags|fedora-review+ |fedora-review?
https://bugzilla.redhat.com/show_bug.cgi?id=1317939
--- Comment #8 from Benji Wiebe benjiwiebe@fedoraproject.org --- OK, I've improved the description, and added empty lines between changelog entries. And here are the latest and greatest links:
SRPM: https://www.benjiwiebe.com/packages/dump1090-20160303git85aa200-4.fc23.src.r...
SPEC: https://www.benjiwiebe.com/packages/dump1090.spec
https://bugzilla.redhat.com/show_bug.cgi?id=1317939
--- Comment #9 from Zbigniew Jędrzejewski-Szmek zbyszek@in.waw.pl --- dump1090.i686: W: incoherent-version-in-changelog 20160303git85a200-4 ['20160303git85aa200-4.fc25', '20160303git85aa200-4']
I think it's question of the number of digits in %shortcommit vs that in the %changelog.
dump1090.i686: W: non-conffile-in-etc /etc/sysconfig/dump1090 You should probably add %config(noreplace) for that file in %files.
README says that the daemon does not have to run as root. It would be great to make it so by default. 1. pick a user name 2. create the user in %pre:
getent group NAME >/dev/null 2>&1 || groupadd -r NAME 2>&1 || : getent passwd NAME >/dev/null 2>&1 || useradd -r -l -g NAME -s /sbin/nologin -c "..." NAME >/dev/null 2>&1 || :
3. add User=NAME in the .service file
4. There might be additional steps necessary for the user to have privileges to access the hardware. If it is enough to add it to some group, that can be done with "-G group" in step 2.
/etc should be written as %{_sysconfdir} in the spec file. I don't think this makes much sense, but the guidelines require that.
I can test that the daemon starts OK, but I don't have suitable hardware, so I can't test it.
https://bugzilla.redhat.com/show_bug.cgi?id=1317939
Benji Wiebe benjiwiebe@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks| |1321424
Referenced Bugs:
https://bugzilla.redhat.com/show_bug.cgi?id=1321424 [Bug 1321424] root:root ownership of USB device prevents access by non-root daemons
https://bugzilla.redhat.com/show_bug.cgi?id=1317939
Benji Wiebe benjiwiebe@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks|1321424 | Depends On| |1321424
Referenced Bugs:
https://bugzilla.redhat.com/show_bug.cgi?id=1321424 [Bug 1321424] root:root ownership of USB device prevents access by non-root daemons
https://bugzilla.redhat.com/show_bug.cgi?id=1317939
--- Comment #10 from Zbigniew Jędrzejewski-Szmek zbyszek@in.waw.pl --- Can you make a new version which uses the new rtlsdr group?
https://bugzilla.redhat.com/show_bug.cgi?id=1317939 Bug 1317939 depends on bug 1321424, which changed state.
Bug 1321424 Summary: root:root ownership of USB device prevents access by non-root daemons https://bugzilla.redhat.com/show_bug.cgi?id=1321424
What |Removed |Added ---------------------------------------------------------------------------- Status|ON_QA |CLOSED Resolution|--- |ERRATA
package-review@lists.fedoraproject.org
-
Red Hat Bugzilla