Enable modplug feature implicitly if available

[uklotzde]

Enabling the modplug feature by default was the wrong way. Instead, it should be enabled separately for each release build on the build server! Each, either internal or external, distribution channel has the choice to do so. implicitly if available.

[rryan]

Could also go with a "default on if present" approach, don't we do that for opus?

…

On Sat, Sep 7, 2019, 9:37 AM Uwe Klotz ***@***.***> wrote: Enabling the *modplug* feature by default was the wrong way. Instead, it should be enabled separately for each release build on the build server! Each either internal or external distribution channel has the choice to do so. ------------------------------ You can view, comment on, or merge this pull request online at: #2272 Commit Summary - Disable modplug feature by default File Changes - *M* build/features.py <https://github.com/mixxxdj/mixxx/pull/2272/files#diff-0> (7) Patch Links: - https://github.com/mixxxdj/mixxx/pull/2272.patch - https://github.com/mixxxdj/mixxx/pull/2272.diff — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#2272?email_source=notifications&email_token=AAAGPHYLTZ6BX7QEJVZKSLTQIPKC3A5CNFSM4IUQTDXKYY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4HJ6ZXOA>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAGPH6ZW4YCOYZTHINHJLTQIPKC3ANCNFSM4IUQTDXA> .

[uklotzde]

This would be the preferred solution. I didn't know that we already had a blueprint for this that I could copy.

[rrrapha]

I know, the "default on if present" approach is used for other flags too, but I am not a big fan of it.
This output is now ambigous:

Building with flags: asan=0 battery=1 buildtime=1 bulk=1 color=0 modplug=1 ...

I wouldn't expect that using the defaults is different than setting modplug=1 explicitly.
(It feels a bit like magic)

[daschuer]

Works like a charm, thank you. LGTM.

@rrrapha: Yes, you are right, you cannot distinguish now why the flag is set or not.
Fixing this is however out of scope of his PR. Do you have an idea how?

[rrrapha]

Fixing this is however out of scope of his PR. Do you have an idea how?

@daschuer: My preferred solution would be to remove the "default on if present" feature and not to distinguish between default flags and explicit flags.
(In case of modplug, I'd just disable it by default.)

As it is now, one has to specify all the flags explicitly to get reproducible builds.
If you run $ scons (using default flags) on different systems, the resulting binaries may differ depending on the installed packages. I find this potentially confusing, but I can live with it. ;)

[daschuer]

I think you describe a rare use case. Normally people are happy if Mixxx builds instantly from source without thinking about different flags and their meaning. So IMHO we should keep it by default.

How about add an additional flag that disables the auto-detect feature?

[rrrapha]

I think you describe a rare use case.

I don't think it is a rare use case. It makes it a little bit harder for linux/bsd package maintainers to get the dependencies right.
For example:
If the default flags are used, and modplug is not listed as a dependency of the mixxx package, there is a problem. The build will depend on the order of packages being built on the build server. If modplug is already present when mixxx is built, it is enabled, otherwise not.
(This is just a possible pitfall, not a 'real' problem, so just ignore if you are happy with the current approach)

How about add an additional flag that disables the auto-detect feature?

This would complicate things even more, i guess..

[daschuer]

Do we just need a warning than?
Warning: libmodplug not found, disable modplug support ..

[rrrapha]

The README has this text:

WARNING: We do not recommend building Mixxx with any
flags other than the defaults if you are going to use Mixxx
for live performance. Features that are disabled by default
may not compile or may be unstable. Be forewarned.

Maybe it could be mentioned here that not all defaults are enforced:

Not all of the default flags are not enforced. 
Some features are automatically enabled/disabled depending on
installed packages. If you want the build to be reproducible
(like in a package/port), it is safer to set the build flags explicitly.

..but this sounds like admitting that the "default on if present" approach is questionable. ;)

[Be-ing]

Builds are now failing on the build server. I am not sure if it is because of this or another recent PR. I have not been following the discussions about modplug closely.

[daschuer]

I have recently merged 2.2 into master.
I guess we meet to do the same for the build environment.

[uklotzde]

Should be unrelated as Qt5X11Extras seems to be missing?

The i386 build succeeded after fixing the Debian package. But this fix should also be unrelated.

[rryan]

Should be unrelated as Qt5X11Extras seems to be missing?

Sorry, I had to wipe the Ubuntu builder VM and restore from snapshot because it consumed all disk space on my system (this happens every few months). I updated the build chroots, but forgot to run the fabric "install_mixxx_dependencies" rule, so it did not have Qt5X11Extras installed.