[Bug 1317939] Review Request: dump1090 - Decode ADS-B messages from RTL-SDR

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1317939

Zbigniew Jędrzejewski-Szmek  changed:

   What|Removed |Added

 Status|ASSIGNED|CLOSED
 Resolution|--- |INSUFFICIENT_DATA
Last Closed||2019-09-17 15:40:42



--- Comment #11 from Zbigniew Jędrzejewski-Szmek  ---
Let's close this. Feel free to restart the process at any time.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org
Fedora Code of Conduct: 
https://docs.fedoraproject.org/en-US/project/code-of-conduct/
List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines
List Archives: 
https://lists.fedoraproject.org/archives/list/package-review@lists.fedoraproject.org

[Bug 1317939] Review Request: dump1090 - Decode ADS-B messages from RTL-SDR

[bugzilla]

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



-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list
package-review@lists.fedoraproject.org
http://lists.fedoraproject.org/admin/lists/package-review@lists.fedoraproject.org

[Bug 1317939] Review Request: dump1090 - Decode ADS-B messages from RTL-SDR

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1317939



--- Comment #10 from Zbigniew Jędrzejewski-Szmek  ---
Can you make a new version which uses the new rtlsdr group?

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review

[Bug 1317939] Review Request: dump1090 - Decode ADS-B messages from RTL-SDR

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1317939

Benji Wiebe  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
-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review

[Bug 1317939] Review Request: dump1090 - Decode ADS-B messages from RTL-SDR

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1317939

Benji Wiebe  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
-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review

[Bug 1317939] Review Request: dump1090 - Decode ADS-B messages from RTL-SDR

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1317939



--- Comment #9 from Zbigniew Jędrzejewski-Szmek  ---
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.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review

[Bug 1317939] Review Request: dump1090 - Decode ADS-B messages from RTL-SDR

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1317939



--- Comment #3 from Zbigniew Jędrzejewski-Szmek  ---
(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 MMDDgitSHORTCOMMIT 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.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review

[Bug 1317939] Review Request: dump1090 - Decode ADS-B messages from RTL-SDR

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1317939



--- Comment #6 from Benji Wiebe  ---
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.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review

[Bug 1317939] Review Request: dump1090 - Decode ADS-B messages from RTL-SDR

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1317939

Zbigniew Jędrzejewski-Szmek  changed:

   What|Removed |Added

 Status|NEW |ASSIGNED
 Blocks|177841 (FE-NEEDSPONSOR) |
   Assignee|nob...@fedoraproject.org|zbys...@in.waw.pl
  Flags||fedora-review?



--- Comment #5 from Zbigniew Jędrzejewski-Szmek  ---
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
-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review

[Bug 1317939] Review Request: dump1090 - Decode ADS-B messages from RTL-SDR

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1317939



--- Comment #4 from Benji Wiebe  ---
(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.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review

[Bug 1317939] Review Request: dump1090 - Decode ADS-B messages from RTL-SDR

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1317939

Zbigniew Jędrzejewski-Szmek  changed:

   What|Removed |Added

  Flags|fedora-review?  |fedora-review+



--- Comment #7 from Zbigniew Jędrzejewski-Szmek  ---
+ 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).

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review

[Bug 1317939] Review Request: dump1090 - Decode ADS-B messages from RTL-SDR

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1317939



--- Comment #2 from Benji Wiebe  ---
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 MMDDgitSHORTCOMMIT 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.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review

[Bug 1317939] Review Request: dump1090 - Decode ADS-B messages from RTL-SDR

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1317939

Zbigniew Jędrzejewski-Szmek  changed:

   What|Removed |Added

  Flags|fedora-review+  |fedora-review?



-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review

[Bug 1317939] Review Request: dump1090 - Decode ADS-B messages from RTL-SDR

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1317939



--- Comment #8 from Benji Wiebe  ---
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.rpm

SPEC: https://www.benjiwiebe.com/packages/dump1090.spec

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review

[Bug 1317939] Review Request: dump1090 - Decode ADS-B messages from RTL-SDR

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1317939

Zbigniew Jędrzejewski-Szmek  changed:

   What|Removed |Added

 CC||zbys...@in.waw.pl



--- Comment #1 from Zbigniew Jędrzejewski-Szmek  ---
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 ?

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review

[Bug 1317939] Review Request: dump1090 - Decode ADS-B messages from RTL-SDR

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1317939

Benji Wiebe  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
-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review