https://bugzilla.redhat.com/show_bug.cgi?id=1645172 --- Comment #10 from Ondrej Dubaj <odubaj@xxxxxxxxxx> --- Thank you for your comments. (In reply to Jaroslav Škarvada from comment #9) > (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+. > Is it then OK just to leave a comment? Or it should be better to ask upstream? > > > - 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 I probbaly do not understand, how could I package something that does not exist in my buildroot? > > > - 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
