Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BUILD(cmake): Improve install handling #6100

Merged
merged 17 commits into from
Jan 1, 2024

Conversation

Krzmbrzl
Copy link
Member

@Krzmbrzl Krzmbrzl commented Apr 8, 2023

Inside the auxiliary_files directory, we were determining install paths for certain files (some of this was also buggy) instead of doing all that in the dedicated script. This has been streamlined now, along with some general changes to the way these paths are determined/handled.

Additionally, there have been some issues concerning e.g. permissions of installed files, which has been fixed as well.

Fixes #6038
Fixes #6039
Fixes #6098

Checks

@Krzmbrzl Krzmbrzl added build Everything related to compiling/building the code release-management labels Apr 8, 2023
@Krzmbrzl
Copy link
Member Author

Krzmbrzl commented Apr 8, 2023

@dvzrv I could use your input on what to do with install paths determined via pkgconf, if the user has provided a custom install prefix.
Since the paths determined via pkgconf are absolute, the normal prefix handling of cmake does not apply to them and therefore they remain unaffected by custom CMAKE_INSTALL_PREFIXes.

What I did for now was to check whether a custom prefix was set (during the first time cmake runs) and if so, make the determined paths relative to that (stripping away what is considered the install prefix from the path and prepending the custom one).
I'm not sure whether this yields desirable results though. Maybe it may be better to default to our fallback values in case a custom install prefix was given?

If you want to inspect the determined paths, you can pass -Ddisplay-install-paths=ON to cmake (and then check its output).
Each of those paths can be changed by an according cmake argument, so it should always be possible to fine-tune where different components are installed to. It's only about the defaults in case the user passes a custom install prefix but doesn't perform further install customizations that the paths can become a bit wonky...

@DarkDefender
Copy link
Contributor

Hello! I just wanted to chime in and say that this fixed my packaging issues when I tried to package the never mumble-server on gentoo. I couldn't get the systemd files to install in the proper locations. But now with the new cmake variables, it seems to work like a charm!

Is there anything holding this up or are you simply still looking for feedback?

@DarkDefender
Copy link
Contributor

I did notice that the new mumble-server.tmpfiles file does not respect the MUMBLE_INSTALL_SYSCONFDIR setting as the path is hard coded in that file.

@Krzmbrzl
Copy link
Member Author

Krzmbrzl commented Jun 1, 2023

Is there anything holding this up or are you simply still looking for feedback?

I'm looking for feedback and I am currently not really happy how the pkgconf integration works. Using pkgconf yields absolute install paths, which breaks cmake's install handling via CMAKE_INSTALL_PREFIX and because of that also causes issues when trying to create packages via CPack.

I'm thinking of removing the pkgconf integration in our cmake files in favor of choosing a (widely) working default location relative to cmake's default install prefix. The new variables should then still allow packagers to get these files to the correct location in their specific distro 🤔

I did notice that the new mumble-server.tmpfiles file does not respect the MUMBLE_INSTALL_SYSCONFDIR setting as the path is hard coded in that file.

Indeed. That needs to be fixed, thanks 👍

@DarkDefender
Copy link
Contributor

DarkDefender commented Jun 1, 2023

Yeah, seems like a logical choice to remove the pkgconf stuff.

Two more things I stumbled upon while testing this out.

  1. Gentoo usually creates all the users/groups for services via the package manager.
    This usually means that any bundled files in MUMBLE_INSTALL_SYSUSERSDIR and MUMBLE_INSTALL_TMPFILESDIR gets removed in favor of the users/groups created by the package manager which already populates these folder with the appropriate files.

However not having a way to configure this is not a big of a deal as we can do a search and replace for the _mumble-server group/user in the service scripts and remove the extra .conf files that will not be needed. But it would of course be nice to be able to configure this via cmake. But I understand if this is a bit too niche.

  1. Because of legacy reasons (which might change soon) the name for the server .ini file is still expected to be murmur.ini on Gentoo. This is so users upgrading from older versions will be prompted if the default server configuration .ini file changed between versions. (And so they know they might need to do some work to get the server up and running again after an upgrade).

On my side when testning it seems like only the mumble-server.ini and MumbleServer.ice is installed into the MUMBLE_INSTALL_SYSCONFDIR. I'm not well versed with MumbleServer.ice, but I don't think it is a config file per se?

Before on Gentoo, the Ice file was manually installed into /usr/share/murmur/.

If there is only the mumble-server.ini file that is installed into the MUMBLE_INSTALL_SYSCONFDIR folder, then perhaps renaming it to MUMBLE_SERVER_INI_FILEPATH and having it specify the entire path including the name would be nice?

@Krzmbrzl
Copy link
Member Author

Krzmbrzl commented Jun 2, 2023

MumbleServer.ice is indeed not a config file. I think I oriented myself at what the Debian package does with this file and just copied that.
I'm not sure whether distros have a standard directory for auxiliary files that cam be used for RPC coding. I guess that one might even go so far as to suggest that this should be only part of a Mumble development package. But then again, that's probably not worth it 🤔

Maybe /usr/share seems like a reasonable choice though 👀

@DarkDefender
Copy link
Contributor

should be only part of a Mumble development package. But then again, that's probably not worth it

