Spec URL: https://domsch.com/fedora/sdr/CubicSDR.spec SRPM URL: https://domsch.com/fedora/sdr/CubicSDR-0.2.5-1.20200226gitd2f9333.fc33.src.rpm Description: Cross-Platform Software-Defined Radio Panadapter Fedora Account System Username: mdomsch This is unretiring a dead package, which was removed because wxGTK 3.1 was not yet available in Fedora, and the package strictly required wxGTK 3.1. Rawhide (F33) now has wxGTK 3.1 in it's repository. As such, this package can be unretired, and it now requires a re-review. In parallel, I've updated the liquid-dsp package, on which CubicSDR depends, to the latest release, as this snapshot of CubicSDR requires that.
Good deal. I'll provide feedback as soon as I can. In an all day tele-workshop for $DAYJOB today.
I hope Richard doesn't think I'm stepping on his toes... but I'll provide some feedback and a package review as well, if I can get to it before he does. I've found two issues so far: * No matching package to install: 'liquid-dsp-devel >= 1.3.2' * No matching package to install: 'wxGTK-devel' It looks like the first was just recently added to rawhide, but hasn't yet hit the repos. I haven't had time to track down the second yet...
you can't build it outside of rawhide because wxGTK-devel doesn't exist outside of rawhide yet. liquid-dsp-devel 1.3.2 hit rawhide last night, and I can do scratch-builds of the SRPM in a rawhide branch successfully. https://koji.fedoraproject.org/koji/taskinfo?taskID=43130539
Ok, first level spec review... 1. Is this a post release of 0.2.4 or a pre-release of 0.2.5? If it's a pre-release then the beginning of the Release: tag should be <1, so this would be appropriate: 0.1.%{snapshotdate}git%{shortcommit}%{?dist} You would increment the 0.1 to 0.2 if you build another pre-release, etc. That way when 0.2.5 final is released it will reset to 1. 2. Minor nit... The Summary is the same as %description, so you COULD use ust "%{summary}." but ideally the description would well, be more descriptive. 3. You should unbundle tinyxml is possible. While the restriction on use of bundled libraries has been reduced A LOT in the last few years it's still a best practice to use the system ones if practical. As the maintainer of a lot of CMake projects, it's usually pretty easy unless upstream has intentionally obfuscated their config. 4. BuildRequires: nit... It's best practice to list them one per line HOWEVER, I'm not that pedantic but I generally only group together ones that are related, e.g.: BuildRequires: cmake gcc-c++ desktop-file-utils # Library dependencies BuildRequires: SoapySDR-devel BuildRequires: wxGTK-devel BuildRequires: hamlib-devel BuildRequires: fftw-devel BuildRequires: rtaudio-devel BuildRequires: liquid-dsp-devel >= 1.3.2 5. Interesting you have to tell cmake where wx-config is even though which finds it just fine... May be a good idea in the long run to fix CMake and send the patch upstream. 6. Not really a packaging guidelines violation, but it may be best if you need a bash script to load it to move the binary to /usr/libexec instead or renaming to %{name}.bin. 7. There's no requirement to use %dir if you're just going to glob everything in it, just use a trailing forward slash so: %dir %{_datadir}/cubicsdr %{_datadir}/cubicsdr/* Becomes: %{_datadir}/cubicsdr/ 8. One more minor nit, the guidelines the spec file should be "readable" but doesn't specify everything that makes it "readable". I usually add two newlines between major sections of the spec file, (%prep, %build, %install, % files, %changelog).
Thanks for the review notes. 1. Its a post-release of 0.2.5, or a pre-release of 0.2.6. I've kept it as 0.2.5 for this reason. 3. Upstream declined to let this be easily separated, or move to tinyxml2, during the initial review. https://github.com/cjcliffe/CubicSDR/issues/670 6. It's not clear to me if this is required for all systems, or just mine, due to the graphics card. I've moved this environment setting into the .desktop file, which works fine, but then you can't start it from the command line without setting this in your environment. :-( I could use libexec.
(In reply to Matt Domsch from comment #5) > Thanks for the review notes. > > 1. Its a post-release of 0.2.5, or a pre-release of 0.2.6. I've kept it as > 0.2.5 for this reason. Ok, so where is the 0.2.5 release, when I got to https://cubicsdr.com/ I only see 0.2.4. > 3. Upstream declined to let this be easily separated, or move to tinyxml2, > during the initial review. https://github.com/cjcliffe/CubicSDR/issues/670 So at a high level, I get it, they are prioritizing having a known entity over a distro's responsibility for security. Nothing new. It's not a show stopper for the review but unfortunate. > 6. It's not clear to me if this is required for all systems, or just mine, > due to the graphics card. I've moved this environment setting into the > .desktop file, which works fine, but then you can't start it from the > command line without setting this in your environment. :-( I could use > libexec. That's definitely a concern, but somewhat orthogonal to the (albeit subjecting) requirement. If the purpose of the bash script is to properly load the program, and it is not possible or practical to run the binary directly, then it should get placed in /usr/libexec. This is more of a policy thing than something that has any practical impact. Basically, if an end user can't run the binary directly, it shouldn't be in /usr/bin. After you've updated the spec file, please increment the release (whichever way is more appropriate per #1) and pose a new SPEC and SPRM noting the major changes in the %changelog.
0.2.5 is posted on the github releases page. https://github.com/cjcliffe/CubicSDR/releases Therefore, this is 0.2.5-3.{date}git{sha}. I've updated the SRPM and spec at https://domsch.com/fedora/sdr/ following all other comments, including moving the executable to libexecdir. https://koji.fedoraproject.org/koji/taskinfo?taskID=43172146 has a scratch-build. I rebuilt the source tarball to delete directories from the external/ subdirectory that are unused for our builds (MSVC stuff, prebuilt copies of liquid-dsp, hamlib, rtaudio). That brought the SRPM size down from 35MB to 1.4MB. Thanks, Matt
Good deal. Typically sources are not repacked unless it has content that must be removed but in this case that's a lot of bloat.
Working on the full review... 1. Where does the GPLv2+ come from? Looking at the github page it specifies GPL-2.0 and license file in the source has no reference to "or greater". 2. Minor nit... The desktop file can be improved. Categories=Science;HamRadio;DataVisualization; Hmm... DataVisualization? I would say "Audio"... Not a show stopper by interesting choices by upstream. 3. Thanks for moving the binary, but I'm confused. I thought you were using a shell script to launch it (which is why the binary was renamed to append .bin), but the Desktop file is calling the binary in /usr/libexec... $ cat CubicSDR-0.2.5-3.20200226gitd2f9333.fc33.x86_64.rpm/usr/bin/CubicSDR #!/usr/bin/sh GDK_BACKEND=x11 /usr/libexec/CubicSDR/CubicSDR The desktop file should be using the shell script, which would solve the problem of rpmlint complaining about the use of "env" in the desktop file anyway... Am I missing something?
The header of each file in the src/ tree has: // SPDX-License-Identifier: GPL-2.0+ I used upstream's desktop file as much as possible, with the sole exception to point to the binary in libexec, and the env. I moved the 'env' into the desktop file so as to have one fewer running shell, no need for that extra bash shell when the desktop environment can run it directly with the proper environment set. One can use the bash to start it from a CLI, or the desktop icon, it works fine either way. I'm not partial to what Categories to put in there, I just didn't change it from upstream without good reason.
(In reply to Matt Domsch from comment #10) > The header of each file in the src/ tree has: > // SPDX-License-Identifier: GPL-2.0+ Interesting, that's not enough for the licensecheck program to pick up. I also compared the LICENSE file with gnu.org's GPLv2 (not v2+) and it's identical. Upstream should really pick one or the other. However, attributions in a file win over a LICENSE file (though I'm not sure just having the one-liners count) so we'll go with v2+... Would be good to file an issue on github to get clarification of their intentions there.
Package Review ============== Legend: [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated [ ] = Manual review needed Issues: ======= ===== 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: "Unknown or generated", "zlib/libpng license", "Expat License", "*No copyright* zlib/libpng license". 275 files have unknown license. Detailed output of licensecheck in /home/build/fedora- review/CubicSDR/licensecheck.txt [x]: License file installed when any subpackage combination is installed. [x]: If the package is under multiple licenses, the licensing breakdown must be documented in the spec. [x]: %build honors applicable compiler flags or justifies otherwise. [-]: 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]: Package consistently uses macros (instead of hard-coded directory names). [-]: 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. [x]: If the package is a rename of another package, proper Obsoletes and Provides are present. [-]: Requires correct, justified where necessary. [x]: Spec file is legible and written in American English. [-]: Package contains systemd file(s) if in need. [-]: Useful -debuginfo package or justification otherwise. [x]: Package is not known to require an ExcludeArch tag. [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]: 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]: 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 is not relocatable. [x]: Spec file name must match the spec package %{name}, in the format %{name}.spec. [x]: File names are valid UTF-8. [x]: Large documentation must go in a -doc subpackage. Large could be size (~1MB) or number of files. Note: Documentation size is 0 bytes in 0 files. [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. [-]: Patches link to upstream bugs/comments/lists or are otherwise justified. [x]: 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. Rpmlint ------- Checking: CubicSDR-0.2.5-3.20200226gitd2f9333.fc33.x86_64.rpm CubicSDR-debuginfo-0.2.5-3.20200226gitd2f9333.fc33.x86_64.rpm CubicSDR-debugsource-0.2.5-3.20200226gitd2f9333.fc33.x86_64.rpm CubicSDR-0.2.5-3.20200226gitd2f9333.fc33.src.rpm CubicSDR.x86_64: W: incoherent-version-in-changelog 0.2.5-3.20200407gitd2f9333 ['0.2.5-3.20200226gitd2f9333.fc33', '0.2.5-3.20200226gitd2f9333'] CubicSDR.x86_64: W: no-documentation CubicSDR.x86_64: W: no-manual-page-for-binary CubicSDR CubicSDR.x86_64: W: desktopfile-without-binary /usr/share/applications/CubicSDR.desktop env CubicSDR.src: W: strange-permission CubicSDR-d2f93335236ef37557057d7b71c46969bec14471.tar.gz 775 4 packages and 0 specfiles checked; 0 errors, 5 warnings. Rpmlint (debuginfo) ------------------- Checking: CubicSDR-debuginfo-0.2.5-3.20200226gitd2f9333.fc33.x86_64.rpm 1 packages and 0 specfiles checked; 0 errors, 0 warnings. Rpmlint (installed packages) ---------------------------- perl: warning: Setting locale failed. perl: warning: Please check that your locale settings: LANGUAGE = (unset), LC_ALL = (unset), LC_CTYPE = "C.UTF-8", LANG = "en_US.UTF-8" are supported and installed on your system. perl: warning: Falling back to the standard locale ("C"). perl: warning: Setting locale failed. perl: warning: Please check that your locale settings: LANGUAGE = (unset), LC_ALL = (unset), LC_CTYPE = "C.UTF-8", LANG = "en_US.UTF-8" are supported and installed on your system. perl: warning: Falling back to the standard locale ("C"). CubicSDR.x86_64: W: incoherent-version-in-changelog 0.2.5-3.20200407gitd2f9333 ['0.2.5-3.20200226gitd2f9333.fc33', '0.2.5-3.20200226gitd2f9333'] CubicSDR.x86_64: W: invalid-url URL: https://cubicsdr.com <urlopen error [Errno -2] Name or service not known> CubicSDR.x86_64: W: no-documentation CubicSDR.x86_64: W: no-manual-page-for-binary CubicSDR CubicSDR-debugsource.x86_64: W: invalid-url URL: https://cubicsdr.com <urlopen error [Errno -2] Name or service not known> CubicSDR-debuginfo.x86_64: W: invalid-url URL: https://cubicsdr.com <urlopen error [Errno -2] Name or service not known> 3 packages and 0 specfiles checked; 0 errors, 6 warnings. Source checksums ---------------- https://github.com/cjcliffe/CubicSDR/archive/d2f93335236ef37557057d7b71c46969bec14471/CubicSDR-d2f93335236ef37557057d7b71c46969bec14471.tar.gz : CHECKSUM(SHA256) this package : faf9a625b2b7380726dd3d723d05ee1469d9dbe55ad9d7163ebcb091c1aaf27d CHECKSUM(SHA256) upstream package : 32b53d5bdd1cb8558537e53b5578a710aaa7345ffe250155ff33a30e8b97e5bf diff -r also reports differences Requires -------- CubicSDR (rpmlib, GLIBC filtered): /usr/bin/sh libGLX.so.0()(64bit) libOpenGL.so.0()(64bit) libSoapySDR.so.0.7()(64bit) libc.so.6()(64bit) libdl.so.2()(64bit) libgcc_s.so.1()(64bit) libgcc_s.so.1(GCC_3.0)(64bit) libhamlib.so.4()(64bit) libliquid.so.2.0()(64bit) libm.so.6()(64bit) libpthread.so.0()(64bit) librtaudio.so.6()(64bit) libstdc++.so.6()(64bit) libstdc++.so.6(CXXABI_1.3)(64bit) libstdc++.so.6(CXXABI_1.3.1)(64bit) libstdc++.so.6(CXXABI_1.3.2)(64bit) libstdc++.so.6(CXXABI_1.3.8)(64bit) libwx_baseu-3.1.so.3()(64bit) libwx_baseu-3.1.so.3(WXU_3.1)(64bit) libwx_gtk3u_core-3.1.so.3()(64bit) libwx_gtk3u_core-3.1.so.3(WXU_3.1)(64bit) libwx_gtk3u_gl-3.1.so.3()(64bit) libwx_gtk3u_gl-3.1.so.3(WXU_3.1)(64bit) libwx_gtk3u_propgrid-3.1.so.3()(64bit) libwx_gtk3u_propgrid-3.1.so.3(WXU_3.1)(64bit) rtld(GNU_HASH) CubicSDR-debuginfo (rpmlib, GLIBC filtered): CubicSDR-debugsource (rpmlib, GLIBC filtered): Provides -------- CubicSDR: CubicSDR CubicSDR(x86-64) application() application(CubicSDR.desktop) bundled(librs232) bundled(lodepng) bundled(tinyxml) CubicSDR-debuginfo: CubicSDR-debuginfo CubicSDR-debuginfo(x86-64) debuginfo(build-id) CubicSDR-debugsource: CubicSDR-debugsource CubicSDR-debugsource(x86-64) Generated by fedora-review 0.7.5 (5fa5b7e) last change: 2020-02-16 Command line :/usr/bin/fedora-review -r -n CubicSDR-0.2.5-3.20200226gitd2f9333.fc33.src.rpm Buildroot used: fedora-rawhide-x86_64 Active plugins: C/C++, Generic, Shell-api Disabled plugins: Haskell, Java, PHP, R, fonts, Python, SugarActivity, Ocaml, Perl Disabled flags: EPEL6, EPEL7, DISTTAG, BATCH, EXARCH [build@hobbes CubicSDR]$ *** APPROVED ***
Many thanks Richard for the thorough review. https://pagure.io/releng/issue/9401 is now open with releng to unretire the package.
Package is unretired and built on master branch. Closing.