Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1534873002:
Download sysroot from "commondatastorage.googleapis.com" (Closed)
DescriptionDownload sysroot from "commondatastorage.googleapis.com"
For some reason our intranet here is blocking googleapis.com.
Actually other tools that are updated via runhooks are using
that alias url already (e.g. clang).
Committed: https://crrev.com/dbf2f363526a4a1fc90af90206a2e0e7761eb75d
Cr-Commit-Position: refs/heads/master@{#366481}
Patch Set 1 #Patch Set 2 : doh #
Total comments: 2
Patch Set 3 : bumm, just change the url "genius" :) #Messages
Total messages: 26 (9 generated)
b.kelemen@samsung.com changed reviewers: + agrieve@chromium.org, mmoss@chromium.org
PTAL
PTAL
On 2015/12/17 19:37:59, kbalazs wrote: > PTAL Seems reasonable to me, but I'll let agrieve decide whether to approve, since adding more gyp vars might be counter-productive to his plans to remove dependencies on gyp vars.
Description was changed from ========== Add opt-out from the runhook installing sysroot Do not download any sysroot during 'gclient runhooks' if use_sysroot=0 appears in GYP_DEFINES. For some reason my intranet blocks downloading it. Irrespectively of that I think it's reasonable to have an opt-out if one does not need sysroot. ========== to ========== Add opt-out from the runhook installing sysroot Do not download any sysroot during 'gclient runhooks' if use_sysroot=0 appears in GYP_DEFINES. For some reason my intranet blocks downloading it. Irrespectively of that I think it's reasonable to have an opt-out if one does not need sysroot. ==========
On 2015/12/17 19:56:43, Michael Moss wrote: > On 2015/12/17 19:37:59, kbalazs wrote: > > PTAL > > Seems reasonable to me, but I'll let agrieve decide whether to approve, since > adding more gyp vars might be counter-productive to his plans to remove > dependencies on gyp vars. +dranke & sbc, who have also been trying to make this sysroot logic better. I think the current thinking is well outlined here: https://code.google.com/p/chromium/issues/detail?id=570091 I'd personally prefer not to make this change for now, because it causes different behaviour for GYP vs GN. e.g. what if you have no sysroots for GYP, but *do* have sysroots for gn?
On 2015/12/17 21:37:14, agrieve wrote: > On 2015/12/17 19:56:43, Michael Moss wrote: > > On 2015/12/17 19:37:59, kbalazs wrote: > > > PTAL > > > > Seems reasonable to me, but I'll let agrieve decide whether to approve, since > > adding more gyp vars might be counter-productive to his plans to remove > > dependencies on gyp vars. > > +dranke & sbc, who have also been trying to make this sysroot logic better. > > I think the current thinking is well outlined here: > https://code.google.com/p/chromium/issues/detail?id=570091 > > I'd personally prefer not to make this change for now, because it causes > different behaviour for GYP vs GN. e.g. what if you have no sysroots for GYP, > but *do* have sysroots for gn? The script already has different behavior for gyp and gn, no? I.e. it just doesn't really work with gn. Yes, this relies on stuff that need to be reworked, but it fixes the default workflow for us. I don't know how this getting blocked by the stupid firewall but I have no control over that. We can approach the problem from a different angle too. I don't need sysroot. Why am I forced to download it???
sbc@chromium.org changed reviewers: + sbc@chromium.org
I'm fine with this. Its how the script used to work anyway. Out of interest though, are you able to download the other things from cloud storage during runhooks? e.g. gn and clang? Might be worth figuring out why the sysroot download is different. https://codereview.chromium.org/1534873002/diff/20001/build/linux/sysroot_scr... File build/linux/sysroot_scripts/install-sysroot.py (right): https://codereview.chromium.org/1534873002/diff/20001/build/linux/sysroot_scr... build/linux/sysroot_scripts/install-sysroot.py:223: gyp_defines = gyp_chromium.GetGypVars(supplemental_includes) Can you do this in main and pass gyp_defines to DetectTargetArch().
https://codereview.chromium.org/1534873002/diff/20001/build/linux/sysroot_scr... File build/linux/sysroot_scripts/install-sysroot.py (right): https://codereview.chromium.org/1534873002/diff/20001/build/linux/sysroot_scr... build/linux/sysroot_scripts/install-sysroot.py:138: and int(gyp_defines['use_sysroot']) == 0: What about just `gyp_defines.get('use_sysroot') == '0':`
dpranke@chromium.org changed reviewers: + dpranke@chromium.org
lgtm w/ Sam's suggested changes. We should not rely on setting globals.
Description was changed from ========== Add opt-out from the runhook installing sysroot Do not download any sysroot during 'gclient runhooks' if use_sysroot=0 appears in GYP_DEFINES. For some reason my intranet blocks downloading it. Irrespectively of that I think it's reasonable to have an opt-out if one does not need sysroot. ========== to ========== Download sysroot from "commondatastorage.googleapis.com" For some reason our intranet here is blocking googleapis.com. Actually other tools that are updated via runhooks are using that alias url already (e.g. clang). ==========
On 2015/12/17 22:15:45, Sam Clegg wrote: > I'm fine with this. Its how the script used to work anyway. > > Out of interest though, are you able to download the other things from cloud > storage during runhooks? e.g. gn and clang? Excellent point! Those scripts using an alias url that works well from here, problem solved. See new patch. I will wait for another l-g-t-m but although this time there is not much controversy I believe. :)
On 2015/12/18 17:52:56, kbalazs wrote: > On 2015/12/17 22:15:45, Sam Clegg wrote: > > I'm fine with this. Its how the script used to work anyway. > > > > Out of interest though, are you able to download the other things from cloud > > storage during runhooks? e.g. gn and clang? > > Excellent point! Those scripts using an alias url that works well from here, > problem solved. See new patch. > > I will wait for another l-g-t-m but although this time there is not much > controversy I believe. :) ANy
On 2015/12/18 19:13:57, Sam Clegg wrote: > On 2015/12/18 17:52:56, kbalazs wrote: > > On 2015/12/17 22:15:45, Sam Clegg wrote: > > > I'm fine with this. Its how the script used to work anyway. > > > > > > Out of interest though, are you able to download the other things from cloud > > > storage during runhooks? e.g. gn and clang? > > > > Excellent point! Those scripts using an alias url that works well from here, > > problem solved. See new patch. > > > > I will wait for another l-g-t-m but although this time there is not much > > controversy I believe. :) > > ANy Any idea why one URL works and the other doesn't for you? Would switching to https also fix the problem?
On 2015/12/18 19:14:28, Sam Clegg wrote: > On 2015/12/18 19:13:57, Sam Clegg wrote: > > On 2015/12/18 17:52:56, kbalazs wrote: > > > On 2015/12/17 22:15:45, Sam Clegg wrote: > > > > I'm fine with this. Its how the script used to work anyway. > > > > > > > > Out of interest though, are you able to download the other things from > cloud > > > > storage during runhooks? e.g. gn and clang? > > > > > > Excellent point! Those scripts using an alias url that works well from here, > > > problem solved. See new patch. > > > > > > I will wait for another l-g-t-m but although this time there is not much > > > controversy I believe. :) > > > > ANy > > Any idea why one URL works and the other doesn't for you? Would switching to > https also fix the problem? Someone somewhere messed up something. :) Sorry, I really have absolutely no idea and no means to figure it out.
Can I get a rubber-stamp please?
On 2015/12/21 18:47:12, kbalazs wrote: > Can I get a rubber-stamp please? Looks good to me. >Actually other tools that are updated via runhooks are using >that alias url already (e.g. clang). Maybe you can give some example CLs for this in the Description?
The CQ bit was checked by b.kelemen@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org Link to the patchset: https://codereview.chromium.org/1534873002/#ps40001 (title: "bumm, just change the url "genius" :)")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1534873002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1534873002/40001
Message was sent while issue was closed.
Description was changed from ========== Download sysroot from "commondatastorage.googleapis.com" For some reason our intranet here is blocking googleapis.com. Actually other tools that are updated via runhooks are using that alias url already (e.g. clang). ========== to ========== Download sysroot from "commondatastorage.googleapis.com" For some reason our intranet here is blocking googleapis.com. Actually other tools that are updated via runhooks are using that alias url already (e.g. clang). ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Download sysroot from "commondatastorage.googleapis.com" For some reason our intranet here is blocking googleapis.com. Actually other tools that are updated via runhooks are using that alias url already (e.g. clang). ========== to ========== Download sysroot from "commondatastorage.googleapis.com" For some reason our intranet here is blocking googleapis.com. Actually other tools that are updated via runhooks are using that alias url already (e.g. clang). Committed: https://crrev.com/dbf2f363526a4a1fc90af90206a2e0e7761eb75d Cr-Commit-Position: refs/heads/master@{#366481} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/dbf2f363526a4a1fc90af90206a2e0e7761eb75d Cr-Commit-Position: refs/heads/master@{#366481} |
