-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
MAINT/BUILD: Clean up scripts directory #5838
Conversation
@davidebeatrici could you please check that the files that I deleted are indeed not needed anymore? I probably did not think of all use-cases 👀 |
63550b2
to
f29ca8d
Compare
f29ca8d
to
663d73c
Compare
025574d
to
d9890d3
Compare
594f9e9
to
04eb867
Compare
@dvzrv I think I have addressed all of your concerns now. Mind having another look at this PR to check whether I missed anything? |
9009420
to
bd971ae
Compare
Okay, here are the results of building the client:
I noticed a bunch of files that are not part of mumble and should not be installed (MS GSL? Tracy?). Not sure if that's due to this PR though:
Apart from the unrelated files this seems alright. Will have a look at the server next |
Yeah, that's unrelated from this PR. I noticed these issues in #5006 as well. Probably cmake automatically includes the install targets of linked-against targets as well 🤔 |
Hm, with the server I'm running into some issues:
|
Ah yes, the problem seems to be that the internal |
I now also fixed the additional, unrelated files getting installed, when installing Mumble. |
Alright, here are the results for building the server-side:
Here we can also see that additional, unwanted files are being installed, but I guess that's something for another PR?
All looks mainly fine so far, but the systemd service file points at a wrong location for the config and that the config is also located at the wrong location (we don't need the prefix there):
The sysusers.d file is also in the wrong location below It seems that the dbus file is still missing (on Arch it is currently located in |
See my latest commit - that should have fixed it
No, it's not. It has been removed due to our implementation being broken and unmaintained
By config file, I suppose you are referring to What is the rule as to when to use a prefix when writing files to
I'll fix that.
Ah yes indeed, I forgot about the DBus file. Is there also some pkgconf magic that can be used to fetch the correct DBus directory? |
The auxiliary files will now make use of configure_file in order to ensure that e.g. the binary names used in them always match the names of the actually built binaries.
Previously we would define our install paths as relative paths when using the packaging option and turning them into absolute paths otherwise. While relative paths are required for CPack to work properly, we use the paths e.g. for passing them in via macros and there we absolutely require the full, absolute paths to be passed. Thus, this commit removed the discrepancy of the results when built with packagin=ON vs packagin=OFF and now always defines the install paths as relative paths. However, we now keep an additional set of paths around that are now always absolute.
The logrotate script is not intended to be used on systems with systemd support. However, we don't want to take on the burden of having to keep around and maintain config files for non-systemd systems.
This essentially copied the file from https://github.com/archlinux/svntogit-community/blob/ba167269f8336659313d589dea65642b4eae4f85/trunk/murmur.service and only adapted it in as far as that it uses the actual install paths.
This prevents adding all targets in said subdirectories to the ALL target by default. Instead, only those targets that are explicitly referenced (e.g. via target_link_libraries) are built. This also has the side-effect that any install-directives in these subdirectories are also ignored for the default install targets which prevents these libraries of unconditionally installing themselves when issuing a make install. Instead, we now have to make any 3rdparty component that we do indeed want to install, do so explicitly (e.g. via install_library()).
317ec1d
to
b81b060
Compare
I'll merge this as-is now. If any of the install paths require further tuning, that can be done in a separate PR. |
With mumble-voip/mumble#5838 the file structure of the upstream repo has changed slightly and most importantly the location of the default server ini file has changed as well. Thus, we now have to check multiple locations inside our Dockerfile in order to make it work with the old and the new layout.
With mumble-voip/mumble#5838 the file structure of the upstream repo has changed slightly and most importantly the location of the default server ini file has changed as well. Thus, we now have to check multiple locations inside our Dockerfile in order to make it work with the old and the new layout.
PR mumble-voip#5838 commit 81bb8f7 removed the `docker-compose.yml` file in this repository. It points to our mumble-docker repository for docker related things. In that repository, we do not currently have a default or example docker compose configuration file. The README in that repository does have a description for it, so we link to that now. Update the relative file path reference `murmur.ini` -> `mumble-server-ini`. Update `database` path 'murmur' -> 'mumble-server'. I did not test or verify this change. Even if it is incorrect or does not work, I assume it to be *more correct*. Even if nobody chooses to test or verify it in this doc-update-changeset, I think that's preferable.
Docker documentation should be on our website (administrator/hoster documentation) or our [mumble-docker] repository. This repository /docs/ should only contain developer documentation and scoped to this source/main project. --- PR mumble-voip#5838 commit 81bb8f7 removed the docker-compose.yml file in this repository. The `murmur.ini` reference is outdated. It was moved and renamed to `umble-server-ini` The `database` path is outdated ('murmur' name). --- Replaces/Closes PR mumble-voip#6387 which intended to fix the documentation. [mumble-docker]: https://github.com/mumble-voip/mumble-docker/
Docker documentation should be on our website (administrator/hoster documentation) or our [mumble-docker] repository. This repository /docs/ should only contain developer documentation and scoped to this source/main project. --- PR mumble-voip#5838 commit 81bb8f7 removed the docker-compose.yml file in this repository. The `murmur.ini` reference is outdated. It was moved and renamed to `umble-server-ini` The `database` path is outdated ('murmur' name). --- Replaces/Closes PR mumble-voip#6387 which intended to fix the documentation. [mumble-docker]: https://github.com/mumble-voip/mumble-docker/
Inside
scripts
there were several entries that appear to have no use anymore or scripts that have been written in Pearl, which is not really usable in a cross-platform environment.This PR attempts to radically clean up the mess inside the
scripts
directory by deleting unneeded entries and by rewriting several scripts in Python.Furthermore, all scripts that are used to generate source files are now directly embedded into the build process in order to generate the source files on-the-fly instead of generating them once and then keeping them as part of the source tree.
Finally, non-script entries where moved to a new
auxiliary_files
directory. These files mostly got converted to make use of cmake'sconfigure_file
mechanism to replace certain things (as e.g. a binary's name) with the appropriate value. This has to be seen as a first step of resolving #5436.Checks