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

build: Add missed definition for AM_OBJCXXFLAGS #29362

Closed
wants to merge 2 commits into from

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented Feb 1, 2024

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.

@DrahtBot
Copy link
Contributor

DrahtBot commented Feb 1, 2024

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
Copy link
Member

fanquake commented Feb 1, 2024

   CXX      qt/libbitcoinqt_a-moc_bitcoin.o
qt/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]
        NSUserNotification* userNotification = [[NSUserNotification alloc] init];
        ^
/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
@interface NSUserNotification : NSObject <NSCopying> {
           ^
qt/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]
        NSUserNotification* userNotification = [[NSUserNotification alloc] init];
                                                 ^
/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
@interface NSUserNotification : NSObject <NSCopying> {
           ^
qt/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]
        [[NSUserNotificationCenter defaultUserNotificationCenter] deliverNotification: userNotification];
          ^
/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
@interface NSUserNotificationCenter : NSObject {
           ^
3 errors generated.

@DrahtBot
Copy link
Contributor

DrahtBot commented Feb 1, 2024

🚧 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
Copy link
Member

theuni commented Feb 1, 2024

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

@hebasto
Copy link
Member Author

hebasto commented Feb 1, 2024

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
Copy link
Member

theuni commented Feb 1, 2024

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
Copy link
Member

fanquake commented Feb 1, 2024

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
Copy link
Member

fanquake commented Feb 1, 2024

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
Copy link
Contributor

DrahtBot commented Feb 2, 2024

Guix builds (on x86_64)

File commit 5b8c597
(master)
commit 4084d51
(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...

@theuni
Copy link
Member

theuni commented Feb 12, 2024

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

@hebasto
Copy link
Member Author

hebasto commented Feb 12, 2024

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
Copy link
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.

@hebasto
Copy link
Member Author

hebasto commented Feb 12, 2024

the comments in the config of files are not correct.

The comments have been adjusted.

@hebasto
Copy link
Member Author

hebasto commented Feb 13, 2024

@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 bitcoin-core/gui#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 hebasto#84 (comment).

@fanquake
Copy link
Member

fanquake commented Feb 26, 2024

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 added a commit to fanquake/bitcoin that referenced this pull request Mar 6, 2024
These come from GUI code, and haven't/aren't being fixed, see discussion
in bitcoin-core/gui#112. For now, just ignore
them entirely. Note that this only applies to ObjCXX code, so will not
hide any relevant warnings coming from C or CXX code (and they would be
unlikely in any case).

Alternative to bitcoin#29362, which disables all compiler warnings, for macOS
builds in the CI.

Relevant output:
```bash
qt/macnotificationhandler.mm:27:9: warning: 'NSUserNotification' is deprecated: first deprecated in macOS 11.0 - All NSUserNotifications API should be replaced with UserNotifications.frameworks API [-Wdeprecated-declarations]
        NSUserNotification* userNotification = [[NSUserNotification alloc] init];
        ^
/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/System/Library/Frameworks/Foundation.framework/Headers/NSUserNotification.h:24:12: note: 'NSUserNotification' has been explicitly marked deprecated here
@interface NSUserNotification : NSObject <NSCopying> {
           ^
qt/macnotificationhandler.mm:27:50: warning: 'NSUserNotification' is deprecated: first deprecated in macOS 11.0 - All NSUserNotifications API should be replaced with UserNotifications.frameworks API [-Wdeprecated-declarations]
        NSUserNotification* userNotification = [[NSUserNotification alloc] init];
                                                 ^
/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/System/Library/Frameworks/Foundation.framework/Headers/NSUserNotification.h:24:12: note: 'NSUserNotification' has been explicitly marked deprecated here
@interface NSUserNotification : NSObject <NSCopying> {
           ^
qt/macnotificationhandler.mm:30:11: warning: 'NSUserNotificationCenter' is deprecated: first deprecated in macOS 11.0 - All NSUserNotifications API should be replaced with UserNotifications.frameworks API [-Wdeprecated-declarations]
        [[NSUserNotificationCenter defaultUserNotificationCenter] deliverNotification: userNotification];
          ^
/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/System/Library/Frameworks/Foundation.framework/Headers/NSUserNotification.h:118:12: note: 'NSUserNotificationCenter' has been explicitly marked deprecated here
@interface NSUserNotificationCenter : NSObject {
           ^
3 warnings generated.
```
@fanquake
Copy link
Member

fanquake commented Mar 6, 2024

Opened an alternative in #29577.

@hebasto
Copy link
Member Author

hebasto commented Mar 7, 2024

Opened an alternative in #29577.

Closing in favour of #29577.

@hebasto hebasto closed this Mar 7, 2024
fanquake added a commit to fanquake/bitcoin that referenced this pull request Mar 7, 2024
These come from GUI code, and haven't/aren't being fixed, see discussion
in bitcoin-core/gui#112. For now, just ignore
them entirely. Note that this only applies to ObjCXX code, so will not
hide any relevant warnings coming from C or CXX code (and they would be
unlikely in any case).

Alternative to bitcoin#29362, which disables all compiler warnings, for macOS
builds in the CI.

Relevant output:
```bash
qt/macnotificationhandler.mm:27:9: warning: 'NSUserNotification' is deprecated: first deprecated in macOS 11.0 - All NSUserNotifications API should be replaced with UserNotifications.frameworks API [-Wdeprecated-declarations]
        NSUserNotification* userNotification = [[NSUserNotification alloc] init];
        ^
/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/System/Library/Frameworks/Foundation.framework/Headers/NSUserNotification.h:24:12: note: 'NSUserNotification' has been explicitly marked deprecated here
@interface NSUserNotification : NSObject <NSCopying> {
           ^
qt/macnotificationhandler.mm:27:50: warning: 'NSUserNotification' is deprecated: first deprecated in macOS 11.0 - All NSUserNotifications API should be replaced with UserNotifications.frameworks API [-Wdeprecated-declarations]
        NSUserNotification* userNotification = [[NSUserNotification alloc] init];
                                                 ^
/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/System/Library/Frameworks/Foundation.framework/Headers/NSUserNotification.h:24:12: note: 'NSUserNotification' has been explicitly marked deprecated here
@interface NSUserNotification : NSObject <NSCopying> {
           ^
qt/macnotificationhandler.mm:30:11: warning: 'NSUserNotificationCenter' is deprecated: first deprecated in macOS 11.0 - All NSUserNotifications API should be replaced with UserNotifications.frameworks API [-Wdeprecated-declarations]
        [[NSUserNotificationCenter defaultUserNotificationCenter] deliverNotification: userNotification];
          ^
/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/System/Library/Frameworks/Foundation.framework/Headers/NSUserNotification.h:118:12: note: 'NSUserNotificationCenter' has been explicitly marked deprecated here
@interface NSUserNotificationCenter : NSObject {
           ^
3 warnings generated.
```
fanquake added a commit that referenced this pull request Mar 7, 2024
…+ macOS code

8b7630c build: ignore deprecated-declaration warnings in objc++ macOS code (fanquake)
bd8f035 build: Add missed definition for `AM_OBJCXXFLAGS` (Hennadii Stepanov)

Pull request description:

  These come from GUI code, and haven't/aren't being fixed, see discussion in bitcoin-core/gui#112. For now, just ignore them entirely. Note that this only applies to ObjCXX code, so will not hide any relevant warnings coming from C or CXX code (and they would be unlikely in any case).

  Alternative to #29362 (which disables all compiler warnings, for macOS builds in the CI). This PR includes one commit from that PR.

  Relevant output:
  ```bash
  qt/macnotificationhandler.mm:27:9: warning: 'NSUserNotification' is deprecated: first deprecated in macOS 11.0 - All NSUserNotifications API should be replaced with UserNotifications.frameworks API [-Wdeprecated-declarations]
          NSUserNotification* userNotification = [[NSUserNotification alloc] init];
          ^
  /Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/System/Library/Frameworks/Foundation.framework/Headers/NSUserNotification.h:24:12: note: 'NSUserNotification' has been explicitly marked deprecated here
  @interface NSUserNotification : NSObject <NSCopying> {
             ^
  qt/macnotificationhandler.mm:27:50: warning: 'NSUserNotification' is deprecated: first deprecated in macOS 11.0 - All NSUserNotifications API should be replaced with UserNotifications.frameworks API [-Wdeprecated-declarations]
          NSUserNotification* userNotification = [[NSUserNotification alloc] init];
                                                   ^
  /Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/System/Library/Frameworks/Foundation.framework/Headers/NSUserNotification.h:24:12: note: 'NSUserNotification' has been explicitly marked deprecated here
  @interface NSUserNotification : NSObject <NSCopying> {
             ^
  qt/macnotificationhandler.mm:30:11: warning: 'NSUserNotificationCenter' is deprecated: first deprecated in macOS 11.0 - All NSUserNotifications API should be replaced with UserNotifications.frameworks API [-Wdeprecated-declarations]
          [[NSUserNotificationCenter defaultUserNotificationCenter] deliverNotification: userNotification];
            ^
  /Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/System/Library/Frameworks/Foundation.framework/Headers/NSUserNotification.h:118:12: note: 'NSUserNotificationCenter' has been explicitly marked deprecated here
  @interface NSUserNotificationCenter : NSObject {
             ^
  3 warnings generated.
  ```

ACKs for top commit:
  hebasto:
    re-ACK 8b7630c.

Tree-SHA512: 2f1fec97d9aa46aa23989d9fb283fc574dff9dc8f44847bb273b8fcf942f56f64c6d93dfcd7af8fbb52bf152e0fe76818118ce416d8cec5de852c32b6697a243
kevkevinpal pushed a commit to kevkevinpal/bitcoin that referenced this pull request Mar 13, 2024
These come from GUI code, and haven't/aren't being fixed, see discussion
in bitcoin-core/gui#112. For now, just ignore
them entirely. Note that this only applies to ObjCXX code, so will not
hide any relevant warnings coming from C or CXX code (and they would be
unlikely in any case).

Alternative to bitcoin#29362, which disables all compiler warnings, for macOS
builds in the CI.

Relevant output:
```bash
qt/macnotificationhandler.mm:27:9: warning: 'NSUserNotification' is deprecated: first deprecated in macOS 11.0 - All NSUserNotifications API should be replaced with UserNotifications.frameworks API [-Wdeprecated-declarations]
        NSUserNotification* userNotification = [[NSUserNotification alloc] init];
        ^
/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/System/Library/Frameworks/Foundation.framework/Headers/NSUserNotification.h:24:12: note: 'NSUserNotification' has been explicitly marked deprecated here
@interface NSUserNotification : NSObject <NSCopying> {
           ^
qt/macnotificationhandler.mm:27:50: warning: 'NSUserNotification' is deprecated: first deprecated in macOS 11.0 - All NSUserNotifications API should be replaced with UserNotifications.frameworks API [-Wdeprecated-declarations]
        NSUserNotification* userNotification = [[NSUserNotification alloc] init];
                                                 ^
/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/System/Library/Frameworks/Foundation.framework/Headers/NSUserNotification.h:24:12: note: 'NSUserNotification' has been explicitly marked deprecated here
@interface NSUserNotification : NSObject <NSCopying> {
           ^
qt/macnotificationhandler.mm:30:11: warning: 'NSUserNotificationCenter' is deprecated: first deprecated in macOS 11.0 - All NSUserNotifications API should be replaced with UserNotifications.frameworks API [-Wdeprecated-declarations]
        [[NSUserNotificationCenter defaultUserNotificationCenter] deliverNotification: userNotification];
          ^
/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/System/Library/Frameworks/Foundation.framework/Headers/NSUserNotification.h:118:12: note: 'NSUserNotificationCenter' has been explicitly marked deprecated here
@interface NSUserNotificationCenter : NSObject {
           ^
3 warnings generated.
```
kevkevinpal pushed a commit to kevkevinpal/bitcoin that referenced this pull request Mar 13, 2024
These come from GUI code, and haven't/aren't being fixed, see discussion
in bitcoin-core/gui#112. For now, just ignore
them entirely. Note that this only applies to ObjCXX code, so will not
hide any relevant warnings coming from C or CXX code (and they would be
unlikely in any case).

Alternative to bitcoin#29362, which disables all compiler warnings, for macOS
builds in the CI.

Relevant output:
```bash
qt/macnotificationhandler.mm:27:9: warning: 'NSUserNotification' is deprecated: first deprecated in macOS 11.0 - All NSUserNotifications API should be replaced with UserNotifications.frameworks API [-Wdeprecated-declarations]
        NSUserNotification* userNotification = [[NSUserNotification alloc] init];
        ^
/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/System/Library/Frameworks/Foundation.framework/Headers/NSUserNotification.h:24:12: note: 'NSUserNotification' has been explicitly marked deprecated here
@interface NSUserNotification : NSObject <NSCopying> {
           ^
qt/macnotificationhandler.mm:27:50: warning: 'NSUserNotification' is deprecated: first deprecated in macOS 11.0 - All NSUserNotifications API should be replaced with UserNotifications.frameworks API [-Wdeprecated-declarations]
        NSUserNotification* userNotification = [[NSUserNotification alloc] init];
                                                 ^
/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/System/Library/Frameworks/Foundation.framework/Headers/NSUserNotification.h:24:12: note: 'NSUserNotification' has been explicitly marked deprecated here
@interface NSUserNotification : NSObject <NSCopying> {
           ^
qt/macnotificationhandler.mm:30:11: warning: 'NSUserNotificationCenter' is deprecated: first deprecated in macOS 11.0 - All NSUserNotifications API should be replaced with UserNotifications.frameworks API [-Wdeprecated-declarations]
        [[NSUserNotificationCenter defaultUserNotificationCenter] deliverNotification: userNotification];
          ^
/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/System/Library/Frameworks/Foundation.framework/Headers/NSUserNotification.h:118:12: note: 'NSUserNotificationCenter' has been explicitly marked deprecated here
@interface NSUserNotificationCenter : NSObject {
           ^
3 warnings generated.
```
Retropex pushed a commit to Retropex/bitcoin that referenced this pull request Mar 28, 2024
Retropex pushed a commit to Retropex/bitcoin that referenced this pull request Mar 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants