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

Libshout intern #2040

Merged
merged 11 commits into from
Dec 17, 2019
Merged

Libshout intern #2040

merged 11 commits into from
Dec 17, 2019

Conversation

daschuer
Copy link
Member

@daschuer daschuer commented Feb 25, 2019

This PR adds libshout to the lib folder and uses it, if the systems libshout suffers
https://bugs.launchpad.net/mixxx/+bug/1833225

This will be also useful if we later switch to libshout-idjc. This way we can provide the patched library to distributions without idjc.

@daschuer daschuer mentioned this pull request Mar 14, 2019
@daschuer daschuer changed the base branch from master to 2.2 August 17, 2019 13:48
@daschuer
Copy link
Member Author

@uklotzde this branch is again relevant for fixing https://bugs.launchpad.net/mixxx/+bug/1833225
can you have a look? How is the Fedora situation?

@daschuer daschuer changed the title [WIP] Libshout intern Libshout intern Aug 29, 2019
@daschuer
Copy link
Member Author

More and more users are effected as the buggy libshout version spreads the world:
https://bugs.launchpad.net/bugs/1853179
Can one have a look?

@Holzhaus
Copy link
Member

I'll test this tomorrow. Can you also add this to CMakeLists.txt or should I open PR for your repo?

@daschuer
Copy link
Member Author

I would be happy if you care about the CMake integration. Thank you

build/features.py Outdated Show resolved Hide resolved
build/features.py Outdated Show resolved Hide resolved
@Holzhaus
Copy link
Member

I would be happy if you care about the CMake integration. Thank you

I'll do that later when the 2.2 branch has been merged into master again.

build/features.py Outdated Show resolved Hide resolved
build/features.py Outdated Show resolved Hide resolved
build/features.py Outdated Show resolved Hide resolved
build/features.py Outdated Show resolved Hide resolved
build/features.py Outdated Show resolved Hide resolved
@Holzhaus
Copy link
Member

I can report that this builds for me using SCons 3.1.1 when applying these patches:

diff --git a/build/depends.py b/build/depends.py
index 9d5efa2293..1663efb8c2 100644
--- a/build/depends.py
+++ b/build/depends.py
@@ -1304,7 +1304,7 @@ class MixxxCore(Feature):
             'preferences/dialog/dlgprefvinyldlg.ui',
             'preferences/dialog/dlgprefwaveformdlg.ui',
         ]
-        map(Qt.uic(build), ui_files)
+        list(map(Qt.uic(build), ui_files))

         if build.platform_is_windows:
             # Add Windows resource file with icons and such
diff --git a/build/features.py b/build/features.py
index 5fe3d24475..a68d94b31b 100644
--- a/build/features.py
+++ b/build/features.py
@@ -812,7 +812,7 @@ class LiveBroadcasting(Feature):

     def add_options(self, build, vars):
         vars.Add('shoutcast', 'Set to 1 to enable live broadcasting support', 1)
-   vars.Add('shoutcast_internal', 'Set to 1 to use internal libshout', 1)
+        vars.Add('shoutcast_internal', 'Set to 1 to use internal libshout', 1)

     def configure(self, build, conf):
         if not self.enabled(build):

I didn't try to actually start a live broadcast since I don't have a server, but since there aren't any actual code changes I guess it will work.

@uklotzde
Copy link
Contributor

We need a CMake integration before merging this.

@Holzhaus
Copy link
Member

This is for 2.2, there is no CMakeLists.txt in the 2.2 branch.

@Holzhaus Holzhaus added the build label Dec 5, 2019
@Holzhaus Holzhaus added this to the 2.2.x milestone Dec 5, 2019
@Holzhaus
Copy link
Member

Holzhaus commented Dec 5, 2019

Is this a candidate for 2.2.3-rc2? Or will this go into 2.2.4/2.3.0?

@daschuer
Copy link
Member Author

daschuer commented Dec 5, 2019

We are quite late to merge this in on one hand. On the other hand it is no new code essentially and it fixes broadcasting for users on effected OSs. So I am leaning to merge this. What do others think?

@Holzhaus
Copy link
Member

Holzhaus commented Dec 5, 2019

Since this should only cause build-time issues (if any), we can do a late merge IMHO. But please fix the review comments first (without fixing the mixed spaces/tabs error, I cannot build mixxx anymore).

@Holzhaus
Copy link
Member

Holzhaus commented Dec 6, 2019

This branch builds and Mixxx starts without issues. I can't test the libshout integration because I have no server, but since there are no business logic changes this should work. LGTM.

@uklotzde uklotzde modified the milestones: 2.2.x, 2.2.4 Dec 10, 2019
@Holzhaus
Copy link
Member

Merge?

@daschuer
Copy link
Member Author

Yes, but I a biased.

@uklotzde
Copy link
Contributor

I have restarted the macOS build. If it passes we could merge it. As far as I understand this should not have any side effects if you don't enable the new option explicitly.

@daschuer
Copy link
Member Author

Yes, no side effect on MacOs and Windows.

It should have the side effect Linux, that it defaults to the internal version, if the system provided version suffers the bug.

@uklotzde
Copy link
Contributor

Ok, so let's finally merge this and skip waiting for a successful Travis build.

@uklotzde
Copy link
Contributor

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants