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

macOS build fix #16

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

v-kiniv
Copy link

@v-kiniv v-kiniv commented Nov 11, 2023

I've encounter multiple issues while trying to build Sigrok packages using sigrok-util on a modern Mac:

  • Binaries and shared libs are not signed, this lead to application crash(it is now required to sign binary, after you've modified it with install_name_tool);
  • macdeployqt for some reason does not copy QtDBus and libdbus libraries to the application bundle(even though it's a dependency of QtGui);
  • You need to get into weeds to find out which version of python3(especially when multiple installed) was picked up by libsigrokdecode build script to set appropriate PYVER in create_dmg;
  • Compiling PulseView using Qt5.x produce so much errors, that I was not even trying to resolve it, I'm not sure it's supported anymore by the project for macOS and I couldn't find much information on that matter.
  • Tests for libsigrok crash with segfault, I traced the problem to libusb_init function.

In this PR I tried to make as less changes as possible but make it successfully compile and bundle app on Apple Silicon, so I added APPLE_SILICON and ENABLE_TESTS option, so if someone wants to build for old macOS they can do it by setting APPLE_SILICON to n and ENABLE_TESTS to y - old behaviour, but with fixes for python3 version detection.

Summary:

  • Fix building using Qt6
  • Add binary signing
  • Add missing libraries for PulseView app bundle
  • Add auto resolve for PYVER in create_dmg
  • Move contrib/pulseview and contrib/sigrok-cli wrappers inside create_dmg so it can dynamically set appropriate PYVER for wrapper.
  • I couldn't quickly resolve situation with crashing tests, seems like whole another journey with debugger is required to find out why libusb is causing segfault. Instead I added option ENABLE_TESTS and set it's disabled by default.

p.s. I was not sure what is better, leave old scripts in place and add new one, or modify old ones. I decided that the second option will be better to avoid duplication. If you think the first option is better, let me know.

p.s.s. APPLE_SILICON option is figurative, I think most fixes are relevant to Intel Mac running latest macOS, so read it as LATEST_MACOS.

Copy link
Contributor

@fenugrec fenugrec left a comment

Choose a reason for hiding this comment

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

let me preface this by stating that I'm not familiar with apple stuff and don't much care for it at all, so this is a very general take.

The first commit has a lot of things going on, e.g. deleting/replacing the 'helper' scripts sigrok-cli and pulseview, could that have been done as a separate commit ?

See also inline comments.

I had a list of other macos users but lost it; it would be important to get other opinions & testers for this.

# Use Qt6 if building for Apple Silicon
QTVER=qt@6
else
# We use Qt 5.5 in order to remain compatible with more versions of Mac OS X.
Copy link
Contributor

Choose a reason for hiding this comment

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

indentation is all over the place

Copy link
Author

Choose a reason for hiding this comment

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

You are reviewing changes from the first commit only, this was fixed in the second commit.

Copy link
Contributor

Choose a reason for hiding this comment

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

Then modify the first commit to be correct and not require a 'fix' ?

@@ -23,6 +23,9 @@ set -e
# The path where the installed sigrok libraries/binaries are located.
PREFIX=$HOME/sr_macosx

# Build for Apple Silicon (y/n)
APPLE_SILICON=y
Copy link
Contributor

@fenugrec fenugrec Nov 19, 2023

Choose a reason for hiding this comment

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

some more context in here would be useful about what "apple silicon" refers to, i.e. what generation / years / cpu type / OS version / whatever, for future refernce by non-apple-specialists or casual devs

Copy link
Author

@v-kiniv v-kiniv Nov 19, 2023

Choose a reason for hiding this comment

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

I tested this PR on macOS 14.1.1 / M1 CPU.

As I stated in description:

APPLE_SILICON option is figurative, I think most fixes are relevant to Intel Mac running latest macOS, so read it as LATEST_MACOS.

I believe it's relevant for macOS 11 and beyond. But since there's no mention in documentation what versions of macOS currently supported by sigrok-util/PulseView it's hard for me to delineate what versions is APPLE_SILICON=y or =n. That's why I added this changes as an option, and not permanent changes, so you can revert to original behaviour by setting APPLE_SILICON to n.

Copy link
Contributor

Choose a reason for hiding this comment

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

I tested this PR on macOS 14.1.1 / M1 CPU.

I believe it's relevant for macOS 11 and beyond.

That's a good start, and that kind of info should definitely not just be deep in here in a comment thread but at least at the top in the PR submission, commit message etc. Otherwise in 2-3 months when another macos user comes back with issues / fixes, then nobody knows what this apply to. This kind of information gets lost easily which is exactly why , as you say,

there's no mention in documentation what versions of macOS currently supported by sigrok-util/PulseView


# Path to Qt5 binaries.
QTBINDIR=`brew list $QTVER | grep bin | head -n 1 | xargs dirname`
QTTRANSLATIONSDIR=`brew --prefix $QTVER`/translations
QTTRANSLATIONSDIR=`brew --prefix $QTVER`/share/qt/translations
Copy link
Contributor

Choose a reason for hiding this comment

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

is this valid for both APPLE_SILICON =y and =n ?

Copy link
Author

Choose a reason for hiding this comment

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

Again,

you are reviewing changes from the first commit only, this was fixed in the second commit.

@v-kiniv
Copy link
Author

v-kiniv commented Nov 19, 2023

I'm not familiar with apple stuff and don't much care for it at all

Then modify the first commit to be correct and not require a 'fix' ?

What a tone. I will leave this PR as is, it may be useful for other people(who cares), or you can close/delete it.

@fenugrec
Copy link
Contributor

What a tone.

Eh, a bit curt, granted, but I don't have much time to spend on this (apparently nobody else, either), and I'm fairly certain this is not mergeable as-is. Other reviewers will be at least as 'severe', possibly a lot more. Just trying to help improve the submission.

If you have no intention of further reworking the PR, that's unfortunate but OK. On behalf of eventual macos users, thanks for publishing it anyway, but don't be surprised if it's left to 'rot' and never merged.

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.

2 participants