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

Build: Mac: Add universal binary and -dev build ad-hoc signing support #2808

Merged
merged 2 commits into from
Aug 28, 2022

Conversation

hoffie
Copy link
Member

@hoffie hoffie commented Aug 25, 2022

Short description of changes

  • Build: Mac: Add universal binary support

    This PR combines the x86_64 and arm64e CI jobs into a single job in order to have both binary results at hand. It then uses lipo to combine those two binaries in a single universal binary which runs on both architectures.
    This increases user-friendliness as users no longer have to worry about downloading the proper build for their architecture.

  • Build: Mac: Use ad-hoc signing when no developer certificate is available

    Previously, only (pre-)release builds were signed. This signing and notarization was only possible with a proper developer certificate. As the Jamulus project currently does not have one and has to rely on other developers doing the signing, we only go this route for (pre-)releases due to the manual effort required.
    With Mac M1 support, having a valid signature becomes more important as unsigned binaries can only be started after turning several knobs.

    This PR adds very basic ad-hoc signing which does not require any Apple account/certificate, but improves user experience for M1 users.

CHANGELOG: Build: Mac: Combined Intel & M1 builds into a single Universal binary and improved M1 -dev build user-friendliness by introducing ad-hoc signing support

Context: Fixes an issue?
Fixes: #2790
Fixes: #2791

Does this change need documentation? What needs to be documented and how?

Status of this Pull Request
Ready for review and testing.

What is missing until this pull request can be merged?

Checklist

  • I've verified that this Pull Request follows the general code principles
  • I tested my code and it does what I want
  • My code follows the style guide
  • I waited some time after this Pull Request was opened and all GitHub checks completed without errors.
  • I've filled all the content above

@hoffie hoffie added this to the Release 3.9.1 milestone Aug 25, 2022
@hoffie hoffie requested a review from ann0see August 25, 2022 20:30
# We need to prune all object files here in order to force re-compilation in the next pass which builds
# for another architecture.
# We need to keep the ${build_path}/ contents though as we will use those artifacts later.
make -f "${build_path}/Makefile" -C "${build_path}" clean
Copy link
Collaborator

@pljones pljones Aug 26, 2022

Choose a reason for hiding this comment

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

I'd do make distclean - it's more vicious. If you need to keep anything, move it somewhere safe like on line 56.

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'll try, I was worried about it being extra-correct and removing intermediate artifacts, but maybe it could work. I'll give it a try when I have time.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, as I feared: Simply running make distclean here fails, because it also removes the artifact which we want to keep:
https://github.com/jamulussoftware/jamulus/runs/8058626894?check_suite_focus=true#step:10:1464
fatal error: /Applications/Xcode_13.4.1.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/lipo: can't open input file: /Users/runner/work/jamulus/jamulus/build/Jamulus.app/Contents/MacOS/Jamulus.arch_* (No such file or directory)

Copy link
Member Author

Choose a reason for hiding this comment

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

If you need to keep anything, move it somewhere safe like on line 56.

I've now more or less implemented that -- I'm running make distclean right before the second (or later) pass' qmake (running it after qmake would possibly be incorrect, as qmake would have generated new cleanup instructions based on the next architecture).

This seems to work properly now:
https://github.com/jamulussoftware/jamulus/runs/8058721581?check_suite_focus=true#step:10:753

@pljones Are you fine with that?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, my standard build is

make distclean; qmake {optional params} && make j3 && make clean

except during a dev cycle, where I just do make a lot :). We don't need the make clean as the container will get dropped. And the way you've done it, the make distclean should always find a Makefile to work from.

Nice.

@ann0see
Copy link
Member

ann0see commented Aug 26, 2022

Intel Mac shows the App as "Universal" and client + Server work as expected.

Copy link
Member

@ann0see ann0see left a comment

Choose a reason for hiding this comment

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

Approving based on my tests. @pljones comments should be addressed before a merge.

