https://bugzilla.redhat.com/show_bug.cgi?id=1645172 --- Comment #8 from dan.cermak@xxxxxxxxxxxxxxxxxxx --- Thanks for packaging firejail! I have some comments concerning the spec file: - Your spec file does not mention a special license, which afaik means that MIT is going to be assumed. However, you seem to have taken the spec from the firejail repository, which means that it is GPLv2 licensed and changing the license to MIT is definitely problematic. - The flags to the configure step: > %configure --disable-userns --disable-contrib-install were (judging from the git log) added for compatibility reasons with RHEL/CentOS. The two disable flags could maybe be dropped for Fedora (not sure if installing contrib is useful though). - The %files section should not add man pages with hardcoded extensions, since it could be changed without further notice. So this: > %{_mandir}/man5/%{name}-login.5.gz > %{_mandir}/man5/%{name}-profile.5.gz > %{_mandir}/man5/%{name}-users.5.gz should become this: > %{_mandir}/man5/%{name}-login.5.* > %{_mandir}/man5/%{name}-profile.5.* > %{_mandir}/man5/%{name}-users.5.* - Minor: you can get a tarball with a better name from github via this url: > Source0: %{URL}/archive/%{version}/%{name}-%{version}.tar.gz - I am not completely sure about the rules when a package should own a directory, but I think it might be necessary for the spec to explicitly own the following: > %dir %{_libdir}/%{name} > %dir %{_datadir}/doc/%{name} > %dir %{_datarootdir}/bash-completion/completions (really not sure about the last one, there was a discussion in the previous packaging request of firejail: https://bugzilla.redhat.com/show_bug.cgi?id=1301286) - Firejail has a testsuite, it can be run with make test (there is also a more "destructive" test suite, which messes with the OS' settings, wouldn't advise to use that one). It requires at least expect to be installed. It seems to test some basic capabilities of firejail and also some applications and their behavior (like firefox, thunderbird, dig, telnet, etc.). I have however not taken an in depth look at it, for instance I am not really sure if it returns a non-zero value on test failure. I am also not sure whether it makes sense to test all the applications requiring X to be running (probably not). Nevertheless, running at least some basic tests would be desirable. -- 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 To unsubscribe send an email to package-review-leave@xxxxxxxxxxxxxxxxxxxxxxx Fedora Code of Conduct: https://getfedora.org/code-of-conduct.html List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedoraproject.org/archives/list/package-review@xxxxxxxxxxxxxxxxxxxxxxx
