Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2070903002:
Add an XPC service to handle alert notifications on mac (Closed)
|
Created:
4 years, 6 months ago by Miguel Garcia Modified:
4 years, 2 months ago CC:
chromium-reviews, Peter Beverloo Base URL:
https://chromium.googlesource.com/chromium/src.git@move_builder_add_response Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd an XPC service to handle alert notifications on mac
BUG=571056
Committed: https://crrev.com/765f221e66c413ff3a50a52b4b1eb9172610c669
Cr-Commit-Position: refs/heads/master@{#423481}
Patch Set 1 : rebase #Patch Set 2 : rebased #
Total comments: 36
Patch Set 3 : review #
Total comments: 1
Patch Set 4 : review #Patch Set 5 #Dependent Patchsets: Messages
Total messages: 40 (21 generated)
First take at the XPC service, not ready for full review yet since all the BUILD/rules changes are essentially a copy & paste of the original prototype Robert sent us. Once the full support for XPC lands I will change them but thought it'd be good for you to have a look at the actual XPC service in the mean time.
The CQ bit was checked by miguelg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
miguelg@chromium.org changed reviewers: + peter@chromium.org, rsesek@chromium.org
This is now ready for review. Everything works module one small problem I am figuring out with Robert offline. Worth noting that since the XPC service is only generated on demand and the code that generates it is behind a compile time flag this code is only ever exercised if enable_xpc_notifications is true. However the code itself is not protected by the flag so it gets compile time coverage.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2070903002/diff/80001/chrome/BUILD.gn File chrome/BUILD.gn (right): https://codereview.chromium.org/2070903002/diff/80001/chrome/BUILD.gn#newcode... chrome/BUILD.gn:1009: ":chrome_framework_services", Adding this deps should be guarded by the buildflag. (The bundle_data target should probably be guarded as well). https://codereview.chromium.org/2070903002/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/notifications/BUILD.gn (right): https://codereview.chromium.org/2070903002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/notifications/BUILD.gn:14: [ "PRODUCT_BUNDLE_IDENTIFIER=$chrome_mac_bundle_id.$output_name" ] The convention for setting the bundle ID is to declare CHROME_BUNDLE_ID=$chrome_mac_bundle_id as the extra_substitutions and then in the .plist file either append a literal or ${EXECUTABLE_NAME}. https://codereview.chromium.org/2070903002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/notifications/BUILD.gn:29: "AppKit.framework", Do you need AppKit? https://codereview.chromium.org/2070903002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/notifications/BUILD.gn:56: public_deps = [ Leave this as deps and add an explicit deps on base where you need it (presumably in alert_notification_xpc_service). https://codereview.chromium.org/2070903002/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/notifications/alert_notification_service.mm (right): https://codereview.chromium.org/2070903002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/notifications/alert_notification_service.mm:7: #include "base/mac/scoped_nsobject.h" #import for both of these, since they're ObjC https://codereview.chromium.org/2070903002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/notifications/alert_notification_service.mm:10: @class NSUserNotification; Don't need these forward declarations since you import Foundation in the header. https://codereview.chromium.org/2070903002/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/notifications/notification_service_delegate.h (right): https://codereview.chromium.org/2070903002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/notifications/notification_service_delegate.h:13: @interface NotificationCenterDelegate There's no @implementation for this class? https://codereview.chromium.org/2070903002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/notifications/notification_service_delegate.h:19: : NSObject<NSXPCListenerDelegate, NSUserNotificationCenterDelegate> { Can omit the curlies here. https://codereview.chromium.org/2070903002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/notifications/notification_service_delegate.h:23: @property(assign, nullable) NSXPCConnection* connection; nonatomic as well https://codereview.chromium.org/2070903002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/notifications/notification_service_delegate.h:25: nit: extra blank line https://codereview.chromium.org/2070903002/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/notifications/notification_service_delegate.mm (right): https://codereview.chromium.org/2070903002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/notifications/notification_service_delegate.mm:5: #import <AppKit/AppKit.h> Do you need AppKit? https://codereview.chromium.org/2070903002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/notifications/notification_service_delegate.mm:6: #import <Foundation/Foundation.h> Foundation is already included in the .h. https://codereview.chromium.org/2070903002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/notifications/notification_service_delegate.mm:15: @class NSUserNotification; Remove forward decls. https://codereview.chromium.org/2070903002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/notifications/notification_service_delegate.mm:20: @synthesize connection = _connection; Use trailing underscores for ivar names. https://codereview.chromium.org/2070903002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/notifications/notification_service_delegate.mm:28: Should set the NSUserNotificationCenter to nil in dealloc. https://codereview.chromium.org/2070903002/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/notifications/xpc_service_main.mm (right): https://codereview.chromium.org/2070903002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/notifications/xpc_service_main.mm:5: #import <AppKit/AppKit.h> Appkit seems unused. https://codereview.chromium.org/2070903002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/notifications/xpc_service_main.mm:26: // displayed. Can we file a bug for this? Otherwise we leave a process hanging around.
Thanks for the review! PTAL I will hold adding a reviewer for the chrome/BUILD.gn file until we've clarified if I need to put it under the compile time flag or not. https://codereview.chromium.org/2070903002/diff/80001/chrome/BUILD.gn File chrome/BUILD.gn (right): https://codereview.chromium.org/2070903002/diff/80001/chrome/BUILD.gn#newcode... chrome/BUILD.gn:1009: ":chrome_framework_services", On 2016/09/29 14:23:04, Robert Sesek wrote: > Adding this deps should be guarded by the buildflag. (The bundle_data target > should probably be guarded as well). What problem do you see in adding this right now? If I guard this by the flag I lose compile coverage which is a pity. https://codereview.chromium.org/2070903002/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/notifications/BUILD.gn (right): https://codereview.chromium.org/2070903002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/notifications/BUILD.gn:14: [ "PRODUCT_BUNDLE_IDENTIFIER=$chrome_mac_bundle_id.$output_name" ] On 2016/09/29 14:23:04, Robert Sesek wrote: > The convention for setting the bundle ID is to declare > CHROME_BUNDLE_ID=$chrome_mac_bundle_id as the extra_substitutions and then in > the .plist file either append a literal or ${EXECUTABLE_NAME}. Ok, let's see if I got this right, in the .plist I went with <key>CFBundleIdentifier</key> <string>${CHROME_BUNDLE_ID}.AlertNotificationService</string> Since <string>${CHROME_BUNDLE_ID}.${output_name}</string> didn't seem to work https://codereview.chromium.org/2070903002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/notifications/BUILD.gn:29: "AppKit.framework", On 2016/09/29 14:23:04, Robert Sesek wrote: > Do you need AppKit? Yes (for NSImage). https://codereview.chromium.org/2070903002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/notifications/BUILD.gn:56: public_deps = [ On 2016/09/29 14:23:04, Robert Sesek wrote: > Leave this as deps and add an explicit deps on base where you need it > (presumably in alert_notification_xpc_service). Done. https://codereview.chromium.org/2070903002/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/notifications/alert_notification_service.mm (right): https://codereview.chromium.org/2070903002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/notifications/alert_notification_service.mm:7: #include "base/mac/scoped_nsobject.h" On 2016/09/29 14:23:05, Robert Sesek wrote: > #import for both of these, since they're ObjC Done. https://codereview.chromium.org/2070903002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/notifications/alert_notification_service.mm:10: @class NSUserNotification; On 2016/09/29 14:23:05, Robert Sesek wrote: > Don't need these forward declarations since you import Foundation in the header. I removed NSUserNotification but for NSUserNotificationCenter it still barfs with partially defined errors if I don't forward declare it. ../../chrome/browser/ui/cocoa/notifications/alert_notification_service.mm:21:5: error: 'NSUserNotificationCenter' is only available on macOS 10_8 or newer [-Werror,-Wunguarded-availability] [[NSUserNotificationCenter defaultUserNotificationCenter] ^~~~~~~~~~~~~~~~~~~~~~~~ /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.11.sdk/System/Library/Frameworks/Foundation.framework/Headers/NSUserNotification.h:116:12: note: 'NSUserNotificationCenter' has been explicitly marked partial here @interface NSUserNotificationCenter : NSObject { https://codereview.chromium.org/2070903002/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/notifications/notification_service_delegate.h (right): https://codereview.chromium.org/2070903002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/notifications/notification_service_delegate.h:13: @interface NotificationCenterDelegate On 2016/09/29 14:23:05, Robert Sesek wrote: > There's no @implementation for this class? Indeed, the ServiceDelegate below implements both interfaces. Removed this. https://codereview.chromium.org/2070903002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/notifications/notification_service_delegate.h:19: : NSObject<NSXPCListenerDelegate, NSUserNotificationCenterDelegate> { On 2016/09/29 14:23:05, Robert Sesek wrote: > Can omit the curlies here. Done. https://codereview.chromium.org/2070903002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/notifications/notification_service_delegate.h:23: @property(assign, nullable) NSXPCConnection* connection; On 2016/09/29 14:23:05, Robert Sesek wrote: > nonatomic as well Done. https://codereview.chromium.org/2070903002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/notifications/notification_service_delegate.h:25: On 2016/09/29 14:23:05, Robert Sesek wrote: > nit: extra blank line Done. https://codereview.chromium.org/2070903002/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/notifications/notification_service_delegate.mm (right): https://codereview.chromium.org/2070903002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/notifications/notification_service_delegate.mm:5: #import <AppKit/AppKit.h> On 2016/09/29 14:23:05, Robert Sesek wrote: > Do you need AppKit? For NSImage https://codereview.chromium.org/2070903002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/notifications/notification_service_delegate.mm:6: #import <Foundation/Foundation.h> On 2016/09/29 14:23:05, Robert Sesek wrote: > Foundation is already included in the .h. Done. https://codereview.chromium.org/2070903002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/notifications/notification_service_delegate.mm:15: @class NSUserNotification; On 2016/09/29 14:23:05, Robert Sesek wrote: > Remove forward decls. Same as before, need to keep NSUserNotificationCenter https://codereview.chromium.org/2070903002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/notifications/notification_service_delegate.mm:20: @synthesize connection = _connection; On 2016/09/29 14:23:05, Robert Sesek wrote: > Use trailing underscores for ivar names. Done. https://codereview.chromium.org/2070903002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/notifications/notification_service_delegate.mm:28: On 2016/09/29 14:23:05, Robert Sesek wrote: > Should set the NSUserNotificationCenter to nil in dealloc. I assume you mean the delegate? If so done. https://codereview.chromium.org/2070903002/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/notifications/xpc_service_main.mm (right): https://codereview.chromium.org/2070903002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/notifications/xpc_service_main.mm:5: #import <AppKit/AppKit.h> On 2016/09/29 14:23:05, Robert Sesek wrote: > Appkit seems unused. Done. https://codereview.chromium.org/2070903002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/notifications/xpc_service_main.mm:26: // displayed. On 2016/09/29 14:23:05, Robert Sesek wrote: > Can we file a bug for this? Otherwise we leave a process hanging around. Make sense, crbug.com/652237 made it block the main implementation bug (crbug.com/571056)
https://codereview.chromium.org/2070903002/diff/80001/chrome/BUILD.gn File chrome/BUILD.gn (right): https://codereview.chromium.org/2070903002/diff/80001/chrome/BUILD.gn#newcode... chrome/BUILD.gn:1009: ":chrome_framework_services", On 2016/10/03 16:07:50, Miguel Garcia wrote: > On 2016/09/29 14:23:04, Robert Sesek wrote: > > Adding this deps should be guarded by the buildflag. (The bundle_data target > > should probably be guarded as well). > > What problem do you see in adding this right now? If I guard this by the flag I > lose compile coverage which is a pity. This will start bundling the XPC service even in the case where it's not explicitly enabled via the compile-time flag. You could add the //chrome/browser/ui/cocoa/notifications:alert_notification_xpc_service to src/BUILD.gn I think. https://codereview.chromium.org/2070903002/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/notifications/BUILD.gn (right): https://codereview.chromium.org/2070903002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/notifications/BUILD.gn:14: [ "PRODUCT_BUNDLE_IDENTIFIER=$chrome_mac_bundle_id.$output_name" ] On 2016/10/03 16:07:50, Miguel Garcia wrote: > On 2016/09/29 14:23:04, Robert Sesek wrote: > > The convention for setting the bundle ID is to declare > > CHROME_BUNDLE_ID=$chrome_mac_bundle_id as the extra_substitutions and then in > > the .plist file either append a literal or ${EXECUTABLE_NAME}. > > Ok, let's see if I got this right, in the .plist I went with > > <key>CFBundleIdentifier</key> > <string>${CHROME_BUNDLE_ID}.AlertNotificationService</string> > > Since <string>${CHROME_BUNDLE_ID}.${output_name}</string> didn't seem to work Yes, that works. ${output_name} won't work but ${EXECUTABLE_NAME} will. https://codereview.chromium.org/2070903002/diff/100001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/notifications/notification_service_delegate.mm (right): https://codereview.chromium.org/2070903002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/notifications/notification_service_delegate.mm:21: if (self = [super init]) { nit: two sets of parens is convention to silence assignment warnings, i.e., |if ((self = [super init])) {|.
The CQ bit was checked by miguelg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/10/04 16:04:26, Robert Sesek wrote: > https://codereview.chromium.org/2070903002/diff/80001/chrome/BUILD.gn > File chrome/BUILD.gn (right): > > https://codereview.chromium.org/2070903002/diff/80001/chrome/BUILD.gn#newcode... > chrome/BUILD.gn:1009: ":chrome_framework_services", > On 2016/10/03 16:07:50, Miguel Garcia wrote: > > On 2016/09/29 14:23:04, Robert Sesek wrote: > > > Adding this deps should be guarded by the buildflag. (The bundle_data target > > > should probably be guarded as well). > > > > What problem do you see in adding this right now? If I guard this by the flag > I > > lose compile coverage which is a pity. > > This will start bundling the XPC service even in the case where it's not > explicitly enabled via the compile-time flag. > > You could add the > //chrome/browser/ui/cocoa/notifications:alert_notification_xpc_service to > src/BUILD.gn I think. Got it, please have a quick look at the new changes in the two BUILD.gn and if they look reasonable I'll add a global reviewer > > https://codereview.chromium.org/2070903002/diff/80001/chrome/browser/ui/cocoa... > File chrome/browser/ui/cocoa/notifications/BUILD.gn (right): > > https://codereview.chromium.org/2070903002/diff/80001/chrome/browser/ui/cocoa... > chrome/browser/ui/cocoa/notifications/BUILD.gn:14: [ > "PRODUCT_BUNDLE_IDENTIFIER=$chrome_mac_bundle_id.$output_name" ] > On 2016/10/03 16:07:50, Miguel Garcia wrote: > > On 2016/09/29 14:23:04, Robert Sesek wrote: > > > The convention for setting the bundle ID is to declare > > > CHROME_BUNDLE_ID=$chrome_mac_bundle_id as the extra_substitutions and then > in > > > the .plist file either append a literal or ${EXECUTABLE_NAME}. > > > > Ok, let's see if I got this right, in the .plist I went with > > > > <key>CFBundleIdentifier</key> > > <string>${CHROME_BUNDLE_ID}.AlertNotificationService</string> > > > > Since <string>${CHROME_BUNDLE_ID}.${output_name}</string> didn't seem to work > > Yes, that works. ${output_name} won't work but ${EXECUTABLE_NAME} will. Ah ok switched to ${CHROME_BUNDLE_ID}.${EXECUTABLE_NAME} in case we ever change the latter. > > https://codereview.chromium.org/2070903002/diff/100001/chrome/browser/ui/coco... > File chrome/browser/ui/cocoa/notifications/notification_service_delegate.mm > (right): > > https://codereview.chromium.org/2070903002/diff/100001/chrome/browser/ui/coco... > chrome/browser/ui/cocoa/notifications/notification_service_delegate.mm:21: if > (self = [super init]) { > nit: two sets of parens is convention to silence assignment warnings, i.e., |if > ((self = [super init])) {|. Done
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM
miguelg@chromium.org changed reviewers: + thakis@chromium.org
+thakis for the BUILD.gn files
The CQ bit was checked by miguelg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
build.gn changes lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
miguelg@chromium.org changed reviewers: - peter@chromium.org
Since I have a few patches pending on this and got all required LGTMs I am going to land it. Peter feel free to add comments and I will be sure to address them in a further CL.
The CQ bit was checked by miguelg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org Link to the patchset: https://chromiumcodereview.appspot.com/2070903002/#ps140001 (title: "")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #5 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Add an XPC service to handle alert notifications on mac BUG=571056 ========== to ========== Add an XPC service to handle alert notifications on mac BUG=571056 Committed: https://crrev.com/765f221e66c413ff3a50a52b4b1eb9172610c669 Cr-Commit-Position: refs/heads/master@{#423481} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/765f221e66c413ff3a50a52b4b1eb9172610c669 Cr-Commit-Position: refs/heads/master@{#423481}
Message was sent while issue was closed.
On 2016/10/06 09:04:32, commit-bot: I haz the power wrote: > Patchset 5 (id:??) landed as > https://crrev.com/765f221e66c413ff3a50a52b4b1eb9172610c669 > Cr-Commit-Position: refs/heads/master@{#423481} Hi Miguel, it look like this change caused a build failure on the ClangToTMac bot, e.g. https://build.chromium.org/p/chromium.fyi/builders/ClangToTMac/builds/11539 Can you please take a look? (This doesn't look like a clang ToT issue, I can't seem to find any other Mac bot that builds the "all" target though.)
Message was sent while issue was closed.
This one builds 'all': https://build.chromium.org/p/chromium/builders/Mac On Thu, Oct 6, 2016 at 2:04 PM, <pcc@chromium.org> wrote: > On 2016/10/06 09:04:32, commit-bot: I haz the power wrote: > > Patchset 5 (id:??) landed as > > https://crrev.com/765f221e66c413ff3a50a52b4b1eb9172610c669 > > Cr-Commit-Position: refs/heads/master@{#423481} > > Hi Miguel, it look like this change caused a build failure on the > ClangToTMac > bot, e.g. > https://build.chromium.org/p/chromium.fyi/builders/ > ClangToTMac/builds/11539 > > Can you please take a look? > > (This doesn't look like a clang ToT issue, I can't seem to find any other > Mac > bot that builds the "all" target though.) > > https://codereview.chromium.org/2070903002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
I think the ToT bots use Xcode 8 but the main waterfall bot probably doesn't yet. Miguel, did you try building this with Xcode 8? On Thu, Oct 6, 2016 at 2:14 PM, Nico Weber <thakis@chromium.org> wrote: > This one builds 'all': https://build.chromium.org/p/chromium/builders/Mac > > On Thu, Oct 6, 2016 at 2:04 PM, <pcc@chromium.org> wrote: > >> On 2016/10/06 09:04:32, commit-bot: I haz the power wrote: >> > Patchset 5 (id:??) landed as >> > https://crrev.com/765f221e66c413ff3a50a52b4b1eb9172610c669 >> > Cr-Commit-Position: refs/heads/master@{#423481} >> >> Hi Miguel, it look like this change caused a build failure on the >> ClangToTMac >> bot, e.g. >> https://build.chromium.org/p/chromium.fyi/builders/ClangToTM >> ac/builds/11539 >> >> Can you please take a look? >> >> (This doesn't look like a clang ToT issue, I can't seem to find any other >> Mac >> bot that builds the "all" target though.) >> >> https://codereview.chromium.org/2070903002/ >> > > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
No, I am using 7.3. My corp laptop only offers this as an option. Is Xcode 8 the recommended compiler now or is it just a test bot? We have some off corp machines, will try to install Xcode 8 in there and give it a shot. On Thu, Oct 6, 2016 at 7:15 PM, Nico Weber <thakis@chromium.org> wrote: > I think the ToT bots use Xcode 8 but the main waterfall bot probably > doesn't yet. Miguel, did you try building this with Xcode 8? > > On Thu, Oct 6, 2016 at 2:14 PM, Nico Weber <thakis@chromium.org> wrote: > >> This one builds 'all': https://build.chromium.org/p/chromium/builders/Mac >> >> On Thu, Oct 6, 2016 at 2:04 PM, <pcc@chromium.org> wrote: >> >>> On 2016/10/06 09:04:32, commit-bot: I haz the power wrote: >>> > Patchset 5 (id:??) landed as >>> > https://crrev.com/765f221e66c413ff3a50a52b4b1eb9172610c669 >>> > Cr-Commit-Position: refs/heads/master@{#423481} >>> >>> Hi Miguel, it look like this change caused a build failure on the >>> ClangToTMac >>> bot, e.g. >>> https://build.chromium.org/p/chromium.fyi/builders/ClangToTM >>> ac/builds/11539 >>> >>> Can you please take a look? >>> >>> (This doesn't look like a clang ToT issue, I can't seem to find any >>> other Mac >>> bot that builds the "all" target though.) >>> >>> https://codereview.chromium.org/2070903002/ >>> >> >> > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
In the interim I would be ok removing the xpc service from src/BUILD.gn to not break the bot. I will lose compile time coverage in the meantime but I can live with that. Uploaded https://codereview.chromium.org/2402683002/ On Fri, Oct 7, 2016 at 11:16 AM, Miguel Garcia <miguelg@chromium.org> wrote: > No, I am using 7.3. My corp laptop only offers this as an option. > > Is Xcode 8 the recommended compiler now or is it just a test bot? > > We have some off corp machines, will try to install Xcode 8 in there and > give it a shot. > > On Thu, Oct 6, 2016 at 7:15 PM, Nico Weber <thakis@chromium.org> wrote: > >> I think the ToT bots use Xcode 8 but the main waterfall bot probably >> doesn't yet. Miguel, did you try building this with Xcode 8? >> >> On Thu, Oct 6, 2016 at 2:14 PM, Nico Weber <thakis@chromium.org> wrote: >> >>> This one builds 'all': https://build.chromium. >>> org/p/chromium/builders/Mac >>> >>> On Thu, Oct 6, 2016 at 2:04 PM, <pcc@chromium.org> wrote: >>> >>>> On 2016/10/06 09:04:32, commit-bot: I haz the power wrote: >>>> > Patchset 5 (id:??) landed as >>>> > https://crrev.com/765f221e66c413ff3a50a52b4b1eb9172610c669 >>>> > Cr-Commit-Position: refs/heads/master@{#423481} >>>> >>>> Hi Miguel, it look like this change caused a build failure on the >>>> ClangToTMac >>>> bot, e.g. >>>> https://build.chromium.org/p/chromium.fyi/builders/ClangToTM >>>> ac/builds/11539 >>>> >>>> Can you please take a look? >>>> >>>> (This doesn't look like a clang ToT issue, I can't seem to find any >>>> other Mac >>>> bot that builds the "all" target though.) >>>> >>>> https://codereview.chromium.org/2070903002/ >>>> >>> >>> >> > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
Or even better, Robert discovered that I am probably just missing an include so uploaded https://codereview.chromium.org/2396363002/ to fix. If this does not fix it I am still ok removing the xpc service from src/BUILD.gn but let's see if this fixes it first. On Fri, Oct 7, 2016 at 2:50 PM, Miguel Garcia <miguelg@google.com> wrote: > In the interim I would be ok removing the xpc service from src/BUILD.gn to > not break the bot. I will lose compile time coverage in the meantime but I > can live with that. > > Uploaded https://codereview.chromium.org/2402683002/ > > > > On Fri, Oct 7, 2016 at 11:16 AM, Miguel Garcia <miguelg@chromium.org> > wrote: > >> No, I am using 7.3. My corp laptop only offers this as an option. >> >> Is Xcode 8 the recommended compiler now or is it just a test bot? >> >> We have some off corp machines, will try to install Xcode 8 in there and >> give it a shot. >> >> On Thu, Oct 6, 2016 at 7:15 PM, Nico Weber <thakis@chromium.org> wrote: >> >>> I think the ToT bots use Xcode 8 but the main waterfall bot probably >>> doesn't yet. Miguel, did you try building this with Xcode 8? >>> >>> On Thu, Oct 6, 2016 at 2:14 PM, Nico Weber <thakis@chromium.org> wrote: >>> >>>> This one builds 'all': https://build.chromium. >>>> org/p/chromium/builders/Mac >>>> >>>> On Thu, Oct 6, 2016 at 2:04 PM, <pcc@chromium.org> wrote: >>>> >>>>> On 2016/10/06 09:04:32, commit-bot: I haz the power wrote: >>>>> > Patchset 5 (id:??) landed as >>>>> > https://crrev.com/765f221e66c413ff3a50a52b4b1eb9172610c669 >>>>> > Cr-Commit-Position: refs/heads/master@{#423481} >>>>> >>>>> Hi Miguel, it look like this change caused a build failure on the >>>>> ClangToTMac >>>>> bot, e.g. >>>>> https://build.chromium.org/p/chromium.fyi/builders/ClangToTM >>>>> ac/builds/11539 >>>>> >>>>> Can you please take a look? >>>>> >>>>> (This doesn't look like a clang ToT issue, I can't seem to find any >>>>> other Mac >>>>> bot that builds the "all" target though.) >>>>> >>>>> https://codereview.chromium.org/2070903002/ >>>>> >>>> >>>> >>> >> > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. |