@hoffie hoffie force-pushed the autobuild/mac-universal-arm64 branch 2 times, most recently from 5fd13b9 to d1d1bab Compare August 28, 2022 15:34
This combines the x86_64 and arm64e CI jobs into a single job in order
to have both binary results at hand. It then uses `lipo` to combine
those two binaries in a single universal binary which runs on both
architectures.
This increases user-friendliness as users no longer have to worry about
downloading the proper build for their architecture.

Fixes: jamulussoftware#2790
…able

Previously, only (pre-)release builds were signed. This signing and
notarization was only possible with a proper developer certificate. As
the Jamulus project currently does not have one and has to rely on other
developers doing the signing, we only go this route for (pre-)releases
due to the manual effort required.
With Mac M1 support, having a valid signature becomes more important as
unsigned binaries can only be started after turning several knobs.

This commit adds very basic ad-hoc signing which does not require any
Apple account/certificate, but improves user experience for M1 users.

Fixes: jamulussoftware#2791
@hoffie hoffie force-pushed the autobuild/mac-universal-arm64 branch from d1d1bab to 92dddf8 Compare August 28, 2022 16:10
@hoffie hoffie mentioned this pull request Aug 28, 2022
59 tasks
@hoffie hoffie merged commit 05f385c into jamulussoftware:master Aug 28, 2022
@ann0see ann0see added the needs documentation PRs requiring documentation changes or additions label Aug 28, 2022
@ann0see
Copy link
Member

ann0see commented Aug 29, 2022

@hoffie it seems as if the build created on master is not a universal build. It shows up as arm only, while the one I tested is universal.

@ann0see
Copy link
Member

ann0see commented Aug 29, 2022

Maybe the distclean thing broke it?

@hoffie
Copy link
Member Author

hoffie commented Aug 29, 2022

Meh, seems so. It's a universal build with a single architecture:
https://github.com/jamulussoftware/jamulus/runs/8071146815?check_suite_focus=true#step:10:1455

Will work on a fix shortly.

@ann0see
Copy link
Member

ann0see commented Aug 29, 2022

Great. Thanks jamulus_3.9.0dev-dead5c5_mac works as expected, that's the one I tested last.

hoffie added a commit to hoffie/jamulus that referenced this pull request Aug 29, 2022
One of the last-minute changes broke the universal build and resulted in
a universal build which only contained a single architecture.
The reason was that the added `distclean` action removed the artifact
for the first architecture.

This change moves the intermediate artifacts to a temporary, already
defined  directory which is definitely not touched by `make distclean`.

Related: jamulussoftware#2808 (comment)
Related: jamulussoftware#2808 (comment)
hoffie added a commit to hoffie/jamulus that referenced this pull request Aug 29, 2022
One of the last-minute changes broke the universal build and resulted in
a universal build which only contained a single architecture.
The reason was that the added `distclean` action removed the artifact
for the first architecture.

This change moves the intermediate artifacts to a temporary, already
defined directory which is definitely not touched by `make distclean`.

Related: jamulussoftware#2808 (comment)
Related: jamulussoftware#2808 (comment)
@pljones
Copy link
Collaborator

pljones commented Sep 7, 2022

jamulussoftware/jamuluswebsite#806 merged, so removing "Needs documentation".

@pljones pljones removed the needs documentation PRs requiring documentation changes or additions label Sep 7, 2022
ann0see pushed a commit to ann0see/jamulus that referenced this pull request Nov 9, 2022
One of the last-minute changes broke the universal build and resulted in
a universal build which only contained a single architecture.
The reason was that the added `distclean` action removed the artifact
for the first architecture.

This change moves the intermediate artifacts to a temporary, already
defined directory which is definitely not touched by `make distclean`.

Related: jamulussoftware#2808 (comment)
Related: jamulussoftware#2808 (comment)
This pull request was closed.
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.

Mac: Sign non-release builds with self-signed key Mac: Provide universal binaries for x86_64 & arm64e
3 participants