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

Restore libtimidity #7454

Closed
wants to merge 3 commits into from
Closed

Restore libtimidity #7454

wants to merge 3 commits into from

Conversation

@Milek7
Copy link

Milek7 commented Apr 1, 2019

extmidi doesn't work on platforms such as emscripten, and fluidsynth requires tens of dependencies (including glib)

@Milek7 Milek7 force-pushed the Milek7:libtimidity branch 2 times, most recently from 0418841 to a425d99 Apr 1, 2019
@LordAro

This comment has been minimized.

Copy link
Member

LordAro commented Apr 1, 2019

Also needs /* virtual */ converting into overrides where appropriate

@Milek7 Milek7 force-pushed the Milek7:libtimidity branch 2 times, most recently from 0462942 to e6498c5 Apr 1, 2019
@TrueBrain

This comment has been minimized.

Copy link
Member

TrueBrain commented Apr 1, 2019

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 :(

@Milek7

This comment has been minimized.

Copy link
Author

Milek7 commented Apr 1, 2019

Ok.
I have broken something in configure while reverting because it tries to compile it even without libtimidity detected, but I don't know why :/

@TrueBrain

This comment has been minimized.

Copy link
Member

TrueBrain commented Apr 2, 2019

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 :(

@glx22

This comment has been minimized.

Copy link
Contributor

glx22 commented Apr 2, 2019

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; }

This comment has been minimized.

Copy link
@michicc

michicc Apr 2, 2019

Member

The remove commit had this line:

if ($0 == "LIBTIMIDITY" && "'$libtimidity'" == "" ) { next; }

This comment has been minimized.

Copy link
@Milek7

Milek7 Apr 2, 2019

Author

With original line it doesn't use libtimidity even when detected. Commited fix for detect_pkg_config from TrueBrain.

This comment has been minimized.

Copy link
@Milek7

Milek7 Apr 3, 2019

Author

That was still wrong, "'$libtimidity_config'" == "" here and unchanged detect_pkg_config should be right.

@Milek7 Milek7 force-pushed the Milek7:libtimidity branch from e6498c5 to 386f0c5 Apr 2, 2019
Copy link
Member

TrueBrain left a comment

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 :)

@Milek7 Milek7 force-pushed the Milek7:libtimidity branch from 386f0c5 to f24f5cf Apr 3, 2019
@TrueBrain

This comment has been minimized.

Copy link
Member

TrueBrain commented Apr 13, 2019

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 :)

@TrueBrain TrueBrain closed this Apr 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.