-
Notifications
You must be signed in to change notification settings - Fork 46
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
base: master
Are you sure you want to change the base?
macOS build fix #16
Conversation
There was a problem hiding this 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. |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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
cross-compile/macosx/create_dmg
Outdated
|
||
# 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 |
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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.
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. |
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. |
I've encounter multiple issues while trying to build Sigrok packages using sigrok-util on a modern Mac:
install_name_tool
);macdeployqt
for some reason does not copyQtDBus
andlibdbus
libraries to the application bundle(even though it's a dependency ofQtGui
);libsigrokdecode
build script to set appropriatePYVER
increate_dmg
;libsigrok
crash with segfault, I traced the problem tolibusb_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
andENABLE_TESTS
option, so if someone wants to build for old macOS they can do it by settingAPPLE_SILICON
ton
andENABLE_TESTS
toy
- old behaviour, but with fixes for python3 version detection.Summary:
PYVER
increate_dmg
contrib/pulseview
andcontrib/sigrok-cli
wrappers insidecreate_dmg
so it can dynamically set appropriatePYVER
for wrapper.libusb
is causing segfault. Instead I added optionENABLE_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.