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

Remove mingw-std-threads from 3rd party deps and use native libs/headers instead #7283

Merged
merged 2 commits into from
Jun 30, 2024

Conversation

FyiurAmron
Copy link
Contributor

@FyiurAmron FyiurAmron commented May 27, 2024

related discussion in #7256

The 24.04 update has been separated from this PR to #7358 , since I found out it's possible to easily switch to POSIX on 20.04 (and earlier as well I think).

@Rossmaxx Rossmaxx requested a review from sakertooth May 27, 2024 16:30
@Rossmaxx
Copy link
Contributor

From what I know, this PR will break MinGW compilation as MinGW standard library lacks the threading features.

@Rossmaxx
Copy link
Contributor

I'll run the CI tho, just to check.

@Rossmaxx
Copy link
Contributor

Rossmaxx commented May 27, 2024

I thought so. I'm converting this to draft instead of closing it for now.

Edit: just saw that it's already in draft

@FyiurAmron
Copy link
Contributor Author

FyiurAmron commented May 27, 2024

@Rossmaxx I created it as a draft precisely because the error is not in this repo, but in CI image itself. MinGW stdlib has pthreads for both Linux and Windows for about 10 years now, the CI image just needs to be configured to use it. I'm fixing the image in my fork as we speak, and I'm writing the necessary description of this problem in parallel as well. However, after fixing the CI image for MinGW, this will be actually the proper solution here.

@Rossmaxx
Copy link
Contributor

@messmerd is looking into the containers. They might be a fit for this review.

@Rossmaxx Rossmaxx requested review from messmerd and removed request for sakertooth May 27, 2024 16:59
@FyiurAmron
Copy link
Contributor Author

FyiurAmron commented May 27, 2024

@Rossmaxx @messmerd g++-mingw-w64-x86-64 is the package name (available e.g. here) - you can select it to go with either g++-mingw-w64-x86-64-win32 (currently used) or g++-mingw-w64-x86-64-posix (which is the wanted solution here).

For Ubuntu, the package should be named the same, for Focal it's https://packages.ubuntu.com/focal/g++-mingw-w64-x86-64 I guess, but I'm not seeing the POSIX package for Focal (edit: update-alternatives is needed for Focal). For Jammy, it's https://packages.ubuntu.com/jammy/g++-mingw-w64-x86-64-posix - however, there is no tobydox for anything > Focal. I'm searching for a replacement as we speak.

@Rossmaxx
Copy link
Contributor

Just a doubt, why are you doing the build.yml changes in the linux container instead of the mingw(32|64) containers (they are seperate containers)

@FyiurAmron
Copy link
Contributor Author

@Rossmaxx I'm just doing a tryout whether Focal has this lib at all, it doesn't really matter where I do it. I could as well download the image to my PC and get into container shell. I'm just doing this in parallel with other stuff so I'm doing what's most convenient for me. Those commits are tryouts and will be rolled back anyway.

@Rossmaxx
Copy link
Contributor

Ohh i see. I thought you mistook the linux and mingw containers.

@messmerd
Copy link
Member

Rather than updating our MinGW image again, I think I'd like to try a proper solution, which would be using a plain GitHub build runner and getting our MinGW dependencies from vcpkg. If I can get that to work, that would end our reliance on Docker images and tobydox's PPA entirely while also providing us with up-to-date dependencies.

@FyiurAmron
Copy link
Contributor Author

@messmerd sounds like a great idea. The only thing that would be required as a prerequisite for dropping mingw-std-threads then would be to add g++-mingw-w64-x86-64-posix (g++-mingw-w64-i686-posix for 32-bit container respectively if 32-bit is still needed) to the base packages. It's readily available for all the relevant distros/base images, and can be built from source and repackaged if that's needed for any reason.

I'm finding&fixing all the problems with current build to be 24.04-compatible in my fork, but honestly most of the stuff is OK, only some updates due to new GCC bitching about stuff and some problems with ZynAddSubFx, GCC-vs-G++ if I see it correctly (wrong compiler gets called for some reason).

@FyiurAmron
Copy link
Contributor Author

@messmerd @Rossmaxx FYI, the 64-bit compilation on 24.04 with mingw-std-threads removed just went through without errors, only minor fixes were required. I'm still trying to fix 32-bit for educational purposes, but I'd say it would be possible to bump the MinGW CI to 24.04 really easily once the mainline fixes required for GCC updates are merged. I'll do the rest of relevant PRs tomorrow, it's time to hit the sack for me now :)

@FyiurAmron FyiurAmron force-pushed the master branch 3 times, most recently from 0be1822 to 4684d71 Compare May 27, 2024 21:42
@messmerd
Copy link
Member

I've been experimenting with MinGW-w64 (with POSIX threads) + vcpkg on GitHub build runners here:
https://github.com/messmerd/lmms/tree/mingw-vcpkg-testing

It's been rough and I haven't been able to build all of the dependencies yet.
On my personal computer with Linux Mint 22.04, I've been able to build/install almost all of them, though I was having an issue with Qt. There was also a problem cross-compiling mp3lame but I was able to create a patch which I hope to merge upstream in vcpkg.

On the GitHub build-runners though, it's been less successful and I'm not sure why yet.

 and use native libs/headers instead
@FyiurAmron FyiurAmron force-pushed the master branch 4 times, most recently from 5a04a6b to 637d807 Compare June 29, 2024 13:10
@FyiurAmron FyiurAmron marked this pull request as ready for review June 29, 2024 13:23
@FyiurAmron
Copy link
Contributor Author

@Rossmaxx @tresf additional testing or comments welcome. I tried to keep the changeset as tight and atomic as possible.

@Rossmaxx
Copy link
Contributor

The only constraint here is the CI, and since CI passes, you have my approval.

@Rossmaxx
Copy link
Contributor

BTW, it would be a good idea to update the developer wiki too to reflect the changes.

@tresf
Copy link
Member

tresf commented Jun 29, 2024

BTW, it would be a good idea to update the developer wiki too to reflect the changes.

In my opinion, it's a prerequisite prior to merging.

@FyiurAmron
Copy link
Contributor Author

@tresf @Rossmaxx wiki updated. I understand this concerned the https://github.com/LMMS/lmms/wiki/Compiling & https://github.com/LMMS/lmms/wiki/Dependencies-Windows pages? BTW, I did some cleanup and restructuring there, and prepared for reintroduction of MSYS2 section (since it should work out-of-the-box after this PR is merged, at least it does at my box)

@tresf
Copy link
Member

tresf commented Jun 30, 2024

@FyiurAmron Thank you very much!

@tresf
Copy link
Member

tresf commented Jun 30, 2024

The only constraint here is the CI, and since CI passes, you have my approval.

There's also an unresolved conversation between @FyiurAmron and @DomClark. The code has changed as a result but I would like @DomClark to mark the conversation as resolved prior to merging.

@Rossmaxx
Copy link
Contributor

Rossmaxx commented Jun 30, 2024

@tresf didn't catch that.. hehe

@sakertooth
Copy link
Contributor

Also wanted to say thank you for this. Having to do the conditional __MINGW32__ include was an issue for the longest. I was planning to try and get rid of that somehow, but then I remembered that this PR exists. 👍

Copy link
Member

@messmerd messmerd left a comment

Choose a reason for hiding this comment

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

I didn't know it would be this easy to drop mingw-std-threads. Nice!

@messmerd messmerd merged commit edf6bf8 into LMMS:master Jun 30, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants