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

use Windows dependencies from vcpkg #3594

Closed
wants to merge 4 commits into from
Closed

Conversation

Be-ing
Copy link
Contributor

@Be-ing Be-ing commented Jan 24, 2021

No description provided.

@github-actions github-actions bot added the build label Jan 24, 2021
@Be-ing Be-ing force-pushed the vcpkg_deps branch 4 times, most recently from 63cddc5 to 5660e1e Compare January 24, 2021 22:40
Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we target this for 2.4? I am not feeling comfortable to change all dependency now just for a release.
We have enough open Windows related bugs. https://launchpad.net/mixxx/+milestone/2.3.0
And are still fighting to get the cmake settings for MSVC build right.
We don't know what will happens with this. Only in the best case it will have no regressions.

What is missing in the 2.3-j00019-PLATFORM-CONFIGURATION-static-55e94982-minimal

@Be-ing
Copy link
Contributor Author

Be-ing commented Jan 24, 2021

No. We did not just spend a month of work to not use this for 2.3.

Making a release with an unreproducible and unmaintainable set of dependencies is a bad idea. So is having some features not available on some OSes but not others.

@Be-ing
Copy link
Contributor Author

Be-ing commented Jan 24, 2021

What is missing in the 2.3-j00019-PLATFORM-CONFIGURATION-static-55e94982-minimal

libkeyfinder and libdjinterop, Qt 5.15 LTS

@daschuer
Copy link
Member

Updating to a new QT version just before a release for no reason is a bad idea.
Do we have an issue with Qt 5.14.2?
Ubuntu Groovy 20.10 uses the same.

Lib libkeyfinder and libdjinterop are added as source code to the download folder, right. This should work independent in the same way as on our other targets.

@Be-ing
Copy link
Contributor Author

Be-ing commented Jan 24, 2021

We cannot simply download libkeyfinder at build time. It depends on fftw which is not in the old build environment.

@daschuer
Copy link
Member

OK, but we can either download the fftw source as well or use the binaries from a vckg that contains only that.

@Be-ing
Copy link
Contributor Author

Be-ing commented Jan 24, 2021

That would be overcomplicated and make it hard for contributors building locally.

@Be-ing Be-ing force-pushed the vcpkg_deps branch 2 times, most recently from e3c6db2 to cf5a5ea Compare January 25, 2021 00:22
@Holzhaus
Copy link
Member

What is missing in the 2.3-j00019-PLATFORM-CONFIGURATION-static-55e94982-minimal

libkeyfinder and libdjinterop, Qt 5.15 LTS

Also Lilv and libmodplug, which are currently missing.

.github/workflows/build.yml Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
@Be-ing
Copy link
Contributor Author

Be-ing commented Jan 25, 2021

Build succeeded but DLLs are missing.

tools/windows_buildenv.bat Outdated Show resolved Hide resolved
@Be-ing
Copy link
Contributor Author

Be-ing commented Jan 26, 2021

It works!!! 🎉 🎉 🎉

@Be-ing
Copy link
Contributor Author

Be-ing commented Jan 28, 2021

So... is anyone going to press merge?

@daschuer
Copy link
Member

This will break broadcasting.

We have the following options,

Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The broadcast issue should be fixed first.

@Be-ing
Copy link
Contributor Author

Be-ing commented Jan 28, 2021

Enable the version 2.4.1 in the lib folder for building with windows

As long as we need this on Linux anyway, I'd prefer that.

@uklotzde
Copy link
Contributor

Please merge 2.3 to check if the internal libshout 2.4.1 is selected now.

@daschuer
Copy link
Member

The internal libshout does not build with windows currently. I have made libshout-idjc 2.4.1 usable with windows. That can be found here:
#3543

Alternative we can pick the libshout.lib from the existing build environment.

@Be-ing
Copy link
Contributor Author

Be-ing commented Jan 30, 2021

I'm not very comfortable rushing libshout-idj at the last minute. @daschuer can you fork https://github.com/Be-ing/icecast-libshout/tree/msvc apply the bug fix and make a pull request to our vcpkg fork to use your fix?

@daschuer
Copy link
Member

No, please use libshout 2.4.1 for Mixxx 2.3.
This is the known working version used during Beta.
Since have no access to a windows machine, I am not the right person to do it.

As I mentioned earlier, I prefer to not update all libraries at this time in our release process. So let's consider to retarget this PR to 2.4.

@Be-ing
Copy link
Contributor Author

Be-ing commented Jan 30, 2021

IIUC going back to libshout 2.4.1 will require a bit of work to get it building with CMake. I don't know if this would be any more or less work than fixing 2.4.5.

I'm not going in circles repeating why delaying this until 2.4 is a bad idea.

@Be-ing
Copy link
Contributor Author

Be-ing commented Jan 30, 2021

I'm pretty tired of dealing with this badly maintained libshout library whose maintainers don't care about our needs.

@Be-ing
Copy link
Contributor Author

Be-ing commented Jan 30, 2021

The internal libshout does not build with windows currently.

Why not? Can you please get it building on Windows?

I'd appreciate if someone who actually uses this feature takes responsibility for maintaining it. @Holzhaus and I have already put a month of work into getting vcpkg working. Neither of us use libshout and now we are told it is entirely broken and upstream has been entirely broken for 5 years?!?! And upstream ignores their issue tracker, has no infrastructure for submitting code changes, requires using a legacy chat system to get any contact with maintainers, and won't accept CMake support?!

This affects the macOS builds too because the macOS environment uses libshout 2.4.4. So I think we should unconditionally use our internal fork of libshout 2.4.1.

@uklotzde
Copy link
Contributor

Does the internal libshout 2.4.1 fork build on all platforms? Then let's use it and not waste any time and efforts.

@Be-ing
Copy link
Contributor Author

Be-ing commented Jan 30, 2021

I gave it a try and it didn't build on macOS. Someone else take care of this hot garbage library.

@Be-ing
Copy link
Contributor Author

Be-ing commented Jan 30, 2021

Since have no access to a windows machine, I am not the right person to do it.

Neither @Holzhaus nor I use Windows and we spent a month working on Windows support using VMs and GitHub Actions. You can get libshout building on Windows if you care about this feature.

@daschuer
Copy link
Member

I would go with libschout-idjc 2.4.1. It has some patches that have been integrated upstream in the mean time.
The work is already done here: #3543.
It builds on all platforms and has been proved on Windows and Linux.

I don't see a need to do more work and watch again the GitHub Workflow hourglass for hours.

We can disable the HE-AAC part if you think that is required, but since this is already a preference option not available by default, because we don't ship the library, it can remain IMHO.

@Be-ing
Copy link
Contributor Author

Be-ing commented Jan 30, 2021

Okay, let's go with that then. I am unsure about the legality of shipping binaries with an AAC encoder so I'd prefer to disable that feature.

@daschuer
Copy link
Member

I can prepare a branch for that.

@uklotzde
Copy link
Contributor

I need to disable AAC on RPMFusion as already discussed! Otherwise we would no longer qualify for the free repositories.

@Be-ing
Copy link
Contributor Author

Be-ing commented Jan 30, 2021

@uklotzde are you sure? I thought RPMFusion could ship code with free copyright licenses that had patent issues.

@uklotzde
Copy link
Contributor

@uklotzde are you sure? I thought RPMFusion could ship code with free copyright licenses that had patent issues.

rpmfusion/mixxx@51d139a#commitcomment-45819425

@Be-ing
Copy link
Contributor Author

Be-ing commented Feb 8, 2021

Closing in favor of #3615 with libshout-idjc

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.

6 participants