epoupon / lms

Lightweight Music Server. Access your self-hosted music using a web interface.
http://lms-demo.poupon.dev
GNU General Public License v3.0
1.13k stars 62 forks source link

Set default path for lms.conf via a configure variable, not hardcode it in multiple places #525

Closed barracuda156 closed 1 month ago

barracuda156 commented 1 month ago

Currently the source hardcodes /etc/lms.conf in multiple files separately. On macOS /etc is a symlink to /private/etc; it is generally a bad idea to write anything into a prefix used by the OS. MacPorts uses /opt/local/etc by default, pkgsrc (I think) uses /opt/pkgsrc/etc or something similar.

With hardcoded path, binaries do not launch:

Caught exception: Cannot open config file '/etc/lms.conf'

It should probably be left to a user to choose desired prefix (and by default pick it form CMAKE_INSTALL_PREFIX). Or at least set it in one place, so that it is easy to patch.

barracuda156 commented 1 month ago

@epoupon Same problem with trying to write into system dirs, but I cannot see where /var/lms is actually set.

36-202% /opt/local/bin/lms
- 0x13ac408 [MAIN] locale set to 'en_US.utf-8'
Caught std::exception: filesystem error: cannot create directories: Permission denied [/var/lms/]
barracuda156 commented 1 month ago

Ah, that is in lms.conf.

epoupon commented 1 month ago

Indeed the only entry point is the configuration file. By default /etc/lms.conf is considered, but you can specify the configuration file to use by passing its path as the first argument to the program. Not sure CMAKE_INSTALL_PREFIX is a good choice, it would mean /usr/etc/lms.conf on linux distributions? Maybe this is more CMAKE_INSTALL_SYSCONFDIR? https://cmake.org/cmake/help/latest/module/GNUInstallDirs.html For now, the lms.conf file is provided as an example, it is up to package maintainers to copy and edit any relevant paths in it, and make the init script point to the lms.conf file

epoupon commented 1 month ago

Hello! Could you try ee87ceb and tell me if it is fine?

barracuda156 commented 1 month ago

@epoupon Sorry for a delay, I will check this today.

barracuda156 commented 1 month ago

@epoupon At least by default it does not pick the right directory:

36-242% lms
Caught std::exception: Cannot open config file '/etc/lms.conf'

What should I pass to configure to make it work? Currently lms.conf is installed into /opt/local/etc/.

epoupon commented 1 month ago

Hmm. By default it should be installed in sysco nfdir/share/lms.conf as it is just an example file you need to tweak as a package maintainer.

What is the value of the CMAKE_INSTALL_PREFIX you set?

barracuda156 commented 1 month ago

@epoupon This is what is done at the moment: https://github.com/macports/macports-ports/blob/762a4052c60759799d1f50a5b68f15b218140523/audio/lms/Portfile#L68-L78

So example goes into ${prefix}/share/lms/lms.conf, then it is copied to ${prefix}/etc/lms.conf, provided lms.conf does not already exist there. ${prefix}/etc/lms.conf was hardcoded via a patch (i.e. I just prepended MacPorts prefix to the existing paths).

CMAKE_INSTALL_PREFIX is /opt/local.

epoupon commented 1 month ago

Ah, I don't see https://github.com/epoupon/lms/commit/ee87ceb262e86c02ecb1252477f855b5c515932b in the patch list? Btw you could as well switch to 3.58.0 that includes it

barracuda156 commented 1 month ago

Ah, I don't see ee87ceb in the patch list? Btw you could as well switch to 3.58.0 that includes it

This is what I mean, I have built 3.58.0 with my patches dropped, and now it cannot find config file anymore :)

epoupon commented 1 month ago

Just realized I forgot to merge :( ... Can you test by manually applying this commit on top of 3.58.0?

barracuda156 commented 1 month ago

With the ee87ceb262e86c02ecb1252477f855b5c515932b patch applied, it fails because the path is confused: it is set to /etc/${prefix} instead of ${prefix}/etc:

36-242% lms
Caught std::exception: Cannot open config file '/etc/opt/local/lms.conf'
epoupon commented 1 month ago

According to https://cmake.org/cmake/help/latest/module/GNUInstallDirs.html#special-cases

For <dir> equal to SYSCONFDIR, LOCALSTATEDIR or RUNSTATEDIR, the CMAKE_INSTALL_FULL_<dir> is computed by appending the prefix to the value of CMAKE_INSTALL_<dir> if it is not user-specified as an absolute path. For example, the SYSCONFDIR value etc becomes /etc/opt/.... This is defined by the [Filesystem Hierarchy Standard](https://refspecs.linuxfoundation.org/FHS_3.0/fhs/index.html).

This behavior does not apply to paths under /opt/homebrew/....

If you set CMAKE_INSTALL_SYSCONFDIR to /opt/local/etc it should work

epoupon commented 1 month ago

Reopened since I am not happy with the proposed (hopefully not merged) solution

barracuda156 commented 1 month ago

@epoupon Sorry, I am away from the hardware for the time-being. What is the status of this? We wait for 3.59.0 to have it fixed correctly?

epoupon commented 1 month ago

For now, I just centralized the default path so that it is easier for you to patch

barracuda156 commented 1 week ago

Just for the record, build of 3.59.1 without patching the path, but with passing -DCMAKE_INSTALL_SYSCONFDIR=/opt/local/etc does not have a desired effect:

36-14% lms
Caught std::exception: Cannot open config file '/etc/lms.conf'
barracuda156 commented 1 week ago

But patching in a single file works, thank you!