scripted-diff: replace deprecated Q_OS_MAC with Q_OS_MACOS #594

pull promag wants to merge 1 commits into bitcoin-core:master from promag:q_os_macos changing 10 files +34 −34

promag commented at 12:27 AM on April 26, 2022: contributor

Q_OS_MAC is deprecated but it is also defined when Qt is configured with -xplatform macx-ios-clang, and currently it guards some features not available on iOS, like QProcess.

scripted-diff: replace deprecated Q_OS_MAC with Q_OS_MACOS

-BEGIN VERIFY SCRIPT-
sed -i 's/Q_OS_MAC/Q_OS_MACOS/' $(git grep -l "Q_OS_MAC" src/qt)
-END VERIFY SCRIPT-

e3daecae03

jarolrod added the label macOS on Apr 26, 2022
jarolrod commented at 4:44 AM on April 26, 2022: member

Concept ACK

This change is ok to do. I've checked qt documentation up to Qt 5.7 that mentions Q_OS_MAC is deprecated and to not use. Qt 5.7 docs even mention to use Q_OS_MACOS or Q_OS_DARWIN if targeting all Darwin platforms (macOS, IOS).
promag commented at 5:36 PM on April 26, 2022: contributor

@fanquake should also rename this? https://github.com/bitcoin-core/gui/blob/833add0f48b0fad84d7b8cf9373a349e7aef20b4/build-aux/m4/bitcoin_qt.m4#L234
katesalazar commented at 6:09 PM on April 26, 2022: contributor

https://doc.qt.io/qt-5/qtglobal.html#Q_OS_MAC

Concept ACK
jarolrod commented at 7:03 AM on April 27, 2022: member

@fanquake should also rename this?

https://github.com/bitcoin-core/gui/blob/833add0f48b0fad84d7b8cf9373a349e7aef20b4/build-aux/m4/bitcoin_qt.m4#L234

yes, it would be appropriate to update what we want defined here. Should be opened up on the main repo though
jarolrod commented at 7:06 AM on April 27, 2022: member

tACK e3daecae0333e5474b54f64619ae6f512b683787
laanwj commented at 7:36 PM on April 27, 2022: member

Should some of these be Q_OS_DARWIN? Ping @Sjors he was building for iOS at some point (bitcoin/bitcoin#1255).
katesalazar commented at 7:39 PM on April 27, 2022: contributor

thought the same thing, didn't tell because I think they can sort those out later 🤔

On Wed, Apr 27, 2022 at 9:37 PM laanwj @.***> wrote:

Should some of these be Q_OS_DARWIN? Ping @Sjors https://github.com/Sjors he was building for iOS at some point (bitcoin/bitcoin#1255 https://github.com/bitcoin/bitcoin/pull/1255 ).

— Reply to this email directly, view it on GitHub https://github.com/bitcoin-core/gui/pull/594#issuecomment-1111405099, or unsubscribe https://github.com/notifications/unsubscribe-auth/AMRS4WYUEUMYG63OR5M4A4DVHGJN3ANCNFSM5UKFFNBQ . You are receiving this because you commented.Message ID: @.***>
laanwj commented at 7:42 PM on April 27, 2022: member

didn't tell because I think they can sort those out later thinking

I'm not sure there's such a hurry, might as well do it properly while we're changing this (and maybe it's not relevant at all, just thought I'd ask).
Sjors commented at 11:23 AM on April 28, 2022: member

I didn't get very far with iOs builds... Specifically I never even tried to compile QT.

However, the more generic Q_OS_DARWIN sounds good to me, unless it's about features that only exists on a Mac.

Update: it's actually not that clear cut, some of the code is really specific to macOS. Maybe just stick to Q_OS_MACOS until someone actually compiles QT for iOs.

in src/qt/bitcoingui.cpp:30 in e3daecae03

  25 | @@ -26,7 +26,7 @@
  26 |  #include <qt/walletview.h>
  27 |  #endif // ENABLE_WALLET
  28 |  
  29 | -#ifdef Q_OS_MAC
  30 | +#ifdef Q_OS_MACOS
  31 |  #include <qt/macdockiconhandler.h>

Sjors commented at 11:25 AM on April 28, 2022:

This is macOS specific, so Q_OS_MACOS makes sense.

hebasto approved
hebasto commented at 9:30 AM on May 20, 2022: member

ACK e3daecae0333e5474b54f64619ae6f512b683787.

Although Q_OS_DARWIN is a synonym for the deprecated Q_OS_MAC, I agree to narrow guards to Q_OS_MACOS as we do not currently support iOS, watchOS, or tvOS.
hebasto merged this on May 20, 2022
hebasto closed this on May 20, 2022
promag deleted the branch on May 20, 2022
sidhujag referenced this in commit 8434c7c5fc on May 28, 2022
bitcoin-core locked this on May 20, 2023

Contributors

Labels