Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
940633003:
Catch AccessDeniedException on bots without permission to download SDK extras. (Closed)
DescriptionCatch AccessDeniedException on bots without permission to download SDK extras.
Non-android bots will not have credentials to download SDK packages from Google
Storage buckets. Thus, if a bot does not have credentials, the hook should print
a warning and continue.
BUG=459681
Committed: https://crrev.com/30bc93f3361397f8cde5d319923f1609508bb909
Cr-Commit-Position: refs/heads/master@{#316973}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Add TODO to move hook. #Patch Set 3 : Exit early if this is not an Android bot. #Patch Set 4 : If OS=android not in GYP_DEFINES then not Android bot, return 0. #Messages
Total messages: 26 (9 generated)
navabi@google.com changed reviewers: + cjhopman@chromium.org, hinoka@chromium.org, hshi@google.com, matthewyuan@chromium.org, nyquist@chromium.org
navabi@google.com changed reviewers: + binji@chromium.org
hshi@chromium.org changed reviewers: + hshi@chromium.org
https://codereview.chromium.org/940633003/diff/1/build/download_sdk_extras.py File build/download_sdk_extras.py (right): https://codereview.chromium.org/940633003/diff/1/build/download_sdk_extras.py... build/download_sdk_extras.py:56: ' If this bot compiles for Android, it may have errors.') why is android_sdk_extras among the packages in the first place? shouldn't we only download sdk extras for android builds?
hinoka@chromium.org changed reviewers: - hshi@chromium.org
Maybe just wrap main(), or make sure this script never returns a non-zero code, since apparently its not critical. https://codereview.chromium.org/940633003/diff/1/build/download_sdk_extras.py File build/download_sdk_extras.py (right): https://codereview.chromium.org/940633003/diff/1/build/download_sdk_extras.py... build/download_sdk_extras.py:52: subprocess.check_call(['python', GSUTIL_PATH, '--force-version', '4.7', s/'python'/sys.executable/
https://codereview.chromium.org/940633003/diff/1/build/download_sdk_extras.py File build/download_sdk_extras.py (right): https://codereview.chromium.org/940633003/diff/1/build/download_sdk_extras.py... build/download_sdk_extras.py:52: subprocess.check_call(['python', GSUTIL_PATH, '--force-version', '4.7', On 2015/02/18 21:33:07, hinoka wrote: > s/'python'/sys.executable/ I don't think this works on windows. I would expect I would see the same error I had seen on other patches. I.e. WindowsError: [Error 193] %1 is not a valid Win32 application https://codereview.chromium.org/940633003/diff/1/build/download_sdk_extras.py... build/download_sdk_extras.py:56: ' If this bot compiles for Android, it may have errors.') On 2015/02/18 21:32:27, hshi1 wrote: > why is android_sdk_extras among the packages in the first place? shouldn't we > only download sdk extras for android builds? If possible, we should only call download_sdk_packages.py for android, but I could not find a way to distinguish platform for hooks (only for deps_os). See src/DEPS. How do you suggest doing this?
On 2015/02/18 21:33:07, hinoka wrote: > Maybe just wrap main(), or make sure this script never returns a non-zero code, > since apparently its not critical. It is critical on Android bots, as if this doesn't run, there will be a compile failure later in the code. We should make it return a warning to make it clear.
Why not make it a failure step on android bots before compiling? Eg pre-compile if the SDK extras isn't detected, vomit out a loud warning there. Remember that this is in the _open source_ chromium hooks. Real external people and non-android bots would run this code all the time.
I need to question why are we downloading things that are off-limited to the external world again?
> Why not make it a failure step on android bots before compiling? Eg pre-compile > if the SDK extras isn't detected, vomit out a loud warning there. Are you suggesting not making this a hook at all? I think main problem there is enforcing that all recipes that may compile Android add that step. We have no good way of listing those, so we would immediately see fallout (more than we are currently seeing) from implementing it that way. If I misunderstood, please clarify. > I need to question why are we downloading things that are off-limited to the > external world again? This was discussed in a lot of detail here: https://code.google.com/p/chromium/issues/detail?id=350151 The short story is because the external world needs to accept terms of service for SDK packages, but we do not want to go on each of the hosts and manually accept terms of service (and then again every time new packages get added). For developers, the packages get installed by install-build-deps-android.sh which will prompt for ToS. For bots, this is installed as part of this hook.
> Why not make it a failure step on android bots before compiling? Eg pre-compile > if the SDK extras isn't detected, vomit out a loud warning there. Are you suggesting not making this a hook at all? I think main problem there is enforcing that all recipes that may compile Android add that step. We have no good way of listing those, so we would immediately see fallout (more than we are currently seeing) from implementing it that way. If I misunderstood, please clarify. > I need to question why are we downloading things that are off-limited to the > external world again? This was discussed in a lot of detail here: https://code.google.com/p/chromium/issues/detail?id=350151 The short story is because the external world needs to accept terms of service for SDK packages, but we do not want to go on each of the hosts and manually accept terms of service (and then again every time new packages get added). For developers, the packages get installed by install-build-deps-android.sh which will prompt for ToS. For bots, this is installed as part of this hook.
lgtm % add crbug.com/459819 as a big todo somewhere.
New patchsets have been uploaded after l-g-t-m from hinoka@chromium.org
The CQ bit was checked by navabi@google.com
sbc@chromium.org changed reviewers: + sbc@chromium.org
https://codereview.chromium.org/940633003/diff/1/build/download_sdk_extras.py File build/download_sdk_extras.py (right): https://codereview.chromium.org/940633003/diff/1/build/download_sdk_extras.py... build/download_sdk_extras.py:56: ' If this bot compiles for Android, it may have errors.') On 2015/02/18 22:47:03, navabi wrote: > On 2015/02/18 21:32:27, hshi1 wrote: > > why is android_sdk_extras among the packages in the first place? shouldn't we > > only download sdk extras for android builds? > > If possible, we should only call download_sdk_packages.py for android, but I > could not find a way to distinguish platform for hooks (only for deps_os). See > src/DEPS. How do you suggest doing this? There are other scripts that do this by inspecting GYP_DEFINES (See download_nacl_toolchains.py). Its not pretty, but I don't know of any other way. Also, I think then name of this script could be more descriptive. 'sdk_extras' is very generic. but thats a different CL I guess. If you are going to handle the exception that you should probably return 0 in the handler, no? But its probably better not to handle it but to return early for non-android bots.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/940633003/20001
The CQ bit was unchecked by navabi@google.com
The CQ bit was checked by navabi@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/940633003/60001
lgtm
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/30bc93f3361397f8cde5d319923f1609508bb909 Cr-Commit-Position: refs/heads/master@{#316973} |
