https://bugzilla.redhat.com/show_bug.cgi?id=1175270 --- Comment #3 from Jonathan Dieter <jdieter@xxxxxxxxx> --- Thanks so much for the review!!! Comments are inline: (In reply to Zbigniew Jędrzejewski-Szmek from comment #2) > 0. A general question: wouldn't this be better build as a part of the kernel > package? The kernel developers don't want to have to deal maintaining the userspace (which is fair enough). See the last part of https://bugzilla.redhat.com/show_bug.cgi?id=1169478#c6 and https://bugzilla.redhat.com/show_bug.cgi?id=1169478#c11. > 1. remove %clean Done > 2. remove %defattr Done > 3. Consider using %license for COPYING Done > 4. Consider adding something like (copied from another package): > > # Use the same directory of the main package for subpackage licence and docs > %global _docdir_fmt %{name} > > This will avoid having a doc/usbip-devel directory with one file (or > licenses/usbip-devel directory with one file if you do 3.) Done > 4. Combine the two %systemd_posts into one, it'll reload systemd just once. Done. > 5. /etc/default is an abomination. You can incorporate the file into the > systemd service file > as a comment. If somebody is doing debugging, they can create > /run/systemd/system/usbip-server.d/override.conf with updates the options > (by the > time F22 comes out, systemctl edit usbip-server will do this automatically). I agree. The problem is that this package has been in RPM Fusion with the /etc/default config file, so I'm a bit hesitant to possibly break people's configs. I have removed it for the moment, but if I get any bug reports, I will probably put it back in. > 6. Module loading in a service file is very much discouraged. Is is not > possible to have the module > load automatically? And rmmod in a service file is usually a bad idea > because rmmod can interfere with other > things. Why is this necessary? First off, I agree on rmmod and have removed those. As for module loading, I don't think there's any other way of loading them only when they're needed. It's not like we can watch for the appearance of a device or anything like that, and, because there are separate modules for server and client, I'd rather not have them both automatically loaded at startup. > 7. Drop After=syslog.target. Done. <snip> > [!]: Fully versioned dependency in subpackages if applicable. > Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in > usbip-devel > It should be added for (your) sanity. An automatic requires is generated on > the > library, but it does not include the specific version, and you do not want > to have reports for a mismatching main and devel packages. I think this might be a false positive. I do have that line exactly as is for the -devel subpackage. <snip> > [?]: Latest version is packaged. > I guess that the version depeneds on the branch. It should be 3.18 for > rawhide. Updating to 3.18. Updated URLs: Spec URL: http://lesloueizeh.com/jdieter/usbip/usbip.spec SRPM URL: http://lesloueizeh.com/jdieter/usbip/usbip-3.18-1.fc21.src.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@xxxxxxxxxxxxxxxxxxxxxxx https://admin.fedoraproject.org/mailman/listinfo/package-review