-
-
Notifications
You must be signed in to change notification settings - Fork 957
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
Restore libtimidity #7454
Restore libtimidity #7454
Conversation
0418841
to
a425d99
Compare
Also needs |
0462942
to
e6498c5
Compare
I would like that we only accept this if we have some CI running this part of the code to validate it is still functioning correctly. Given there is no Debian port (which the CI runs), this might be a bit tricky. As mentioned on IRC, as this is part of EmScripten wasm port, possibly it is good to combine that: upstream wasm port, re-introduce libtimidity, and have a Docker compiling for wasm on every compile (or every release / night). That makes sure libtimidity keeps working. Without that, I am not sure we should accept this PR upstream. Having code that we cannot test is just asking for trouble :( |
Ok. |
I ... do not understand why it is broken. I would think too that a revert would "just work". Hmm .. puzzling. Looked at it for 10+ minutes, can't see what is wrong :( |
Same here, I fail to see why it tries to compile libtimidity.cpp when libtimidity is not found. The code clearly says it shouldn't. |
configure
Outdated
@@ -121,6 +121,7 @@ AWKCOMMAND=' | |||
"'$os'" != "CYGWIN" && "'$os'" != "MSVC") { next; } | |||
if ($0 == "MSVC" && "'$os'" != "MSVC") { next; } | |||
if ($0 == "DIRECTMUSIC" && "'$with_direct_music'" == "0") { next; } | |||
if ($0 == "LIBTIMIDITY" && "'$with_libtimidity'" == "0" ) { next; } |
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 remove commit had this line:
if ($0 == "LIBTIMIDITY" && "'$libtimidity'" == "" ) { next; }
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.
With original line it doesn't use libtimidity even when detected. Commited fix for detect_pkg_config from TrueBrain.
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 was still wrong, "'$libtimidity_config'" == ""
here and unchanged detect_pkg_config should be right.
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.
As requested, we should find a way for the CI to compile this. So possibly, this should be part of a bigger PR that introduces EmScripten support. This "request changes" purely for the reminder about this :)
If you don't mind, I am going to close this Pull Request. Not because I think it is bad, but because I would rather focus on bringing this in together with Emscripten (as mentioned earlier). I will also work on integrating that with the CI, so this library gets tested on a regular basis :) |
extmidi doesn't work on platforms such as emscripten, and fluidsynth requires tens of dependencies (including glib)