I agree that it is probably not worth it as it seems to be the only file of this category?
So having a development package with only this Ice file would be a bit overkill imo.

Maybe /usr/share seems like a reasonable choice though 

It seem reasonable to me, yes :) . Usually most packages chucks misc files into their /usr/share folder from what I can tell.
So I think it could be a good place for it.

@Krzmbrzl Krzmbrzl force-pushed the fix-pkgconf-interface branch 2 times, most recently from f7bc3c2 to 8d3f841 Compare December 29, 2023 19:51
@Krzmbrzl
Copy link
Member Author

@DarkDefender @dvzrv could you have another look at this PR? I have decided to go the regular CMAKE_INSTALL_PREFIX-compatible way by default, but there is now a new option use-pkgconf-install-paths that instead queries pkgconf for certain install paths. It is disabled by default though.

Furthermore, I think I have fixed all outstanding issues that @DarkDefender has discovered during their previous test.

I decided against implementing the seemingly Gentoo-specific configuration of what files to install in the first place. That seems more complicated to implement on my side than I imagine removing unnecessary files is on the packaging side.

If there is only the mumble-server.ini file that is installed into the MUMBLE_INSTALL_SYSCONFDIR folder, then perhaps renaming it to MUMBLE_SERVER_INI_FILEPATH and having it specify the entire path including the name would be nice?

There might be more config files in the future and having file-specific install paths doesn't seem scalable. Plus, I think renaming can easily be done as long as you know where the file can be found in the first place :)

Copy link
Member

@Hartmnt Hartmnt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Due to a lack of cmake knowledge I can only sanity check the commits, and that looks good to me :)

@DarkDefender
Copy link
Contributor

About the dbus config file removal:
I don't know when the sever dbus support were deprecated, but I think it might be best to remove the rest of the references to dbus in the config files and run scripts for murmur/mumble-server.
Because I expect that without this dbus config file in place, turning on dbus in the config file or using the run_scripts that uses dbus will not do anything meaningful.

Besides that nitpick, I don't see any issues what so ever on my end! :)
It seems quite easy to get all files I need to go where they need to be now.

LGTM

@DarkDefender
Copy link
Contributor

DarkDefender commented Dec 31, 2023

Just discovered an other nitpick, sorry.

Shouldn't the Ice file be installed into /usr/share/mumble-server and not the current /usr/share/mumble ?
This is so users don't mistake server specific files for mumble client files.
It doesn't seem like the client installs anything into the share directory currently, but I'm guessing it might in the future?

The incorrect use of this function caused calls to get_pkgconf_variable
to fail.

Fixes mumble-voip#6038
When inferring install paths from pkgconf, failures to do so should not
be silently ignored.

Fixes mumble-voip#6038
By using systemd-tmpfiles, we can let the system set the ownership of
the installed Mumble server INI file to be root and the group ownership
to be _mumble-server, which is the special user that will start the
server through the systemd service file. The group ownership ensures
that the server has read access to the INI file.
The script assumed that the server executable is installed to /usr/sbin,
but this is not necessarily the case.
Since the project's name appears in the cmake generated install path for
the doc directory and it these directory names are usually lowercase,
the project name was changed to be lowercase as well.
Instead of computing the install paths for auxiliary files inside the
respective directory, we now bundle the install path determination with
the rest of the paths inside the dedicated script.
If the config file was installed, then every user on the system running
this DBus service would have unlimited access to the Mumble server. This
is certainly not what we want and given that the DBus API to the server
is considered deprecated anyway, this commit simply removes the config
file from the Mumble source tree.

Fixes mumble-voip#6098
@Krzmbrzl
Copy link
Member Author

Krzmbrzl commented Jan 1, 2024

I don't know when the sever dbus support were deprecated, but I think it might be best to remove the rest of the references to dbus in the config files and run scripts for murmur/mumble-server.
Because I expect that without this dbus config file in place, turning on dbus in the config file or using the run_scripts that uses dbus will not do anything meaningful.

The way I understood #6098 the config file was only providing global, system-wide DBus access to the Mumble server. That is to say that without the config file I assume users can still establish a per-user-level DBus access to the server. But I have never really looked into DBus, so maybe I'm wrong.
Either way, I think I'll err on the side of being cautious and keeping the code in the server user wrapper.

Just discovered an other nitpick, sorry.

Nothing to feel sorry for. I essentially explicitly asked you to nitpick, so you're doing great :D

Shouldn't the Ice file be installed into /usr/share/mumble-server and not the current /usr/share/mumble ?
This is so users don't mistake server specific files for mumble client files.
It doesn't seem like the client installs anything into the share directory currently, but I'm guessing it might in the future?

Good point. Yeah, I guess being explicit that this is a server-specific file probably doesn't hurt 🤔 👍

@Krzmbrzl Krzmbrzl force-pushed the fix-pkgconf-interface branch from 8d3f841 to 6925837 Compare January 1, 2024 14:17
@Krzmbrzl Krzmbrzl merged commit 96f2581 into mumble-voip:master Jan 1, 2024
15 checks passed
@Krzmbrzl Krzmbrzl deleted the fix-pkgconf-interface branch January 1, 2024 15:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Everything related to compiling/building the code release-management
Projects
None yet
3 participants