https://bugzilla.redhat.com/show_bug.cgi?id=1645172 --- Comment #9 from Jaroslav Škarvada <jskarvad@xxxxxxxxxx> --- (In reply to dan.cermak from comment #8) Thanks for comments. > 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. > This is good point. I think this could be resolved by asking firejail authors whether we could reuse their SPEC file under MIT. If not there should be explicit comment stating that the spec file is licensed under GPLv2+. > - 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). > No idea about it. I would also try to drop it. > - 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.* > Valid: https://fedoraproject.org/wiki/Packaging:Guidelines#Manpages > - Minor: you can get a tarball with a better name from github via this url: > > Source0: %{URL}/archive/%{version}/%{name}-%{version}.tar.gz > Good point. > - 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} Not needed, it's already owned, check rpmls. > > %dir %{_datadir}/doc/%{name} owned by %doc macro > > %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) > IMHO no, because it's already owned by filesystem: https://fedoraproject.org/wiki/Packaging:Guidelines#File_and_Directory_Ownership But IMHO the path should be fixed to /etc/bash_completion.d > - 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. Personally I probably wouldn't run the tests in the form there are available now, because they seems like integration tests to me (I didn't check thoroughly). They have a lot of dependencies, which are in fact build time dependencies, but it could slow down or even break the build. Also it seems there are bundled binaries used in the tests you shouldn't definitely run in the build time, e.g. test/filters/memwrexe. Well, there is source for it, but I cannot find recipe to build it. But of course this could be reported upstream, e.g. request to provide some minimal test target. -- 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
