mbuesch / razer

Razer device library and tools
http://bues.ch/h/razercfg
GNU General Public License v2.0
251 stars 50 forks source link

Cmake install issues during rpmbuild #87

Closed hevanaa closed 8 years ago

hevanaa commented 8 years ago

Using latest 0.37 version. I've updated an rpm spec file for Fedora 24 and everything works fine except that during "make install", it is also trying to run the udevadm and systemctl commands etc. This of course it is supposed to do when running make install manually as root, but not when make install is run by rpmbuild as user.

cmake_install.cmake contains for instance:

if(NOT CMAKE_INSTALL_COMPONENT OR "${CMAKE_INSTALL_COMPONENT}" STREQUAL "Unspecified")

                execute_process(COMMAND udevadm control --reload-rules RESULT_VARIABLE install_exec_res)
                if (NOT "${install_exec_res}" STREQUAL "0")
                        message(WARNING "WARNING: udevadm control --reload-rules failed: ${install_exec_res}
Please reload udev manually or reboot the system
")
                else (NOT "${install_exec_res}" STREQUAL "0")
                        message(STATUS "udevadm control --reload-rules: ${install_exec_res}")
                endif (NOT "${install_exec_res}" STREQUAL "0")

endif()

Is there a way to avoid running this during make install when creating an rpm? Is there some config options that can be added, e.g. cmake -D flags (sorry, I'm not familiar with cmake)?

Currently my spec file contains:

...
%build
%cmake .
%make_build %{?_smp_mflags}

%install
%make_install DESTDIR=%{buildroot}
...

The alternative is to leave out "make install" and instead install the required files separately into buildroot by the spec file, but of course it would be nice if make install could be used.

(Razerd and razercfg is running fine in Fedora 24. Qrazercfg is not, because there is no python3-pyside available in Fedora, but this is no big issue).

mbuesch commented 8 years ago

Thanks for your report. Why is this a problem? Yes, this will throw error messages, because it can't run these commands successfully, but the install should finish correctly anyway. For the Debian package this is not a problem.

hevanaa commented 8 years ago

The problem is that when running rpmbuild in Fedora, this triggers an authentication prompt asking for administrator password. This happens both in command line and in GUI. So "make install" can't be used in an automated build environment.

Edit. To be more clear. It is udevadm and systemctl that asks for admin password. I have to press Cancel (in GUI), but in command line it interrupts the build completely.

mbuesch commented 8 years ago

OK, fair enough. Feel free to propose a patch that fixes this.

hevanaa commented 8 years ago

I can't figure out a way to fix this without simply removing the commands from cmake (other than to not use make install and instead copy files manually in the rpmbuild). In my opinion system configuration (service reload) should not be done in a make install script. It is a platform and distribution specific thing that should be done by distribution specific installation scripts (dpkg, rpm etc.), if at all. If you really want this included in the source tarball, it is enough with instructions in the Readme file or a separate script to do system specific configuration. System configuration varies a lot between different distributions and you shouldn't just assume that errors are ignored from make install. For now I will use a patch for Fedora to remove the commands from cmake.

mbuesch commented 8 years ago

I can't figure out a way to fix this

We could use an environment variable, that the distribution build sets, to disable these steps. I will take patches/pull-requests for this.

it is enough with instructions in the Readme file

No, that is not enough. People kept mailing me and reporting "bugs" that the daemon "didn't work". With the introduction of these reload steps these complaints dropped down to zero. People don't read Readmes ;)

System configuration varies a lot between different distributions

Since systemd installing and reloading a daemon is pretty much the same on all distributions.

We want "make install" to install everything that is required for the software to run. Anything else will just confuse the users. Note that this sw tends to attract Linux newbies due to its nature. (Linux-nerds won't buy a lot of gaming mice). It's not a good idea to tell those people to cope with systemd/udev/etc on their own, if we can just do that stuff for them automatically with a "one liner" in the makefile. Not doing this will cause tears on their side and a full mailbox on my side.

hevanaa commented 8 years ago

I tried an environment variable and it seems to work. The RPM_BUILD_ROOT variable is only set when executed from rpmbuild, not when running cmake manually. Do you want me to create a proper patch or is the following OK?

--- razercfg-0.37/CMakeLists.txt    2016-08-10 12:51:46.000000000 +0300
+++ razercfg-0.37_rpm/CMakeLists.txt    2016-10-18 00:12:50.037564889 +0300
@@ -38,8 +38,10 @@
        PERMISSIONS OWNER_READ OWNER_WRITE
                GROUP_READ
                WORLD_READ)
-   install_exec_cmd("udevadm control --reload-rules"
-            "Please reload udev manually or reboot the system")
+   if (NOT DEFINED ENV{RPM_BUILD_ROOT})
+           install_exec_cmd("udevadm control --reload-rules"
+                           "Please reload udev manually or reboot the system")
+   endif (NOT DEFINED ENV{RPM_BUILD_ROOT})
 endif(UDEV_DIR)

 configure_file("pm-hook.sh.template" "pm-hook.sh" @ONLY)
@@ -64,10 +66,12 @@
        PERMISSIONS OWNER_READ OWNER_WRITE
                    GROUP_READ
                    WORLD_READ)
-   install_exec_cmd("systemctl --system daemon-reload"
-            "If you use systemd, please reload systemd manually or reboot the system")
-   install_exec_cmd("systemctl --system --force enable razerd.service"
-            "If you use systemd, enable razerd.service manually")
+   if (NOT DEFINED ENV{RPM_BUILD_ROOT})
+       install_exec_cmd("systemctl --system daemon-reload"
+                "If you use systemd, please reload systemd manually or reboot the system")
+       install_exec_cmd("systemctl --system --force enable razerd.service"
+                "If you use systemd, enable razerd.service manually")
+   endif (NOT DEFINED ENV{RPM_BUILD_ROOT})
 endif(SYSTEMD_UNIT_DIR)
mbuesch commented 8 years ago

Yes, this is fine. Could you create a pull request, please? Thanks.

mbuesch commented 8 years ago

fixed by https://github.com/mbuesch/razer/pull/89