macOS build fix#16
Conversation
There was a problem hiding this comment.
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.
indentation is all over the place
There was a problem hiding this comment.
You are reviewing changes from the first commit only, this was fixed in the second commit.
There was a problem hiding this comment.
Then modify the first commit to be correct and not require a 'fix' ?
| PREFIX=$HOME/sr_macosx | ||
|
|
||
| # Build for Apple Silicon (y/n) | ||
| APPLE_SILICON=y |
There was a problem hiding this comment.
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.
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.
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 |
There was a problem hiding this comment.
is this valid for both APPLE_SILICON =y and =n ?
There was a problem hiding this comment.
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);macdeployqtfor some reason does not copyQtDBusandlibdbuslibraries to the application bundle(even though it's a dependency ofQtGui);libsigrokdecodebuild script to set appropriatePYVERincreate_dmg;libsigrokcrash with segfault, I traced the problem tolibusb_initfunction.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_SILICONandENABLE_TESTSoption, so if someone wants to build for old macOS they can do it by settingAPPLE_SILICONtonandENABLE_TESTStoy- old behaviour, but with fixes for python3 version detection.Summary:
PYVERincreate_dmgcontrib/pulseviewandcontrib/sigrok-cliwrappers insidecreate_dmgso it can dynamically set appropriatePYVERfor wrapper.libusbis causing segfault. Instead I added optionENABLE_TESTSand 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.