-
-
Notifications
You must be signed in to change notification settings - Fork 15.1k
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
Add "Lightweight Music Server" application + service #147408
Conversation
As mentioned previously, the evaluation fails if
|
It doesn't matter here since the config file here is just a trivial set of key value pairs, of types int / bool / string. So either of toml or ini will work just fine. I just don't see why the ini format should fail. |
01d4d83
to
d60c16a
Compare
The I'll try to get back to this and review it tonight! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The module looks great! 👍 Someone else will come along and review the package shortly, I'm sure. Ping me if you don't hear anything back in the next few days.
2388de2
to
f87515b
Compare
Thanks! |
f87515b
to
aa4477e
Compare
# Docs not needed | ||
doxygen = null; | ||
# Graphics libraries not needed | ||
glew = null; libharu = null; pango = null; graphicsmagick = null; | ||
# LMS uses only sqlite | ||
firebird = null; libmysqlclient = null; postgresql = null; | ||
# Qt not used | ||
qt48Full = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really hacky and ? null
should be avoided as much as possible. Can we do this different?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what better way there is. 1) The wt derivation (in development/libraries/wt/default.nix
) does not provide a nicer mechanism to remove components and 2) I'm don't think that the wt derivation should be expanded, given that wt simply does not build the optional components if it doesn't find the required dependencies, and that cutting out optional dependencies is likely only useful on a system that isn't served by a binary cache (e.g. armv7l, on which those dependencies are broken half the time). Having the caller pass in null seems like a robust and minimal way to include or drop those optional portions.
Or, are you against providing the "useMinimalWt" option at all? I'm happy to remove that and keep it in my overlays; I included it because I thought others might find it useful.
patches = [ | ||
(writeText "hardcode-dependency-paths.patch" '' | ||
diff --git a/src/libs/av/impl/Transcoder.cpp b/src/libs/av/impl/Transcoder.cpp | ||
index 7efa044..16a6e4c 100644 | ||
--- a/src/libs/av/impl/Transcoder.cpp | ||
+++ b/src/libs/av/impl/Transcoder.cpp | ||
@@ -38,7 +38,7 @@ static std::filesystem::path ffmpegPath; | ||
void | ||
Transcoder::init() | ||
{ | ||
- ffmpegPath = Service<IConfig>::get()->getPath("ffmpeg-file", "/usr/bin/ffmpeg"); | ||
+ ffmpegPath = Service<IConfig>::get()->getPath("ffmpeg-file", "${ffmpeg.bin}/bin/ffmpeg"); | ||
if (!std::filesystem::exists(ffmpegPath)) | ||
throw Exception {"File '" + ffmpegPath.string() + "' does not exist!"}; | ||
} | ||
diff --git a/src/lms/main.cpp b/src/lms/main.cpp | ||
index 98c0cec..b60e26a 100644 | ||
--- a/src/lms/main.cpp | ||
+++ b/src/lms/main.cpp | ||
@@ -53,7 +53,7 @@ generateWtConfig(std::string execPath) | ||
const std::filesystem::path wtConfigPath {Service<IConfig>::get()->getPath("working-dir") / "wt_config.xml"}; | ||
const std::filesystem::path wtLogFilePath {Service<IConfig>::get()->getPath("log-file", "/var/log/lms.log")}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please write that into a file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not possible, because I need a reference to the ffmpeg
and wt
paths (which is the whole point of the patch).
Or am I missing something..?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to add: the author of lms did not provide for a command line option to set those paths, so the usual solution of writing a wrapper shell script won't work here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be cleaner to write the diff into a file with placeholders for the parts you need to modify and then use sed or something to fix them up at configure time. I think that's what Sandro meant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's possible, but seems way uglier than simply inlining the patch?
2ad8660
to
aa4477e
Compare
@aanderse hi Aaron, could use some of your help here to move things along. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aanderse hi Aaron, could use some of your help here to move things along. Thanks!
Certainly. Let me try my best here...
Written in C++, it has a low memory footprint and even comes with a | ||
recommendation engine. | ||
''; | ||
homepage = https://github.com/epoupon/lms; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since https://github.com/NixOS/rfcs/blob/master/rfcs/0045-deprecate-url-syntax.md these need to be quoted.
homepage = https://github.com/epoupon/lms; | ||
downloadPage = "https://github.com/epoupon/lms"; | ||
maintainers = with maintainers; [ arthur ]; | ||
license = licenses.gpl3; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gpl3
was deprecated a little while ago: please choose gpl3Plus
or gpl3Only
.
--- a/src/libs/av/impl/Transcoder.cpp | ||
+++ b/src/libs/av/impl/Transcoder.cpp | ||
@@ -38,7 +38,7 @@ static std::filesystem::path ffmpegPath; | ||
void | ||
Transcoder::init() | ||
{ | ||
- ffmpegPath = Service<IConfig>::get()->getPath("ffmpeg-file", "/usr/bin/ffmpeg"); | ||
+ ffmpegPath = Service<IConfig>::get()->getPath("ffmpeg-file", "${ffmpeg.bin}/bin/ffmpeg"); | ||
if (!std::filesystem::exists(ffmpegPath)) | ||
throw Exception {"File '" + ffmpegPath.string() + "' does not exist!"}; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to https://github.com/epoupon/lms/blob/835e705e09176797ce9f910640ac091c80b5f717/conf/lms.conf#L8 we could do this in the module instead:
services.lms.settings.ffmpeg-file = "${pkgs.ffmpeg_4.bin}/bin/ffmpeg";
Or is the issue that you want to be able to use this software without a configuration file?
- const std::filesystem::path wtResourcesPath {Service<IConfig>::get()->getPath("wt-resources", "/usr/share/Wt/resources")}; | ||
+ const std::filesystem::path wtResourcesPath {Service<IConfig>::get()->getPath("wt-resources", "${wt-lms}/share/Wt/resources")}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is everyone opposed to:
postPatch = ''
substituteInPlace src/lms/main.cpp \
--replace "/usr/share/Wt/resources" "${wt-lms}/share/Wt/resources" \
--replace "/usr/bin/ffmpeg" "${ffmpeg_4.bin}/bin/ffmpeg"
'';
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess there is a bit of a worry that it might accidentally match other lines than the ones in this patch? But then again those paths would never be correct on NixOS you have a fair point there.
Then again less patching is better so if we can get away with only a config tweak I'd just leave it entirely unpatched. Seems like the config file has an entry for the wt-resources
dir too.
I was wondering why this PR never got merged. I read the comments but didn't understand the final status. |
As #305308 was merged, this can be closed |
Follow up of #147206
Motivation for this change
It's a nice efficient music streaming server, especially suitable for resource constrained devices / servers. See https://github.com/epoupon/lms.
Things done
sandbox = true
set innix.conf
? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)nixos/doc/manual/md-to-db.sh
to update generated release notes