shoutcast_internal always 0 bug in master

[daschuer]

This is a alternative solution for a KeyError in sources when configure` is skipped:
5a0e711

The commit is only in master so we must be remove it when merging 2.2 into master.

[rryan]

Huh, I'm confused. If shoutcast_internal=1 is in SCons.ARGUMENTS, util.get_flags should always use that value instead of returning the default value. enabled is supposed to be idempotent (because as you say, we call it multiple times).

[daschuer]

In this case we correct shoutcast_internal in case the system installed version is buggy:

mixxx/build/features.py

Line 843 in 78e39ce

build.flags['shoutcast_internal'] = 1

This correction is overwritten.

[daschuer]

This now also fixes: https://bugs.launchpad.net/mixxx/+bug/1833225
@rryan: will you have time to review this or should one else jump in?

[rryan]

In this case we correct shoutcast_internal in case the system installed version is buggy:

mixxx/build/features.py

Line 843 in 78e39ce

build.flags['shoutcast_internal'] = 1

Ah I see.. sorry I missed that. I think build.flags should only be used to store user decisions, not automatically determined state. The INTERNAL_LINK examples above for Vamp and HID might be better ways to accomplish this?

[daschuer]

In that case the flag reports shoutcast_internal=0 even though internal is used. This is even more misleading. We may fail the build if the flag is set wrong.

How about removing the shoutcast_internal flag as user option entirely?

[Holzhaus]

I don't know Scons, but from a user perspective I'd say the most straightforward way to configure this would be:

Configuration	Effect
shoutcast_internal not specified	enables shoutcast_internal if broken lib version is detected
shoutcast_internal=1	always use internal lib
shoutcast_internal=0	never use internal lib

[rryan]

I believe the PR as is has the same KeyError I was fixing in 5a0e711 -- it should happen if you run scons -h or anything that skips the configure step.

[rryan]

I don't know Scons, but from a user perspective I'd say the most straightforward way to configure this would be:

That makes sense to me -- I think you can get it with a mix of INTERNAL_LINK pattern above combined with a check for SCons.ARGUMENTS

e.g.

class LiveBroadcasting(Feature):
  BUGGY_LIBSHOUT_PRESENT = False

  def enabled(self, build):
    build.flags['shoutcast_internal'] = util.get_flags('shoutcast_internal', int(LiveBroadcasting.BUGGY_LIBSHOUT_PRESENT))

  def configure(self, build, conf):
    if buggy_shoutcast_present:
      LiveBroadcasting.BUGGY_LIBSHOUT_PRESENT = True
      build.flags['shoutcast_internal'] = util.get_flags('shoutcast_internal', int(LiveBroadcasting.BUGGY_LIBSHOUT_PRESENT))
    # User has the final say:
    if int(build.flags['shoutcast_internal']):
      # Setup the internal build.
      pass

  def sources(self, build):
    if int(build.flags['shoutcast_internal']):
      # Return internal sources.
      pass

[daschuer]

OK done. I have reverted back to the self.INTERNAL_LINK = True solution, because there is no use case for users to overwrite the default.

[Holzhaus]

Thinking about it, do package maintainers rebuild all dependant packages if a library is updated? If not, maybe we should always use internal libshout if the version is below 2.4.4 even if 2.4.1 is currently installed?

[daschuer]

They normally try to rip off the internal versions and prefere external versions to keep iso image size small. They like to apply a security patch only once.

In the current state, they have still the option for both. They are free to install the external dev package for libshout or not.

[uklotzde]

I agree with @Holzhaus that we always need to build and bundle the internal version until an official version with the fix is available in the distribution. We can only assume that the version during the build is the minimum version that is available, but not the other way round.

[daschuer]

OK, I don't really mind. If a maintainer does not like it he is free to alter the code.
I will change it.

[daschuer]

Done

[uklotzde]

Much cleaner now 👍 LGTM. Anyone objections?

[uklotzde]

The build failures on AppVeyor seem to be unrelated, but I'm not completely sure.

[rryan]

LGTM, thanks!

[Be-ing]

The commit is only in master so we must be remove it when merging 2.2 into master.

So why was this merged to 2.2? It seems to have broken the 2.2 build.

[daschuer]

This is required to a actually bulid the internal libshout instead if the broken system version of Ubuntu Eoan.

To clarify, the build succeeds, but bundeling the source code fails. We need to investigate why.

[Fluffkin]

Compiling stable on some distros is now broken again. libshout 2.4.5 cannot stream. The mixxx compile config checks for 2.4.4 and just uses the system lib otherwise, which is once again broken. Removing the use_internal option may well have been a mistake. Fixable by editing features.py and forcing mixxx to use internal but not ideal or documented.

[Swiftb0y]

considering 2.3 is pretty much deemed stable, have you tried compiling that instead? we dropped the old build system quite some time ago so I'm not sure if anyone would even bother to go back and fix a practically outdated version...

[Fluffkin]

Hi, yes I have. But that brings us to an unrelated point.
I have a self built version of "mixed in key" that I use to present a bunch of good candidates for the last loaded track. Uses librosa and a few tricks to make a pre-built database of music details to give suggestions in realtime. Works great with Mixxx 2.2 because I can parse the debug log and detect newly loaded tracks.

In 2.3 and 2.4 I can't yet see a clear way of detecting from the logs when a track has been successfully loaded to a deck. I find this odd. Once you turn on debugging log output there is a LOT of output. So not finding anything that says "Successfully loaded track <...> to deck <?>" seems a bit surreal. Since this renders several months of my hard work utterly useless. I guess I'm stuck with 2.2 for now. Ah technology.

[Swiftb0y]

I can understand that this is an issue for you, however you are building on top of a lot of hacks so I hope you can understand that we can't guarantee that your stuff will continue to work. There are several options you have for making your project work again:

1. you fix the issue and create PR and we'll consider to merge it (since 2.2 is practically unsupported anyways, I'm not sure how that PR will go).
2. You patch mixxx 2.3 so the log output matches what your application expects and maintain that fork yourself.
3. You can work of one the abandoned PRs which try to bring OSC support to mixxx so you can read metadata about the current track via the network.
4. You can try to compile expose current track metadata to external APIs #3483 which writes metadata about the current track to the filesystem so it can be read by external programms.
5. You port your suggestion feature to C++ and integrate it into mixxx and we'll take care of maintaining it for the future.

[Fluffkin]

Hi and thanks for the helpful reply to an entirely unrelated problem. (Apologies to anyone else subbed to this and rolling eyes). I'm fine with the hacky approach of scanning the logfile. Probably the easiest solution is I get around to suggesting a patch to include a line of logging on successful track load. (More likely I'll hack the 2.3 code because I'm lazy). I doubt there would be much resistance to one line of debug output? Trying to bolt on some kind of api seems a bit extreme. Thanks again.

[Swiftb0y]

I doubt there would be much resistance to one line of debug output?

I'm not sure, its still a hack which we don't really want to support, the logfile is not stable so we can't guarantee any stability when parsing it.

Trying to bolt on some kind of api seems a bit extreme.

Not really, we see the usecase and we want to provide a proper solution, the issue is that nobody found the time to create a mergeable PR. IIRC #3483 is scheduled for 2.4 though so thats why I'm recommending basing your work on that since its likely it will eventually get merged.

[Holzhaus]

Btw, although Mixed in Key still has superior key detected, try out 2.3 with KeyFinder support. It vastly improves key detection. Maybe that also fits your needs and doesn't require this hack at all?

[uklotzde]

@Swiftb0y I second that! Log files are not considered an API. Adding or modifying log messages just for the purpose of interoperability is not desired.

[Fluffkin]

@Holzhaus What I've written doesn't primarily key match although it does highlight suitable keys in the returned results. It uses chroma/contrast/rms/spectral rolloff and other comparable data sets. Much of the coding time was spent testing a vast number of possible ways of comparing music data to determine the most useful subset.

I hear the lack of appetite for a "log hack", but it leaves me with a simple decision. I already need a one line hack to be able to compile Mixxx 2.2. Adding a "personal hack" log file line to 2.3 or 2.4 is probably about 2 lines of code. I appreciate an api is desired and the right way to do things (at the moment I use drag and drop to get track selections in to mixxx), but it's been desired for 2.2, 2.3, and now there's a chance it might get included in 2.4. So it's 2 lines of "hack" code, or get involved in developing an api that may or may not make it in to 2.4, and may or may not provide the functionality I need. Thanks to all for your comments, and I'm almost sorry I mentioned it. Good luck with the #3483 merge.

One last thought though. If 2.2 can no longer reliably compile without "hacks" for most distros, and 2.3 is suggested as the next stable option in these comments, perhaps 2.3 should be moved to stable sooner rather than later?

[Swiftb0y]

What I've written doesn't primarily key match although it does highlight suitable keys in the returned results. It uses chroma/contrast/rms/spectral rolloff and other comparable data sets. Much of the coding time was spent testing a vast number of possible ways of comparing music data to determine the most useful subset.

This sounds quite interesting. Is your project open source? I'd love to know more about it.

One last thought though. If 2.2 can no longer reliably compile without "hacks" for most distros, and 2.3 is suggested as the next stable option in these comments, perhaps 2.3 should be moved to stable sooner rather than later?

We'd love to release it ASAP, but we don't want to ship broken releases and since we are just volunteers with limited time and resources, fixing severe bugs like this can be difficult and time consuming... To reiterate, if you are not suffering from known release-breaking bugs, 2.3 is stable, but there are some bugs we know about and we can't ship a release knowing the release will be broken for a not insignificant portion of users.

I guess your preferred solution for now is just reintroducing the 2 lines required for your logfile and maintaining that hack yourself.