https://bugzilla.redhat.com/show_bug.cgi?id=2126785 Björn Persson <bjorn@xxxxxxxxxxxxxxxxxxxx> changed: What |Removed |Added ---------------------------------------------------------------------------- Flags| |needinfo?(mark.e.fuller@gmx | |.de) --- Comment #5 from Björn Persson <bjorn@xxxxxxxxxxxxxxxxxxxx> --- Well, there are quite a few things to fix in this package. 1: The source URL is wrong. 404: Not Found. 2: The name "usbrelay-common" suggests a package that isn't useful by itself, but contains files that other packages need. The summary reinforces this impression. Users are likely to get the impression that there are interfaces for C, Python and MQTT but no command line interface. I think it would be better to call this subpackage just "usbrelay" (making it the main package), but it should at least be made clearer that it contains a command line tool. The summary could say something like "a library and command line tool for controlling USB-connected relays". 3: In the license field, SPDX syntax is now required for new packages. "GPLv2" is the old syntax. 4: GPL version 2 only is also wrong for the C parts. All the C files say "any later version", which is GPL-2.0-or-later in SPDX. 5: What license applies to the Python parts is unclear. The Python files themselves don't say. For the daemon it looks like GPL-2.0-only, because LICENSE.md presumably applies and nothing says "any later version" for usbrelayd. The file METADATA in python3-usbrelay seems to say GPL-2.0-only, but that's contradicted by libusbrelay_py.c which says "any later version". METADATA also disagrees with the file LICENSE in the same directory, which looks like MIT. I can't tell what code the MIT license applies to. And why is that file not in /usr/share/licenses? Please work with upstream to get this licensing mess sorted out. 6: The dependency on Git seems superfluous. I don't see Git invoked in the build log. 7: The explicit dependency on hidapi (not -devel) seems superfluous. The automatic soname dependency should suffice. If there is a reason for the explicit dependency, then it should be explained in a comment. 8: usbrelay-devel should require %{name}-common%{_isa} = %{version}-%{release}. 9: Is there a good reason why you don't use %{make_install}? The makefile looks like it supports DESTDIR correctly. 10: Several files are incorrectly marked as executable. >From the build log: *** WARNING: ./usr/lib64/python3.11/site-packages/usbrelay/usbrelay_test.py is executable but has no shebang, removing executable bit *** WARNING: ./usr/lib/udev/rules.d/50-usbrelay.rules is executable but has no shebang, removing executable bit *** WARNING: ./usr/lib/systemd/system/usbrelayd.service is executable but has no shebang, removing executable bit *** WARNING: ./usr/include/libusbrelay.h is executable but has no shebang, removing executable bit *** WARNING: ./etc/usbrelayd.conf is executable but has no shebang, removing executable bit >From RPMlint: usbrelay-mqtt.x86_64: W: spurious-executable-perm /usr/share/man/man8/usbrelayd.8.gz usbrelay-common.x86_64: W: spurious-executable-perm /usr/share/man/man1/usbrelay.1.gz Use cp instead of install, or pass --mode to install. 11: File timestamps should be preserved. Pass --preserve-timestamps to install, or use cp --preserve=timestamps. 12: The flags in py3_shebang_flags are missing from usbrelayd. It seems they're added automatically to programs in /usr/bin but not in /usr/sbin. Maybe work around it with py3_shebang_fix? 13: The comment "Create and empty key file and pid file to be marked as a ghost file below." confuses me. I see the pid file but what is the key file? If the sentence mentions two files, then it should be rephrased as plural. If it's about a single file, then the first "and" should be "an". 14: As Paweł noted, the scriptlets must be assigned to the correct subpackages. 15: No package owns the directory /usr/lib64/python3.11/site-packages/usbrelay. 16: usbrelay-common must require systemd-udev because it places a file in /usr/lib/udev/rules.d. (If it can function in a stripped-down system witout Udev installed, then it must instead own /usr/lib/udev and /usr/lib/udev/rules.d.) 17: /etc/usbrelayd.conf should be marked as %config(noreplace). 18: The manpages must be listed in the %files sections as patterns to allow other suffixes than ".gz". I'm not sure about these. They may be serious problems: 19: I get no hits searching for "usbrelay" on PyPI. There is a policy to prevent name collisions between Fedora and PyPI. I don't know whether a Python module that is only a part of the project can be published on PyPI. If not, the policy says that the name shall be blocked on PyPI. Has that been done? 20: usbrelay.sysusers creates only a group, but a username is specified for the pid file. I'm not sure how that's going to work. 21: The makefile contains code for signing a release. Can we get that signature and verify it? When upstream projects sign their releases, packages are supposed to verify the signature. 22: The comment in %check seems to say that the Python module can't even be imported unless one of the supported relays is connected. Do I understand that correctly? That seems like terrible design that should be rectified upstream. The policy requires at least a smoke test. These are just suggestions. They don't block the review: 23: Why not use %{make_build}? I acknowledge that there's nothing to parallelize in the makefile, but it's policy and it shouldn't hurt. 24: The gzip commands shouldn't be necessary. RPMbuild compresses manpages automatically. -- 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 https://bugzilla.redhat.com/show_bug.cgi?id=2126785 _______________________________________________ package-review mailing list -- package-review@xxxxxxxxxxxxxxxxxxxxxxx To unsubscribe send an email to package-review-leave@xxxxxxxxxxxxxxxxxxxxxxx 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@xxxxxxxxxxxxxxxxxxxxxxx Do not reply to spam, report it: https://pagure.io/fedora-infrastructure/new_issue
