Spec URL: https://gathman.org/linux/SPECS/dump1090.spec SRPM URL: https://gathman.org/linux/f33/src/dump1090-0-1.20210727gitde61bd5.fc33.src.rpm Description: Dump 1090 is a Mode S decoder specifically designed for RTLSDR devices. Install this to use your RTL-SDR to track commercial aircraft in your area. The main features are: * Robust decoding of weak messages, with mode1090 many users observed improved range compared to other popular decoders. * Network support: TCP30003 stream (MSG5...), Raw packets, HTTP. * Embedded HTTP server that displays the currently detected aircraft on Google Map. * Single bit errors correction using the 24 bit CRC. * Ability to decode DF11, DF17 messages. * Ability to decode DF formats like DF0, DF4, DF5, DF16, DF20 and DF21 where the checksum is xored with the ICAO address by brute forcing the checksum field using recently seen ICAO addresses. * Decode raw IQ samples from file (using --ifile command line switch). * Interactive command-line interface mode where aircraft currently detected are shown as a list refreshing as more data arrives. * CPR coordinates decoding and track calculation from velocity. * TCP server streaming and receiving raw data to/from connected clients (using --net). Fedora Account System Username: sdgathman
Any particular reason the fork https://github.com/antirez/dump1090 was chosen as opposed to the fork mentioned in the readme of that repo, namely https://github.com/MalcolmRobb/dump1090?
Nevermind, I see now that despite the comment in the README that I saw, the chosen repo has a lot more recent activity so it makes perfect sense what this one was chosen. I'll start the review.
I see Troy has already taken the review, so please consider this supplemental feedback. Package Review ============== Legend: [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated ===== Issues ===== - The applicable compiler flags are only partially honored. The correct CFLAGS are applied, but the correct LDFLAGS are not. Consider replacing: %make_build CC=gcc CFLAGS="${CFLAGS:-%optflags}" with: %set_build_flags %make_build - The patch should be explained and preferably linked to an upstream bug report or PR. - Please consider asking upstream to add a separate license file so you don’t have to make one with sed. - A man page would be great (https://docs.fedoraproject.org/en-US/packaging-guidelines/#_manpages), although the package can be approved without one. If you’re willing to maintain it downstream, I can write one in groff_man(7) format based on the --help output, either now or as a PR to the package once it is approved. ===== Notes (no change required) ===== - You could simplify referencing source quite a bit with the “forge” macros; see https://docs.fedoraproject.org/en-US/packaging-guidelines/SourceURL/#_commit_example. These can handle adding the snapshot info to the release and generating the correct source URL and extraction directory for you. %global forgeurl https://github.com/antirez/%{name} %global commit de61bd564f1aa929bae414a70e421acd0b81789a Name: dump1090 Version: 0 %forgemeta Release: 1 Summary: Simple Mode S decoder specifically designed for RTLSDR devices License: BSD URL: %{forgeurl} Source0: %{forgesource} … %prep %forgeautosetup ===== MUST items ===== C/C++: [x]: Package does not contain kernel modules. [x]: Package contains no static executables. [x]: If your application is a C or C++ application you must list a BuildRequires against gcc, gcc-c++ or clang. [x]: Header files in -devel subpackage, if present. [x]: Package does not contain any libtool archives (.la) [x]: Rpath absent or only used for internal libs. 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: "BSD (2 clause)", "Unknown or generated", "BSD (3 clause)". 6 files have unknown license. Detailed output of licensecheck in /home/reviewer/1987045-dump1090/licensecheck.txt [x]: License file installed when any subpackage combination is installed. [!]: %build honors applicable compiler flags or justifies otherwise. [x]: Package contains no bundled libraries without FPC exception. [x]: Changelog in prescribed format. [x]: Sources contain only permissible code or content. [-]: Package contains desktop file if it is a GUI application. [-]: Development files must be in a -devel package [x]: Package uses nothing in %doc for runtime. [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]: Useful -debuginfo package or justification otherwise. [x]: Package is not known to require an ExcludeArch tag. [-]: Large documentation must go in a -doc subpackage. Large could be size (~1MB) or number of files. Note: Documentation size is 20480 bytes in 2 files. [x]: Package complies to the Packaging Guidelines (except as noted) [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: There are rpmlint messages (see attachment). [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 %license. [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]: 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]: Dist tag is present. [x]: Package does not contain duplicates in %files. [x]: Permissions on files are set properly. [x]: Package must not depend on deprecated() packages. [x]: Package use %makeinstall only when make install DESTDIR=... doesn't work. [x]: Package is named using only allowed ASCII characters. [x]: Package does not use a name that already exists. [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 ===== SHOULD items ===== Generic: [!]: 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). [?]: Package functions as described. [x]: Latest version is packaged. [x]: Package does not include license text files separate from upstream. I’m going to say what you’re doing with sed is OK. [!]: Patches link to upstream bugs/comments/lists or are otherwise justified. [-]: Sources are verified with gpgverify first in %prep if upstream publishes signatures. Note: gpgverify is not used. [-]: Description and summary sections in the package spec file contains translations for supported Non-English languages, if available. [x]: Package should compile and build into binary rpms on all supported architectures. [x]: %check is present and all tests pass. [x]: Packages should try to preserve timestamps of original installed files. [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]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin. [x]: Fully versioned dependency in subpackages if applicable. [x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file [x]: Sources can be downloaded from URI in Source: tag [x]: SourceX is a working URL. [x]: Spec use %global instead of %define unless justified. ===== EXTRA items ===== Generic: [x]: Rpmlint is run on debuginfo package(s). Note: No rpmlint messages. [x]: Rpmlint is run on all installed packages. Note: There are rpmlint messages (see attachment). [x]: Large data in /usr/share should live in a noarch subpackage if package is arched. [x]: Spec file according to URL is the same as in SRPM. Rpmlint ------- Checking: dump1090-0-1.20210727gitde61bd5.fc35.aarch64.rpm dump1090-debuginfo-0-1.20210727gitde61bd5.fc35.aarch64.rpm dump1090-debugsource-0-1.20210727gitde61bd5.fc35.aarch64.rpm dump1090-0-1.20210727gitde61bd5.fc35.src.rpm dump1090.aarch64: W: spelling-error %description -l en_US checksum -> check sum, check-sum, checks um dump1090.aarch64: W: spelling-error %description -l en_US xored -> cored, gored, pored dump1090.aarch64: W: spelling-error %description -l en_US ifile -> file, i file dump1090.aarch64: W: no-manual-page-for-binary dump1090 dump1090.src: W: spelling-error %description -l en_US checksum -> check sum, check-sum, checks um dump1090.src: W: spelling-error %description -l en_US xored -> cored, gored, pored dump1090.src: W: spelling-error %description -l en_US ifile -> file, i file 4 packages and 0 specfiles checked; 0 errors, 7 warnings. Rpmlint (debuginfo) ------------------- Checking: dump1090-debuginfo-0-1.20210727gitde61bd5.fc35.aarch64.rpm 1 packages and 0 specfiles checked; 0 errors, 0 warnings. Rpmlint (installed packages) ---------------------------- Cannot parse rpmlint output: Source checksums ---------------- https://github.com/antirez/dump1090/archive/de61bd564f1aa929bae414a70e421acd0b81789a/dump1090-gitde61bd5.tar.gz : CHECKSUM(SHA256) this package : 7ab38886dc901799c7a51e4cfacd413eb27c739979faab99e007c76e631631c9 CHECKSUM(SHA256) upstream package : 7ab38886dc901799c7a51e4cfacd413eb27c739979faab99e007c76e631631c9 Requires -------- dump1090 (rpmlib, GLIBC filtered): glibc ld-linux-aarch64.so.1()(64bit) libc.so.6()(64bit) libm.so.6()(64bit) librtlsdr.so.0()(64bit) rtld(GNU_HASH) dump1090-debuginfo (rpmlib, GLIBC filtered): dump1090-debugsource (rpmlib, GLIBC filtered): Provides -------- dump1090: dump1090 dump1090(aarch-64) dump1090-debuginfo: debuginfo(build-id) dump1090-debuginfo dump1090-debuginfo(aarch-64) dump1090-debugsource: dump1090-debugsource dump1090-debugsource(aarch-64) Generated by fedora-review 0.7.6 (b083f91) last change: 2020-11-10 Command line :/usr/bin/fedora-review -b 1987045 Buildroot used: fedora-rawhide-aarch64 Active plugins: Shell-api, Generic, C/C++ Disabled plugins: fonts, PHP, SugarActivity, Ocaml, R, Python, Java, Haskell, Perl Disabled flags: EPEL6, EPEL7, DISTTAG, BATCH, EXARCH
Great pointers Ben! In addition to Ben's feedback, I had a few additional suggestions and questions. I'll save posting fedora-review output until the next round since my output is similar to Ben's. Questions: - Should the `*` be removed from the license text? - Should the slightly more restrictive BSD 3-clause found in anet.h be used instead? Small issues: - the html page does not need execute perms, suggest 644 instead. - Remove the commented lines for Requires & configure - Can you do a scratch build so we can see how this package behaves on other archs? I followed the directions in the README and was able to get between 6-8 tracks using my little whip antenna almost immediately, so it seems to works as described! If you do decide to create and maintain the manpage, it might be nice to put the few quick examples outlined in the README there. I found them to be really helpful. Personal opinion on the patch. The patch is arguably specific to this package, so I would be surprised if it makes sense to upstream. Still there should be a comment on the patch explaining that.
Spec URL: https://gathman.org/linux/SPECS/dump1090.spec SRPM URL: https://gathman.org/linux/f33/src/dump1090-0-2.20210727gitde61bd5.fc33.src.rpm https://koji.fedoraproject.org/koji/taskinfo?taskID=73175881
Just built and installed on rpi3. It complains: Found 1 device(s): 0: , , SN: (currently selected) usb_open error -3 Please fix the device permissions, e.g. by installing the udev rules file rtl-sdr.rules Error opening the RTLSDR device: Permission denied rtl_test finds no errors. Had to add login to rtlsdr group. I didn't have to do that on an Intel f33 system. After adding login to rtlsdr group, it works on rpi3.
(In reply to Stuart D Gathman from comment #6) > Just built and installed on rpi3. It complains: > > Found 1 device(s): > 0: , , SN: (currently selected) > usb_open error -3 > Please fix the device permissions, e.g. by installing the udev rules file > rtl-sdr.rules > Error opening the RTLSDR device: Permission denied > > rtl_test finds no errors. Had to add login to rtlsdr group. I didn't have > to do that on an Intel f33 system. > > After adding login to rtlsdr group, it works on rpi3. This is interesting. The rtl-sdr package is responsible for the mentioned udev rules file: https://src.fedoraproject.org/rpms/rtl-sdr/blob/rawhide/f/rtl-sdr.spec. I would say this sounds like a bug in that package, but perhaps on x86 rather than on arm/rpi3; the spec file comment implies you *should* have to be a member of the “rtlsdr” group to control the device.
Ah. When I first plugin the rtlsdr, it has group root. When I restart systemd-udev, still owned by group root. But now when I plugin again, it is owned by group rtlsdr. I had indeed restarted systemd-udev on rpi3, thinking it was because I had just installed rtl-sdr that the usb perms weren't getting set. And I had to replug the rtlsdr on rpi3 also (thinking it was because the udev rules had just been installed). So nice to know it works the same on both arches. :-) It could be a bug in udev or in rtl-sdr - but I am not deep enough into udev to be sure. I will let it go, and maybe make a bug report.
Yeah when I plugged mine in I wondered to myself if I had setup the RTLSDR on this laptop or not, and happily I had! Overall looks good now, especially will those handly forge macros, just a few nits in the text but otherwise looks good to go to me! ===== Issues ==== - A small rpmlint warning that should probably be addressed: - dump1090.src:7: W: mixed-use-of-spaces-and-tabs (spaces: line 2, tab: line 7) - It probably makes the most sense to put the %forgemeta line before the Name: line, since it defines values used later, and isn't really "part" of the header. ===== MUST items ===== C/C++: [x]: Package does not contain kernel modules. [x]: Package contains no static executables. [x]: If your application is a C or C++ application you must list a BuildRequires against gcc, gcc-c++ or clang. [x]: Header files in -devel subpackage, if present. [x]: Package does not contain any libtool archives (.la) [x]: Rpath absent or only used for internal libs. 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: "BSD (3 clause)", "Unknown or generated", "BSD (2 clause)". 6 files have unknown license. Detailed output of licensecheck in /home/troycurtisjr/working/oss/fedora/reviews/dump1090/2/review- dump1090/licensecheck.txt [-]: License file installed when any subpackage combination is installed. [x]: %build honors applicable compiler flags or justifies otherwise. [x]: Package contains no bundled libraries without FPC exception. [x]: Changelog in prescribed format. [x]: Sources contain only permissible code or content. [-]: Package contains desktop file if it is a GUI application. [-]: Development files must be in a -devel package [x]: Package uses nothing in %doc for runtime. [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]: Useful -debuginfo package or justification otherwise. [x]: Package is not known to require an ExcludeArch tag. [-]: Large documentation must go in a -doc subpackage. Large could be size (~1MB) or number of files. Note: Documentation size is 20480 bytes in 2 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: There are rpmlint messages (see attachment). [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 %license. [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]: 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]: Dist tag is present. [x]: Package does not contain duplicates in %files. [x]: Permissions on files are set properly. [x]: Package must not depend on deprecated() packages. [x]: Package use %makeinstall only when make install DESTDIR=... doesn't work. [x]: Package is named using only allowed ASCII characters. [x]: Package does not use a name that already exists. [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 ===== 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. Note: Technically the license text is from upstream, but extracted from a header. [x]: Patches link to upstream bugs/comments/lists or are otherwise justified. [-]: Sources are verified with gpgverify first in %prep if upstream publishes signatures. Note: gpgverify is not used. [-]: Description and summary sections in the package spec file contains translations for supported Non-English languages, if available. [x]: Package should compile and build into binary rpms on all supported architectures. [-]: %check is present and all tests pass. [x]: Packages should try to preserve timestamps of original installed files. [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]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin. [x]: Fully versioned dependency in subpackages if applicable. [x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file [x]: Sources can be downloaded from URI in Source: tag [x]: SourceX is a working URL. [x]: Spec use %global instead of %define unless justified. ===== EXTRA items ===== Generic: [x]: Rpmlint is run on debuginfo package(s). Note: No rpmlint messages. [x]: Rpmlint is run on all installed packages. Note: There are rpmlint messages (see attachment). [x]: Large data in /usr/share should live in a noarch subpackage if package is arched. [x]: Spec file according to URL is the same as in SRPM. Rpmlint ------- Checking: dump1090-0-2.20210727gitde61bd5.fc35.x86_64.rpm dump1090-debuginfo-0-2.20210727gitde61bd5.fc35.x86_64.rpm dump1090-debugsource-0-2.20210727gitde61bd5.fc35.x86_64.rpm dump1090-0-2.20210727gitde61bd5.fc35.src.rpm dump1090.x86_64: W: spelling-error %description -l en_US checksum -> check sum, check-sum, checks um dump1090.x86_64: W: spelling-error %description -l en_US xored -> cored, gored, pored dump1090.x86_64: W: spelling-error %description -l en_US ifile -> file, i file dump1090.x86_64: W: no-manual-page-for-binary dump1090 dump1090.src: W: spelling-error %description -l en_US checksum -> check sum, check-sum, checks um dump1090.src: W: spelling-error %description -l en_US xored -> cored, gored, pored dump1090.src: W: spelling-error %description -l en_US ifile -> file, i file dump1090.src:74: W: macro-in-%changelog %{distprefix} dump1090.src:77: W: macro-in-%changelog %forge dump1090.src:7: W: mixed-use-of-spaces-and-tabs (spaces: line 2, tab: line 7) dump1090.src: W: patch-not-applied Patch0: dump1090-share.patch 4 packages and 0 specfiles checked; 0 errors, 11 warnings. Rpmlint (debuginfo) ------------------- Checking: dump1090-debuginfo-0-2.20210727gitde61bd5.fc35.x86_64.rpm 1 packages and 0 specfiles checked; 0 errors, 0 warnings. Rpmlint (installed packages) ---------------------------- Cannot parse rpmlint output: Source checksums ---------------- https://github.com/antirez/dump1090/archive/de61bd564f1aa929bae414a70e421acd0b81789a/dump1090-de61bd564f1aa929bae414a70e421acd0b81789a.tar.gz : CHECKSUM(SHA256) this package : 7ab38886dc901799c7a51e4cfacd413eb27c739979faab99e007c76e631631c9 CHECKSUM(SHA256) upstream package : 7ab38886dc901799c7a51e4cfacd413eb27c739979faab99e007c76e631631c9 Requires -------- dump1090 (rpmlib, GLIBC filtered): libc.so.6()(64bit) libm.so.6()(64bit) librtlsdr.so.0()(64bit) rtld(GNU_HASH) dump1090-debuginfo (rpmlib, GLIBC filtered): dump1090-debugsource (rpmlib, GLIBC filtered): Provides -------- dump1090: dump1090 dump1090(x86-64) dump1090-debuginfo: debuginfo(build-id) dump1090-debuginfo dump1090-debuginfo(x86-64) dump1090-debugsource: dump1090-debugsource dump1090-debugsource(x86-64) Generated by fedora-review 0.7.6 (b083f91) last change: 2020-11-10 Command line :/usr/bin/fedora-review -n dump1090 Buildroot used: fedora-rawhide-x86_64 Active plugins: C/C++, Shell-api, Generic Disabled plugins: fonts, R, Haskell, PHP, SugarActivity, Ocaml, Python, Perl, Java Disabled flags: EPEL6, EPEL7, DISTTAG, BATCH, EXARCH
Spec URL: https://gathman.org/linux/SPECS/dump1090.spec SRPM URL: https://gathman.org/linux/f35/src/dump1090-0-3.20210727gitde61bd5.fc35.src.rpm Fixed the two textual nits I am thinking about trying to make a tool to convert --help output to a markdown man page (not hard). And also insert the README or portions of it (harder). I've been writing man pages in markdown, and using pandoc (previously used marked-man) to convert to nroff at build time.
Do I need to cobble up a man page to get approved? Is there a tool auto make an initial one from --help output ? (I'll ask on IRC)
Spec URL: https://gathman.org/linux/SPECS/dump1090.spec SRPM URL: https://gathman.org/linux/f35/src/dump1090-0-4.20210727gitde61bd5.fc35.src.rpm Manually made a markdown man page. I couldn't get help2man to work with the --help output, even with an h2m file.
Sorry I wasn't clear Stuart, I approved on the last comment. The text nits weren't blockers IMO, and neither was the man page. However, I took the opportunity to check out your updates and I think they look great. It is definitely much nicer to have that man page available! I wonder if upstream would be open to integrating some form of the man page eventually? So to be very clear this time, it's a approved (+) from me.
(fedscm-admin): The Pagure repository was created at https://src.fedoraproject.org/rpms/dump1090