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

Mixxx for macOS on M1/M2 Apple silicon #11398

Merged
merged 10 commits into from
May 11, 2023
Merged

Conversation

daschuer
Copy link
Member

This is the final step to bring Apple silicon support to Mixxx

It also enables hash checks for the integrity of the windows environment and disposes the hard to find "build_environment" files in the packaging folder. It uses the release only environment that will free up some space in our workflow cache.

@github-actions github-actions bot added the build label Mar 22, 2023
@daschuer
Copy link
Member Author

All green.
We need to check all vcpkg based builds before merge.

@@ -1,6 +1,6 @@
@ECHO OFF
SETLOCAL ENABLEDELAYEDEXPANSION
REM this � is just to force some editors to recognize this file as ANSI, not UTF8.
Copy link
Member

Choose a reason for hiding this comment

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

The encoding of this file is broken - please make a binary diff.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed. This is always a pain. Not sure which editor here is broken.

Copy link
Member

Choose a reason for hiding this comment

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

GitHub Diff still marks this line as changed

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh sorry. This is fixed now and I have created #11513 in order that this will not happen again.

@@ -25,6 +21,10 @@ IF NOT DEFINED INSTALL_ROOT (
SET INSTALL_ROOT=%MIXXX_ROOT%\install
)

SET BUILDENV_NAME=mixxx-deps-2.4-x64-windows-8f8342a
SET BUILDENV_BRANCH=2.4-rel
Copy link
Member

@JoergAtGithub JoergAtGithub Mar 22, 2023

Choose a reason for hiding this comment

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

Our reviewed buildenv is in branch 2.4 not 2.4-rel.
I guess this will break the Debug builds for local Windows developers - not tried yet.
Downloading of the buildenv happens before the developer selects if he want to build Debug or Release.

Copy link
Member Author

Choose a reason for hiding this comment

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

You can still use the 2.4 branch for debugging on windows. Just download and replace the folder.
The 2.4-rel branch is the default for GitHub workflows because the debug libraries are not used there.
This means we get exact the same binary result if we use the 2.4 branch or the 2.4-rel branch.

Copy link
Member

Choose a reason for hiding this comment

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

Why do we need two source branches in Git to build two sets of binaries?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because the triplets files have the same name, but different content. mixxxdj/vcpkg@c2c93b5
We need also a different branch so that the packages are going into two locations named by the branch on https://downloads.mixxx.org/dependencies/2.4-rel/

Copy link
Member

Choose a reason for hiding this comment

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

If it's just the value of this one CMake variable VCPKG_BUILD_TYPE, please set it from outside.
Or, if this is not possible, generate the 5 files in overlay/triplets on the fly during the build of the VCPKG buildenvs.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have probably not understand what you want.

Do you want to create the release builds and the combined debug/release build from a single workflow run?

Copy link
Member

@JoergAtGithub JoergAtGithub Mar 23, 2023

Choose a reason for hiding this comment

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

I request two things:

  1. Compile all build environment ZIP-Files from the same VCPKG branch
  2. Use the full build environment ZIP-File (with Debug and Release DLLs) for local builds by default.

Copy link
Member Author

@daschuer daschuer Mar 23, 2023

Choose a reason for hiding this comment

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

  1. Does the opposite of the original intend. It will populate the cache during PR builds with the more code than with less. It is very convenient to develop with the release only version and reducing the build time to 3 hours instead of 5. That makes a difference.
  2. That means that every casual contributor on windows will need to download 1.3 GB instead of just 530 MB. Unpacked the environment consumes 5 GB instead of 3 GB. Why should that be the default? How about offer both options equal easy, that the user can make the choice?

Copy link
Member

Choose a reason for hiding this comment

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

The new approach addresses 2. well!

To make it safe, please make line

CALL :Configuration2CMakeSettings_JSON off Debug

conditional too.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@fwcd fwcd mentioned this pull request Mar 23, 2023
@fwcd
Copy link
Member

fwcd commented Mar 23, 2023

Nice, very excited to see official support for arm64 macOS moving forward 👍

From some quick smoke testing everything seems to work well. Interestingly this also solves fwcd/m1xxx#23 and correctly displays ARM64 in the about panel even though it's a cross-build, I must have missed something in my builds.

@daschuer daschuer force-pushed the arm64_macOS branch 4 times, most recently from 39c90c4 to d85e8f6 Compare March 26, 2023 23:45
@daschuer
Copy link
Member Author

I consider this now as ready to merge. The open topic for discussion, a joint vcpkg branch, is unrelated to this PR.

@JoergAtGithub
Copy link
Member

The open topic for discussion, a joint vcpkg branch, is unrelated to this PR.

I agree!

@daschuer
Copy link
Member Author

OK @JoergAtGithub, so we can merge this?

@JoergAtGithub
Copy link
Member

I've not tested the windows build yet. If this works as expected, I'm fine with the changes - but I can't test the MacOS stuff.

@@ -0,0 +1,2 @@
@ECHO OFF
set DEFINED BUILDENV_RELEASE=TRUE && call "%~dp0windows_buildenv.bat" %* & set DEFINED BUILDENV_RELEASE=
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
set DEFINED BUILDENV_RELEASE=TRUE && call "%~dp0windows_buildenv.bat" %* & set DEFINED BUILDENV_RELEASE=
set "BUILDENV_RELEASE=TRUE" && call "%~dp0windows_buildenv.bat" %* & set "BUILDENV_RELEASE="

This doesn't work for me. Are you sure, that CI used the release only buildenv?

@JoergAtGithub
Copy link
Member

Could you please add a suffix to the files in http://downloads.mixxx.org/dependencies/2.4-rel/Windows/ to distinguish them from http://downloads.mixxx.org/dependencies/2.4/Windows/
This name is also used for the subdirectory name under ./buildenv/ - I could imagine clashes, if a user switches from rel to full buildenv.

@daschuer
Copy link
Member Author

@JoergAtGithub this is now read to merge.

This updates 
* protobuf 
* libusb 
* libmad
* portaudio

See:
mixxxdj/vcpkg#66
@JoergAtGithub
Copy link
Member

Code looks good to me now, but I haven't tested it on Windows yet. Did anybody tested this on Macintosh?

@daschuer
Copy link
Member Author

daschuer commented May 5, 2023

The vcpkg environment has been tested here:
mixxxdj/vcpkg#66
The previous version has been also tested on a Mac with Apple Silicon but not the latest update. This does not include any special macOS code, so it should be safe.

@JoergAtGithub
Copy link
Member

I just tested this PR on Windwos11 and both build environments work for me!
I suppose that MacOS builds work too, but can't test it myself.
Thank you!

@JoergAtGithub JoergAtGithub merged commit 7c4d87b into mixxxdj:2.4 May 11, 2023
@daschuer daschuer added this to the 2.4.0 milestone May 12, 2023
@daschuer
Copy link
Member Author

Thank you ,🚀

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