build: Add missed definition for `AM

hebasto commented at 11:54 am on February 1, 2024: member

This PR adds the missed definition for AM_OBJCXXFLAGS which has the same value as AM_CXXFLAGS.

Otherwise, the compiling flags used by Objective C++ (for .mm source files) differ from C++ ones including hardening, debug, warning etc options.

build: Add missed definition for `AM_OBJCXXFLAGS` 0620a663bd

hebasto added the label macOS on Feb 1, 2024

hebasto added the label Build system on Feb 1, 2024

DrahtBot commented at 11:54 am on February 1, 2024: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept NACK	fanquake

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

fanquake commented at 12:41 pm on February 1, 2024: member

 0   CXX      qt/libbitcoinqt_a-moc_bitcoin.o
 1qt/macnotificationhandler.mm:27:9: error: 'NSUserNotification' is deprecated: first deprecated in macOS 11.0 - All NSUserNotifications API should be replaced with UserNotifications.frameworks API [-Werror,-Wdeprecated-declarations]
 2        NSUserNotification* userNotification = [[NSUserNotification alloc] init];
 3        ^
 4/Applications/Xcode_15.0.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/System/Library/Frameworks/Foundation.framework/Headers/NSUserNotification.h:24:12: note: 'NSUserNotification' has been explicitly marked deprecated here
 5[@interface](/bitcoin-bitcoin/contributor/interface/) NSUserNotification : NSObject <NSCopying> {
 6           ^
 7qt/macnotificationhandler.mm:27:50: error: 'NSUserNotification' is deprecated: first deprecated in macOS 11.0 - All NSUserNotifications API should be replaced with UserNotifications.frameworks API [-Werror,-Wdeprecated-declarations]
 8        NSUserNotification* userNotification = [[NSUserNotification alloc] init];
 9                                                 ^
10/Applications/Xcode_15.0.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/System/Library/Frameworks/Foundation.framework/Headers/NSUserNotification.h:24:12: note: 'NSUserNotification' has been explicitly marked deprecated here
11[@interface](/bitcoin-bitcoin/contributor/interface/) NSUserNotification : NSObject <NSCopying> {
12           ^
13qt/macnotificationhandler.mm:30:11: error: 'NSUserNotificationCenter' is deprecated: first deprecated in macOS 11.0 - All NSUserNotifications API should be replaced with UserNotifications.frameworks API [-Werror,-Wdeprecated-declarations]
14        [[NSUserNotificationCenter defaultUserNotificationCenter] deliverNotification: userNotification];
15          ^
16/Applications/Xcode_15.0.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/System/Library/Frameworks/Foundation.framework/Headers/NSUserNotification.h:118:12: note: 'NSUserNotificationCenter' has been explicitly marked deprecated here
17[@interface](/bitcoin-bitcoin/contributor/interface/) NSUserNotificationCenter : NSObject {
18           ^
193 errors generated.

DrahtBot added the label CI failed on Feb 1, 2024

DrahtBot commented at 1:01 pm on February 1, 2024: contributor

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

Debug: https://github.com/bitcoin/bitcoin/runs/21106512462

theuni commented at 2:12 pm on February 1, 2024: member

This warning seems legitimate. Looks to me like we should modernize this code rather than ignoring it, no?

hebasto commented at 2:14 pm on February 1, 2024: member

This warning seems legitimate. Looks to me like we should modernize this code rather than ignoring it, no?

It is Qt 5 code base. I did not check it out, but I hope Qt 6 dropped using of a deprecated API.

theuni commented at 2:18 pm on February 1, 2024: member

This warning seems legitimate. Looks to me like we should modernize this code rather than ignoring it, no?

It is Qt 5 code base. I did not check it out, but I hope Qt 6 dropped using of a deprecated API.

I don’t understand what you mean. From what @fanquake pasted above, It’s in our macnotificationhandler.mm. A quick google says that we should switch to the UserNotifications.frameworks API instead. What part of this is qt specific?

Or are there warnings other than those 3?

fanquake commented at 2:20 pm on February 1, 2024: member

I don’t understand what you mean.

Me either. The code that is producing warnings is definitely in our source tree. I don’t see how this is related to the Qt version.

fanquake commented at 2:24 pm on February 1, 2024: member

I just had a look, and anyone compiling on macOS, with Qt 6.6.1, will still see these exact same deprecation warnings. So this isn’t solved by Qt 6.

DrahtBot removed the label CI failed on Feb 1, 2024

maflcko added the label DrahtBot Guix build requested on Feb 2, 2024

DrahtBot commented at 8:37 pm on February 2, 2024: contributor

Guix builds (on x86_64)

File	commit 5b8c5970bdfc817cac9b59f699925c4426c59b61(master)	commit 4084d51d9da812b7448a2ee4fbca314644086203(master and this pull)
SHA256SUMS.part	`57c6ab3b21eb3a09...`	`34bcb3d76d3c51ea...`
*-aarch64-linux-gnu-debug.tar.gz	`be53953269a8ac42...`	`1edc730395382a68...`
*-aarch64-linux-gnu.tar.gz	`41bb7a5fb14be431...`	`463ffa4d27b38625...`
*-arm-linux-gnueabihf-debug.tar.gz	`10e78e10babe7504...`	`60a63da268e2796f...`
*-arm-linux-gnueabihf.tar.gz	`fad8f69e29ddc87b...`	`4b18995b51124b7a...`
*-arm64-apple-darwin-unsigned.tar.gz	`57ae6228771a65a0...`	`c605c91e6cd37227...`
*-arm64-apple-darwin-unsigned.zip	`cf15574b2779ef73...`	`d0798669433be1ed...`
*-arm64-apple-darwin.tar.gz	`ca697a12e89eaa55...`	`0976a2a87c39f2d4...`
*-powerpc64-linux-gnu-debug.tar.gz	`943c67610f1d4702...`	`ac7346d812658ea0...`
*-powerpc64-linux-gnu.tar.gz	`cf28170ae00aee83...`	`0730586d154f4bbd...`
*-powerpc64le-linux-gnu-debug.tar.gz	`f71559e0658bec70...`	`e194e7651dc9280b...`
*-powerpc64le-linux-gnu.tar.gz	`0ea2ad98f028654f...`	`e88650f3b38f2566...`
*-riscv64-linux-gnu-debug.tar.gz	`1e87775ff9231cd0...`	`9c87eccd434cce7a...`
*-riscv64-linux-gnu.tar.gz	`a8612d1adb2155da...`	`b390015ca66ba9b4...`
*-x86_64-apple-darwin-unsigned.tar.gz	`bb8c8cbe3fea1fc8...`	`721b466228184ac0...`
*-x86_64-apple-darwin-unsigned.zip	`ac3b4965cb983977...`	`5d3647ab804e3daf...`
*-x86_64-apple-darwin.tar.gz	`203c8fb15312f7e4...`	`f434345bbe4c0471...`
*-x86_64-linux-gnu-debug.tar.gz	`a16d15d38419bce6...`	`ee4f337deb11450d...`
*-x86_64-linux-gnu.tar.gz	`13043b9495c8e848...`	`8bcce5dba4399776...`
*.tar.gz	`e84c2a3741aa5533...`	`0364d40cc2a74414...`
guix_build.log	`b2335404ab6a37db...`	`5d0b42b3813ca492...`
guix_build.log.diff		`1575c16e13428ca7...`

DrahtBot removed the label DrahtBot Guix build requested on Feb 2, 2024

epiccurious approved

theuni commented at 3:31 pm on February 12, 2024: member

@epiccurious What specifically are you ACKing here? Have you read the conversation above?

hebasto commented at 9:39 pm on February 12, 2024: member

This warning seems legitimate. Looks to me like we should modernize this code rather than ignoring it, no?

Last time I tried to replace NSUserNotificationCenter with UNUserNotificationCenter on macOS 10.15, I got the authorization error “Notifications are not allowed for this application”.

Now, the minimum supported macOS is 11.0. So it might be worth trying once more.

It is Qt 5 code base. I did not check it out, but I hope Qt 6 dropped using of a deprecated API.

I don’t understand what you mean. From what @fanquake pasted above, It’s in our macnotificationhandler.mm. A quick google says that we should switch to the UserNotifications.frameworks API instead. What part of this is qt specific?

I was waiting when Qt does the same replacement to prove that this is a thing. Alas. See QTBUG-110998.

UPD. Also an alternative approach was NACKed due to “the visible global menu icon”.

fanquake commented at 9:54 pm on February 12, 2024: member

I guess mark as draft or something else for now then? This can’t be merged as-is, because the comments in the config of files are not correct.

ci: Add `-Wno-error` flags for macOS jobs 17861b9cd5

hebasto force-pushed on Feb 12, 2024

hebasto commented at 9:59 pm on February 12, 2024: member

the comments in the config of files are not correct.

The comments have been adjusted.

hebasto commented at 4:20 pm on February 13, 2024: member

@theuni

This warning seems legitimate. Looks to me like we should modernize this code rather than ignoring it, no?

I agree with you. More details regarding our options to “modernize this code” are posted in the dedicated https://github.com/bitcoin-core/gui/issues/112.

However, the goal of this PR is to fix the build system to prevent porting this bug into the new CMake-based build system. For the context, please refer to https://github.com/hebasto/bitcoin/pull/84#discussion_r1469371615.

fanquake commented at 3:16 pm on February 26, 2024: member

NACK - this currently disables all compiler warning output in the CI for an entire release platform, rather than fixing GUI code? Not sure that is the right tradeoff.

fanquake referenced this in commit d3c76a2e87 on Mar 6, 2024

fanquake commented at 1:54 pm on March 6, 2024: member

Opened an alternative in #29577.

hebasto commented at 12:29 pm on March 7, 2024: member

Opened an alternative in #29577.

Closing in favour of #29577.

hebasto closed this on Mar 7, 2024

build: Add missed definition for AM_OBJCXXFLAGS #29362

Code Coverage

Reviews

Guix builds (on x86_64)

build: Add missed definition for `AM_OBJCXXFLAGS` #29362