https://bugzilla.redhat.com/show_bug.cgi?id=1826439 --- Comment #23 from Michal Schmidt <mschmidt@xxxxxxxxxx> --- (In reply to igor.ivanov.va from comment #22) > > > %build > > > export revision=%{use_rel} > > > > This seems unnecessary. I did not find anything referencing the "revision" > > environment variable in your build system. > > > > It is used during configure to set VMA_SVN_REVISION. Oh, I see. I was looking at the master branch, where it's not used anymore, but it is indeed used in the version you packaged. > > > %install > > > %{make_build} DESTDIR=${RPM_BUILD_ROOT} install > > > > You should use %{make_install} instead. > > > > Corrected but following rules as > https://docs.fedoraproject.org/en-US/packaging-guidelines/ > #_why_the_makeinstall_macro_should_not_be_used permit original usage. The current guidelines allow this form: make DESTDIR=$RPM_BUILD_ROOT install but that's not the same as what you originally had: %{make_build} DESTDIR=${RPM_BUILD_ROOT} install There's been a recent push to convert all Fedora spec files to %make_install: https://lists.fedoraproject.org/archives/list/devel@xxxxxxxxxxxxxxxxxxxxxxx/message/IPPMIU6DS664TCWTLBR7WA2CUAOFUD7M/ Thanks for having changed it to %{make_install}. > > > %files > > > %{_libdir}/%{name}.so* > > > > So the package contains something like this: > > libvma.so -> libvma.so.9.1.0 > > libvma.so.9 -> libvma.so.9.1.0 > > libvma.so.9.1.0 > > > > Since libvma.so is to be used only for LD_PRELOAD and no program should be > > linked against it (and have DT_NEEDED for it),: > > 1. is there a point in versioning the library? > > 2. does it have to be installed in the linker's default path? Would it make > > more sense to have it as %{_libdir}/libvma/libvma.so ? > > livma has own extra api with vma_extra.h header file and admits direct > linkage. I didn't see such usage in the documentation, but I have now tested linking a C program with libvma directly and it seems to work, so OK. > > Do you actually use configure_options for anything? I don't think it's > > common for Fedora packages to have macros like that "just in case". > > This option is used to provide flexibility in final binary libvma version. OK, it does no harm. It can stay. > > 30-libvma-limits.conf: > > > # Default limits that are needed for proper work of libvma > > > # Read more about this topic in the VMA's User Manual > > > * - memlock unlimited > > > * soft memlock unlimited > > > * hard memlock unlimited > > > > Does having the package installed give every user on the system the > > permission to mlock unlimited amounts of RAM? Is that really necessary? > > Could it be at least limited to a user group? > > There is such recommendation at > https://docs.mellanox.com/display/VMAv902/VMA+Installation+Options that > forces to > https://github.com/Mellanox/libvma/wiki/VMA-over-RHEL-7.x-with-inbox- > driver#4-common-configurations Thanks for the pointer, but it does not answer the question why the memlock resource limit has to be lifted for all users by default. Have other options been considered? Could it be limited to a user group? i.e.: @libvmauser - memlock unlimited > > > [Service] > > > Type=forking > > > Restart=on-failure > > > ExecStart=/usr/sbin/vma start > > > ExecStop=/usr/sbin/vma stop > > > > /usr/sbin/vma is a SysV initscript. That's terrible. > > Why not start the deamon binary directly?: > > ExecStart=/usr/sbin/vmad > > (And maybe set KillSignal if needed.) > > It does support of some systems easier for us. Please elaborate. What systems? How exactly does running a SysV script from a systemd service help? I see the SysV script performs work that systemd already does by itself (checking whether the service is already running, finding a process to kill, reporting status). And it does it less accurately than systemd (which tracks services by cgroups). It also adds some "sleep 1", which is either pointless, or papering over a race condition bug. I tried running the daemon directly using "ExecStart=/usr/sbin/vmad" it the unit file. To my surprise the daemon exits immediately when run this way. This is because of this code in tools/daemon/daemon.c:main(): /* already a daemon */ if (getppid() == 1) { return 0; } That should be removed. Daemons should not change behaviour depending on which process spawned them. > > > RestartForceExitStatus=1 SIGTERM > > > > It's unusual to need to use this setting. There may be a good reason for it, > > but please double check. > > SIGTERM is generated by kill utility by default. So it is used to restart > vmad in this case too. Yes, kill sends SIGTERM by default. I don't understand how the second sentence follows. If I kill a service with SIGTERM, presumably I want it to terminate, don't I? -- 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://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedoraproject.org/archives/list/package-review@xxxxxxxxxxxxxxxxxxxxxxx
