Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2243863003:
Added authorized install with a script to do the copy. (Closed)
|
Created:
4 years, 4 months ago by ivanhernandez Modified:
4 years, 3 months ago CC:
chromium-reviews, grt+watch_chromium.org, pennymac+watch_chromium.org, wfh+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdded authorized install with a script to do the copy.
The script will be loaded at the start of the program and hang until it is needed later on in the unpacking step.
Committed: https://crrev.com/c5390b48dd8a6187c919549d2900be84a8e6176e
Cr-Commit-Position: refs/heads/master@{#414151}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Changed to OO design #
Total comments: 19
Patch Set 3 : Cleaned up the API for authInstall. #Patch Set 4 : Cleaned up code and made the script waiting not awkward #
Total comments: 24
Patch Set 5 : Addressed Elly's comments. #Patch Set 6 : Rebased #Patch Set 7 : Changed BUILD.gn so that only the app would compile AuthorizedInstall.m and not the tests. #
Messages
Total messages: 42 (22 generated)
ivanhernandez@google.com changed reviewers: + ellyjones@chromium.org, mark@chromium.org, zengster@google.com
Ready for first round of review.
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
review'd patchset 1 :) https://codereview.chromium.org/2243863003/diff/1/chrome/installer/mac/app/Au... File chrome/installer/mac/app/AuthorizedInstall.m (right): https://codereview.chromium.org/2243863003/diff/1/chrome/installer/mac/app/Au... chrome/installer/mac/app/AuthorizedInstall.m:15: exit(1); that seems kind of abrupt? maybe we should signal an error here https://codereview.chromium.org/2243863003/diff/1/chrome/installer/mac/app/Au... chrome/installer/mac/app/AuthorizedInstall.m:74: *status = AuthorizationExecuteWithPrivileges( Hm - if this is deprecated, is there a newer, shinier way we should be using? https://codereview.chromium.org/2243863003/diff/1/chrome/installer/mac/app/co... File chrome/installer/mac/app/copy_to_disk.sh (right): https://codereview.chromium.org/2243863003/diff/1/chrome/installer/mac/app/co... chrome/installer/mac/app/copy_to_disk.sh:4: # Use of this source code is governed by a BSD-style license that can be This script should set -e and it can probably just be #!/bin/sh :)
Patchset #2 (id:60001) has been deleted
https://codereview.chromium.org/2243863003/diff/1/chrome/installer/mac/app/Au... File chrome/installer/mac/app/AuthorizedInstall.m (right): https://codereview.chromium.org/2243863003/diff/1/chrome/installer/mac/app/Au... chrome/installer/mac/app/AuthorizedInstall.m:74: *status = AuthorizationExecuteWithPrivileges( Elly Jones wrote: > Hm - if this is deprecated, is there a newer, shinier way we should be using? Ivan asked me about this. There isn’t a replacement that works for us.
Addressed Elly's comments and switched this file to be class based like the rest of our program. No changes in Info.plist. I modified Info.plist at some point and undid all the changes but diff shows that my editor replaced spaces with tabs? https://codereview.chromium.org/2243863003/diff/1/chrome/installer/mac/app/Au... File chrome/installer/mac/app/AuthorizedInstall.m (right): https://codereview.chromium.org/2243863003/diff/1/chrome/installer/mac/app/Au... chrome/installer/mac/app/AuthorizedInstall.m:15: exit(1); On 2016/08/16 14:59:42, Elly Jones wrote: > that seems kind of abrupt? maybe we should signal an error here Acknowledged. https://codereview.chromium.org/2243863003/diff/1/chrome/installer/mac/app/co... File chrome/installer/mac/app/copy_to_disk.sh (right): https://codereview.chromium.org/2243863003/diff/1/chrome/installer/mac/app/co... chrome/installer/mac/app/copy_to_disk.sh:4: # Use of this source code is governed by a BSD-style license that can be On 2016/08/16 14:59:42, Elly Jones wrote: > This script should set -e and it can probably just be #!/bin/sh :) I appended the e flag to -p. Also added comments to explain why -p is necessary (actually though, I just copy pasted marks comments from chrome/browser/mac/install.sh where he does something similar and needs roots permissions and explains why -p is needed). Not having -p causes the script to lose root permissions when I run it from the program. https://codereview.chromium.org/2243863003/diff/80001/chrome/installer/mac/ap... File chrome/installer/mac/app/copy_to_disk.sh (right): https://codereview.chromium.org/2243863003/diff/80001/chrome/installer/mac/ap... chrome/installer/mac/app/copy_to_disk.sh:18: read -n 1 We want to authenticate as soon as the program launches but we don't want to this script to run until later. This is my "solution" to this problem. The main program will feed this program input when its ready to copy stuff and thus unblock this script. Im not sure if this is a duct tape solution or if it is viable for the final product.
https://codereview.chromium.org/2243863003/diff/80001/chrome/installer/mac/ap... File chrome/installer/mac/app/AuthorizedInstall.h (right): https://codereview.chromium.org/2243863003/diff/80001/chrome/installer/mac/ap... chrome/installer/mac/app/AuthorizedInstall.h:5: #ifndef CHROME_INSTALLER_MAC_APP_AUTHORIZEDINSTALL_H_ Is there a test file for this new class? Let's add that file in to this CL! https://codereview.chromium.org/2243863003/diff/80001/chrome/installer/mac/ap... chrome/installer/mac/app/AuthorizedInstall.h:14: FILE* communicationPipe_; authRef_, status_, and communicationPipe_ can all be local variables -- nobody else really needs to know about these instance variables! Let's make it happen. :D https://codereview.chromium.org/2243863003/diff/80001/chrome/installer/mac/ap... chrome/installer/mac/app/AuthorizedInstall.h:18: - (BOOL)authorizeInstallationTool; Can we make this the init function? https://codereview.chromium.org/2243863003/diff/80001/chrome/installer/mac/ap... chrome/installer/mac/app/AuthorizedInstall.h:21: - (void)sendMessageToTool:(NSString*)message; I think the preferred API is a function that doesn't take any parameters, but returns the path of the Chrome app in its final resting place. I also suggest that is what you change it to be. https://codereview.chromium.org/2243863003/diff/80001/chrome/installer/mac/ap... File chrome/installer/mac/app/AuthorizedInstall.m (right): https://codereview.chromium.org/2243863003/diff/80001/chrome/installer/mac/ap... chrome/installer/mac/app/AuthorizedInstall.m:31: #pragma clang diagnostic ignored "-Wdeprecated-declarations" Mark (mark@chromium.org): Have we tried NSAppleScript yet? (https://developer.apple.com/library/mac/documentation/Cocoa/Reference/Foundat...) Ivan did mention we've determined SMJobBless was not a viable alternative, but is NSAppleScript possible? https://codereview.chromium.org/2243863003/diff/80001/chrome/installer/mac/ap... chrome/installer/mac/app/AuthorizedInstall.m:40: // TODO: Move attemptAuthorizedInstall body into Unpacker method which will As discussed, we're actually going to have AuthorizedInstall take care of putting Chrome into the Applications folder of choice. https://codereview.chromium.org/2243863003/diff/80001/chrome/installer/mac/ap... chrome/installer/mac/app/AuthorizedInstall.m:60: NSString* sourceAppBundlePath = [NSString We can make this a parameter. https://codereview.chromium.org/2243863003/diff/80001/chrome/installer/mac/ap... chrome/installer/mac/app/AuthorizedInstall.m:62: NSString* destinationAppBundlePath = If root, this can just be @"/Applications/Google Chrome.app". Is there any particular reason this needs to be found programmatically, or cannot be hardcoded in such a way? If not root, we can use `[[NSFileManager defaultManager] isWritableFileAtPath:@"/Applications/"]` to check if it's actually writeable before feeding in the path. I think that logic definitely belongs in here. https://codereview.chromium.org/2243863003/diff/80001/chrome/installer/mac/ap... File chrome/installer/mac/app/Info.plist (right): https://codereview.chromium.org/2243863003/diff/80001/chrome/installer/mac/ap... chrome/installer/mac/app/Info.plist:5: <key>CFBundleDevelopmentRegion</key> Switch to spaces! https://google.github.io/styleguide/objcguide.xml#Spaces_vs._Tabs https://codereview.chromium.org/2243863003/diff/80001/chrome/installer/mac/ap... File chrome/installer/mac/app/copy_to_disk.sh (right): https://codereview.chromium.org/2243863003/diff/80001/chrome/installer/mac/ap... chrome/installer/mac/app/copy_to_disk.sh:18: read -n 1 On 2016/08/16 17:18:39, ivanhernandez wrote: > We want to authenticate as soon as the program launches but we don't want to > this script to run until later. This is my "solution" to this problem. The main > program will feed this program input when its ready to copy stuff and thus > unblock this script. Im not sure if this is a duct tape solution or if it is > viable for the final product. Minor correction: "... but we don't want this script..." Also, could using `wait` work? http://stackoverflow.com/questions/13296863/difference-between-wait-and-sleep... It's a bit less hack-like but seems a bit less intuitive how we could use it.
Patchset #3 (id:100001) has been deleted
Patchset #3 (id:120001) has been deleted
Description was changed from ========== Added authorized install with a script to do the copy. Allows to check whether authentication succeeded before attempting install so we could implement a fallback. ========== to ========== Added authorized install with a script to do the copy. The script will be loaded at the start of the program and hang until it is needed later on in the unpacking step. ==========
Changed the API for AuthorizedInstall to better suit what Unpacker will need. Also cleaned up the implementation file by factoring out long blocks into their own methods and creating variables only when their needed and not when they might be needed. Looking for design advice for the part where we actually start the tool. Currently it starts at the start of the main program and I have it wait by having the tool wait for a single character to its stdin. To unblock it we need to send input to its stdin which the method 'startInstall' does by calling another method sendMessageToTool:(NSString*). https://codereview.chromium.org/2243863003/diff/80001/chrome/installer/mac/ap... File chrome/installer/mac/app/AuthorizedInstall.h (right): https://codereview.chromium.org/2243863003/diff/80001/chrome/installer/mac/ap... chrome/installer/mac/app/AuthorizedInstall.h:5: #ifndef CHROME_INSTALLER_MAC_APP_AUTHORIZEDINSTALL_H_ On 2016/08/18 15:57:16, Anna Zeng wrote: > Is there a test file for this new class? Let's add that file in to this CL! Will probably be on the next patch :) https://codereview.chromium.org/2243863003/diff/80001/chrome/installer/mac/ap... chrome/installer/mac/app/AuthorizedInstall.h:14: FILE* communicationPipe_; On 2016/08/18 15:57:16, Anna Zeng wrote: > authRef_, status_, and communicationPipe_ can all be local variables -- nobody > else really needs to know about these instance variables! Let's make it happen. > :D Done. https://codereview.chromium.org/2243863003/diff/80001/chrome/installer/mac/ap... chrome/installer/mac/app/AuthorizedInstall.h:18: - (BOOL)authorizeInstallationTool; On 2016/08/18 15:57:16, Anna Zeng wrote: > Can we make this the init function? Probably want to use a name other than init to better indicate what this method will do since it does a bit more than set up variables. Also changed the name to better reflect what it does. https://codereview.chromium.org/2243863003/diff/80001/chrome/installer/mac/ap... chrome/installer/mac/app/AuthorizedInstall.h:21: - (void)sendMessageToTool:(NSString*)message; On 2016/08/18 15:57:16, Anna Zeng wrote: > I think the preferred API is a function that doesn't take any parameters, but > returns the path of the Chrome app in its final resting place. I also suggest > that is what you change it to be. Done. https://codereview.chromium.org/2243863003/diff/80001/chrome/installer/mac/ap... File chrome/installer/mac/app/AuthorizedInstall.m (right): https://codereview.chromium.org/2243863003/diff/80001/chrome/installer/mac/ap... chrome/installer/mac/app/AuthorizedInstall.m:40: // TODO: Move attemptAuthorizedInstall body into Unpacker method which will On 2016/08/18 15:57:16, Anna Zeng wrote: > As discussed, we're actually going to have AuthorizedInstall take care of > putting Chrome into the Applications folder of choice. Done. https://codereview.chromium.org/2243863003/diff/80001/chrome/installer/mac/ap... chrome/installer/mac/app/AuthorizedInstall.m:60: NSString* sourceAppBundlePath = [NSString On 2016/08/18 15:57:16, Anna Zeng wrote: > We can make this a parameter. We could but we would need to make some changes to unpacker first. The thing is that this method runs at the very beginning of the program. The Unpacker creates the temporary directory. Theres no way to know what that directory will be at this point. We could just simply create the temporary directory at the beginning and then pass it to both this and to the unpacker. https://codereview.chromium.org/2243863003/diff/80001/chrome/installer/mac/ap... chrome/installer/mac/app/AuthorizedInstall.m:62: NSString* destinationAppBundlePath = On 2016/08/18 15:57:16, Anna Zeng wrote: > If root, this can just be @"/Applications/Google Chrome.app". Is there any > particular reason this needs to be found programmatically, or cannot be > hardcoded in such a way? > > If not root, we can use `[[NSFileManager defaultManager] > isWritableFileAtPath:@"/Applications/"]` to check if it's actually writeable > before feeding in the path. I think that logic definitely belongs in here. I don't know actually. Its true the path will always be "/Applications/Google Chrome.app" for root unless Apple decides to do some major OS redesign. But seeing as thats highly unlikely I suppose either way would be okay. Maybe Elly or Mark can weigh in on which choice is better. https://codereview.chromium.org/2243863003/diff/80001/chrome/installer/mac/ap... File chrome/installer/mac/app/copy_to_disk.sh (right): https://codereview.chromium.org/2243863003/diff/80001/chrome/installer/mac/ap... chrome/installer/mac/app/copy_to_disk.sh:18: read -n 1 On 2016/08/18 15:57:16, Anna Zeng wrote: > On 2016/08/16 17:18:39, ivanhernandez wrote: > > We want to authenticate as soon as the program launches but we don't want to > > this script to run until later. This is my "solution" to this problem. The > main > > program will feed this program input when its ready to copy stuff and thus > > unblock this script. Im not sure if this is a duct tape solution or if it is > > viable for the final product. > > Minor correction: "... but we don't want this script..." > > Also, could using `wait` work? > http://stackoverflow.com/questions/13296863/difference-between-wait-and-sleep... > It's a bit less hack-like but seems a bit less intuitive how we could use it. Hmm seems like 'wait' would need for another process to finish. Which means we would need two subprocesses in this program. Doesn't seem ideal.
Functionally pretty much the same as patch 3 but with cleaned up code and script waiting and reading input now serves a purpose.
Hi podmate! :D More lovely feedback for you. https://codereview.chromium.org/2243863003/diff/160001/chrome/installer/mac/a... File chrome/installer/mac/app/AppDelegate.m (right): https://codereview.chromium.org/2243863003/diff/160001/chrome/installer/mac/a... chrome/installer/mac/app/AppDelegate.m:81: // TODO: Add unpacking step here and pass the path to the app bundle inside I appreciate the verboseness of this TODO. https://codereview.chromium.org/2243863003/diff/160001/chrome/installer/mac/a... chrome/installer/mac/app/AppDelegate.m:123: Odd that there's extra new lines here. Remove? https://codereview.chromium.org/2243863003/diff/160001/chrome/installer/mac/a... File chrome/installer/mac/app/AuthorizedInstall.h (right): https://codereview.chromium.org/2243863003/diff/160001/chrome/installer/mac/a... chrome/installer/mac/app/AuthorizedInstall.h:16: // Sends a message to install tool to start the install. Returns the path to the "Starts the installation process." Stuff about sending a message and such are implementation details and don't need to be in the header file. https://codereview.chromium.org/2243863003/diff/160001/chrome/installer/mac/a... File chrome/installer/mac/app/copy_to_disk.sh (right): https://codereview.chromium.org/2243863003/diff/160001/chrome/installer/mac/a... chrome/installer/mac/app/copy_to_disk.sh:15: set -euo pipefail It'd be helpful to include what the flags mean in the documentation. https://codereview.chromium.org/2243863003/diff/160001/chrome/installer/mac/a... chrome/installer/mac/app/copy_to_disk.sh:26: rsync -lrptq "${SRC}" "${DEST}" Ditto!
lgtm! looking good :) https://codereview.chromium.org/2243863003/diff/160001/chrome/installer/mac/a... File chrome/installer/mac/app/AppDelegate.m (right): https://codereview.chromium.org/2243863003/diff/160001/chrome/installer/mac/a... chrome/installer/mac/app/AppDelegate.m:85: // startInstall:@"/Users/ivanhernandez/Downloads/Google Chromo.app"]; In comments it's fine to be like, @"$HOME/Downloads/Google Chromo.app" or whatever; you don't need to stick your username in them. https://codereview.chromium.org/2243863003/diff/160001/chrome/installer/mac/a... File chrome/installer/mac/app/AuthorizedInstall.h (right): https://codereview.chromium.org/2243863003/diff/160001/chrome/installer/mac/a... chrome/installer/mac/app/AuthorizedInstall.h:16: // Sends a message to install tool to start the install. Returns the path to the On 2016/08/23 19:36:12, Anna Zeng wrote: > "Starts the installation process." Stuff about sending a message and such are > implementation details and don't need to be in the header file. +1 https://codereview.chromium.org/2243863003/diff/160001/chrome/installer/mac/a... File chrome/installer/mac/app/AuthorizedInstall.m (right): https://codereview.chromium.org/2243863003/diff/160001/chrome/installer/mac/a... chrome/installer/mac/app/AuthorizedInstall.m:64: // c) The user doesn't authenticate and is not an admin. This comment is so good. You can refer to /Users/username as just "$HOME"; it's idiomatic on macOS and other unixes, so this path is $HOME/Applications https://codereview.chromium.org/2243863003/diff/160001/chrome/installer/mac/a... chrome/installer/mac/app/AuthorizedInstall.m:103: applicationsDirectory, @"Google Chrome.app"]]; no more Chromo? :) https://codereview.chromium.org/2243863003/diff/160001/chrome/installer/mac/a... chrome/installer/mac/app/AuthorizedInstall.m:122: [self sendMessageToTool:@"\n"]; that's odd - why is the newline necessary? is the tool waiting for it? if it's always necessary, maybe sendMessageToTool: should append it? https://codereview.chromium.org/2243863003/diff/160001/chrome/installer/mac/a... File chrome/installer/mac/app/copy_to_disk.sh (right): https://codereview.chromium.org/2243863003/diff/160001/chrome/installer/mac/a... chrome/installer/mac/app/copy_to_disk.sh:17: # Wait's for the main app to pass the path to the app bundle inside the mounted Wait's -> Waits https://codereview.chromium.org/2243863003/diff/160001/chrome/installer/mac/a... chrome/installer/mac/app/copy_to_disk.sh:21: DEST=${1} quotes here
Patchset #5 (id:180001) has been deleted
Patchset #5 (id:200001) has been deleted
https://codereview.chromium.org/2243863003/diff/160001/chrome/installer/mac/a... File chrome/installer/mac/app/AppDelegate.m (right): https://codereview.chromium.org/2243863003/diff/160001/chrome/installer/mac/a... chrome/installer/mac/app/AppDelegate.m:81: // TODO: Add unpacking step here and pass the path to the app bundle inside On 2016/08/23 19:36:12, Anna Zeng wrote: > I appreciate the verboseness of this TODO. Done. https://codereview.chromium.org/2243863003/diff/160001/chrome/installer/mac/a... chrome/installer/mac/app/AppDelegate.m:85: // startInstall:@"/Users/ivanhernandez/Downloads/Google Chromo.app"]; On 2016/08/24 16:39:22, Elly Jones wrote: > In comments it's fine to be like, @"$HOME/Downloads/Google Chromo.app" or > whatever; you don't need to stick your username in them. Done. https://codereview.chromium.org/2243863003/diff/160001/chrome/installer/mac/a... chrome/installer/mac/app/AppDelegate.m:123: On 2016/08/23 19:36:12, Anna Zeng wrote: > Odd that there's extra new lines here. Remove? Done. https://codereview.chromium.org/2243863003/diff/160001/chrome/installer/mac/a... File chrome/installer/mac/app/AuthorizedInstall.h (right): https://codereview.chromium.org/2243863003/diff/160001/chrome/installer/mac/a... chrome/installer/mac/app/AuthorizedInstall.h:16: // Sends a message to install tool to start the install. Returns the path to the On 2016/08/23 19:36:12, Anna Zeng wrote: > "Starts the installation process." Stuff about sending a message and such are > implementation details and don't need to be in the header file. Done. https://codereview.chromium.org/2243863003/diff/160001/chrome/installer/mac/a... File chrome/installer/mac/app/AuthorizedInstall.m (right): https://codereview.chromium.org/2243863003/diff/160001/chrome/installer/mac/a... chrome/installer/mac/app/AuthorizedInstall.m:64: // c) The user doesn't authenticate and is not an admin. On 2016/08/24 16:39:22, Elly Jones wrote: > This comment is so good. You can refer to /Users/username as just "$HOME"; it's > idiomatic on macOS and other unixes, so this path is $HOME/Applications Done. https://codereview.chromium.org/2243863003/diff/160001/chrome/installer/mac/a... chrome/installer/mac/app/AuthorizedInstall.m:103: applicationsDirectory, @"Google Chrome.app"]]; On 2016/08/24 16:39:22, Elly Jones wrote: > no more Chromo? :) This app is moving up in the world. https://codereview.chromium.org/2243863003/diff/160001/chrome/installer/mac/a... chrome/installer/mac/app/AuthorizedInstall.m:122: [self sendMessageToTool:@"\n"]; On 2016/08/24 16:39:22, Elly Jones wrote: > that's odd - why is the newline necessary? is the tool waiting for it? if it's > always necessary, maybe sendMessageToTool: should append it? Good point, having sendMessageToTool append the newline is better. Added a comment to sendMessageToTool to explain why appending the newline character is necessary. https://codereview.chromium.org/2243863003/diff/160001/chrome/installer/mac/a... File chrome/installer/mac/app/copy_to_disk.sh (right): https://codereview.chromium.org/2243863003/diff/160001/chrome/installer/mac/a... chrome/installer/mac/app/copy_to_disk.sh:15: set -euo pipefail On 2016/08/23 19:36:12, Anna Zeng wrote: > It'd be helpful to include what the flags mean in the documentation. nah. https://codereview.chromium.org/2243863003/diff/160001/chrome/installer/mac/a... chrome/installer/mac/app/copy_to_disk.sh:17: # Wait's for the main app to pass the path to the app bundle inside the mounted On 2016/08/24 16:39:22, Elly Jones wrote: > Wait's -> Waits Done. https://codereview.chromium.org/2243863003/diff/160001/chrome/installer/mac/a... chrome/installer/mac/app/copy_to_disk.sh:21: DEST=${1} On 2016/08/24 16:39:22, Elly Jones wrote: > quotes here Done. https://codereview.chromium.org/2243863003/diff/160001/chrome/installer/mac/a... chrome/installer/mac/app/copy_to_disk.sh:26: rsync -lrptq "${SRC}" "${DEST}" On 2016/08/23 19:36:12, Anna Zeng wrote: > Ditto! Done.
The CQ bit was checked by ivanhernandez@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from ellyjones@chromium.org Link to the patchset: https://codereview.chromium.org/2243863003/#ps220001 (title: "Addressed Elly's comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/2243863003/diff/160001/chrome/installer/mac/a... File chrome/installer/mac/app/copy_to_disk.sh (right): https://codereview.chromium.org/2243863003/diff/160001/chrome/installer/mac/a... chrome/installer/mac/app/copy_to_disk.sh:15: set -euo pipefail On 2016/08/24 18:23:17, ivanhernandez wrote: > On 2016/08/23 19:36:12, Anna Zeng wrote: > > It'd be helpful to include what the flags mean in the documentation. > > nah. Whoops forgot to change this joke reply to 'Done.' I did add comments to explain the need for the flags :).
The CQ bit was checked by ivanhernandez@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from ellyjones@chromium.org Link to the patchset: https://codereview.chromium.org/2243863003/#ps240001 (title: "Rebased")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by ivanhernandez@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from ellyjones@chromium.org Link to the patchset: https://codereview.chromium.org/2243863003/#ps260001 (title: "Changed BUILD.gn so that only the app would compile AuthorizedInstall.m and not the tests.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by ivanhernandez@google.com
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.
Description was changed from ========== Added authorized install with a script to do the copy. The script will be loaded at the start of the program and hang until it is needed later on in the unpacking step. ========== to ========== Added authorized install with a script to do the copy. The script will be loaded at the start of the program and hang until it is needed later on in the unpacking step. ==========
Message was sent while issue was closed.
Committed patchset #7 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== Added authorized install with a script to do the copy. The script will be loaded at the start of the program and hang until it is needed later on in the unpacking step. ========== to ========== Added authorized install with a script to do the copy. The script will be loaded at the start of the program and hang until it is needed later on in the unpacking step. Committed: https://crrev.com/c5390b48dd8a6187c919549d2900be84a8e6176e Cr-Commit-Position: refs/heads/master@{#414151} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/c5390b48dd8a6187c919549d2900be84a8e6176e Cr-Commit-Position: refs/heads/master@{#414151}
Message was sent while issue was closed.
Patchset #8 (id:280001) has been deleted |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
