| Message ID | 20161222181323.GA5053@obsidianresearch.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
On 2016-12-22 1:13 PM, Jason Gunthorpe wrote: > On Thu, Dec 22, 2016 at 01:02:20PM -0500, Jarod Wilson wrote: >> These are numerous additional little fixups for the Fedora/Red Hat spec >> file, stemming from Fedora package review. >> >> - Per https://fedoraproject.org/wiki/Packaging:Systemd it seems we need to >> have systemd as a BuildRequires for %{_unitdir} to evaluate properly. >> >> - Add explicit dependencies on kmod, initscripts and systemd, as there >> are container and vm cases where there might not be a kernel >> installed > > Do we need initscripts? Not really sure, the Fedora package reviewer insists we do. >> - Make sure srp_daemon.sh is executable > > Oops, lets fix that commonly please, can you roll this in and drop the chmod: > > diff --git a/srp_daemon/CMakeLists.txt b/srp_daemon/CMakeLists.txt > index f2752a8e6e1580..acb851bcb8a438 100644 > --- a/srp_daemon/CMakeLists.txt > +++ b/srp_daemon/CMakeLists.txt > @@ -22,7 +22,8 @@ rdma_install_symlink(srp_daemon "${CMAKE_INSTALL_SBINDIR}/ibsrpdm") > rdma_install_symlink(srp_daemon "${CMAKE_INSTALL_SBINDIR}/run_srp_daemon") > rdma_subst_install(FILES "srp_daemon.sh.in" > DESTINATION "${CMAKE_INSTALL_SBINDIR}" > - RENAME "srp_daemon.sh") > + RENAME "srp_daemon.sh" > + PERMISSIONS OWNER_WRITE OWNER_READ GROUP_READ WORLD_READ OWNER_EXECUTE GROUP_EXECUTE WORLD_EXECUTE) Ah, yeah, that's much better. Crap, hit send on v2 just as your mail came in. I guess I'll spin up a v3 momentarily. >> Requires: %{name}%{?_isa} = %{version}-%{release} >> +Requires: logrotate > > I don't think we use logrotate for anything unless the srp rsyslog > examples are being used? Maybe it would be better to move the logrotate > and rsyslog stuff to doc/examples or something? This is another one where the package reviewer seems insistent.
On Thu, Dec 22, 2016 at 01:17:00PM -0500, Jarod Wilson wrote: > >>- Add explicit dependencies on kmod, initscripts and systemd, as there > >> are container and vm cases where there might not be a kernel > >> installed > > > >Do we need initscripts? > > Not really sure, the Fedora package reviewer insists we do. Hm, no idea what for. We don't provide any LSB init scripts any more, right? > >> Requires: %{name}%{?_isa} = %{version}-%{release} > >>+Requires: logrotate > > > >I don't think we use logrotate for anything unless the srp rsyslog > >examples are being used? Maybe it would be better to move the logrotate > >and rsyslog stuff to doc/examples or something? > > This is another one where the package reviewer seems insistent. By default "journald -u srp_daemon" does the same job, so I view these files as obsolete for pre-systemd distros. I think we should just not include the rsyslog and logrotate config files at all. Sophisticated users that need that stuff can install rsyslog and setup their own filtering. I assume that is the FC policy? That avoids the reviewer comment. It certainly makes 0 sense to Require logrotate without also requiring rsyslog because rsyslog is the thing that writes the file being rotated... Forcing rsyslog is *really* not desirable, most people don't want/need that overhead. Forcing logrotate is not desirable because it forces a useless cron job to run and folks in this world are sensitive to that jitter.. Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/22/2016 1:35 PM, Jason Gunthorpe wrote: > On Thu, Dec 22, 2016 at 01:17:00PM -0500, Jarod Wilson wrote: > >>>> - Add explicit dependencies on kmod, initscripts and systemd, as there >>>> are container and vm cases where there might not be a kernel >>>> installed >>> >>> Do we need initscripts? >> >> Not really sure, the Fedora package reviewer insists we do. > > Hm, no idea what for. We don't provide any LSB init scripts any more, > right? I think this is a distro specific thing. Last I knew, there might still be one or two things that haven't been migrated from initscripts to systemd, or maybe they want the functions that the initscripts package provides? >>>> Requires: %{name}%{?_isa} = %{version}-%{release} >>>> +Requires: logrotate >>> >>> I don't think we use logrotate for anything unless the srp rsyslog >>> examples are being used? Maybe it would be better to move the logrotate >>> and rsyslog stuff to doc/examples or something? >> >> This is another one where the package reviewer seems insistent. > > By default "journald -u srp_daemon" does the same job, so I view > these files as obsolete for pre-systemd distros. > > I think we should just not include the rsyslog and logrotate config > files at all. Sophisticated users that need that stuff can install > rsyslog and setup their own filtering. I assume that is the FC policy? > That avoids the reviewer comment. > > It certainly makes 0 sense to Require logrotate without also requiring > rsyslog because rsyslog is the thing that writes the file being > rotated... > > Forcing rsyslog is *really* not desirable, most people don't want/need > that overhead. Forcing logrotate is not desirable because it forces a > useless cron job to run and folks in this world are sensitive to that > jitter.. Agreed.
On Thu, Dec 22, 2016 at 01:41:59PM -0500, Doug Ledford wrote: > On 12/22/2016 1:35 PM, Jason Gunthorpe wrote: > > On Thu, Dec 22, 2016 at 01:17:00PM -0500, Jarod Wilson wrote: > > > >>>> - Add explicit dependencies on kmod, initscripts and systemd, as there > >>>> are container and vm cases where there might not be a kernel > >>>> installed > >>> > >>> Do we need initscripts? > >> > >> Not really sure, the Fedora package reviewer insists we do. > > > > Hm, no idea what for. We don't provide any LSB init scripts any more, > > right? > > I think this is a distro specific thing. Last I knew, there might still > be one or two things that haven't been migrated from initscripts to > systemd, or maybe they want the functions that the initscripts package > provides? No idea, but it is a big dependency (12 pacakges, 5M) If the reviewer wants it, lets have them propose a comment about what it is needed for since none of us seem to know :) Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/22/2016 2:00 PM, Jason Gunthorpe wrote: > On Thu, Dec 22, 2016 at 01:41:59PM -0500, Doug Ledford wrote: >> On 12/22/2016 1:35 PM, Jason Gunthorpe wrote: >>> On Thu, Dec 22, 2016 at 01:17:00PM -0500, Jarod Wilson wrote: >>> >>>>>> - Add explicit dependencies on kmod, initscripts and systemd, as there >>>>>> are container and vm cases where there might not be a kernel >>>>>> installed >>>>> >>>>> Do we need initscripts? >>>> >>>> Not really sure, the Fedora package reviewer insists we do. >>> >>> Hm, no idea what for. We don't provide any LSB init scripts any more, >>> right? >> >> I think this is a distro specific thing. Last I knew, there might still >> be one or two things that haven't been migrated from initscripts to >> systemd, or maybe they want the functions that the initscripts package >> provides? > > No idea, but it is a big dependency (12 pacakges, 5M) > > If the reviewer wants it, lets have them propose a comment about what > it is needed for since none of us seem to know :) Fair enough ;-)
diff --git a/srp_daemon/CMakeLists.txt b/srp_daemon/CMakeLists.txt index f2752a8e6e1580..acb851bcb8a438 100644 --- a/srp_daemon/CMakeLists.txt +++ b/srp_daemon/CMakeLists.txt @@ -22,7 +22,8 @@ rdma_install_symlink(srp_daemon "${CMAKE_INSTALL_SBINDIR}/ibsrpdm") rdma_install_symlink(srp_daemon "${CMAKE_INSTALL_SBINDIR}/run_srp_daemon") rdma_subst_install(FILES "srp_daemon.sh.in" DESTINATION "${CMAKE_INSTALL_SBINDIR}" - RENAME "srp_daemon.sh") + RENAME "srp_daemon.sh" + PERMISSIONS OWNER_WRITE OWNER_READ GROUP_READ WORLD_READ OWNER_EXECUTE GROUP_EXECUTE WORLD_EXECUTE) install(FILES logrotate-srp_daemon DESTINATION "${CMAKE_INSTALL_SYSCONFDIR}/logrotate.d" RENAME "srp_daemon") install(FILES rsyslog-srp_daemon.conf DESTINATION "${CMAKE_INSTALL_SYSCONFDIR}/rsyslog.d" RENAME "srp_daemon.conf")