Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1493913002:
Always install default host sysroots from gclient runhooks (Closed)
DescriptionAlways install default host sysroots from gclient runhooks
This removes some of the smarts that relied on GYP_DEFINES being set.
With the move to GN, GYP_DEFINES will often not be set.
BUG=564869
Committed: https://crrev.com/38d1fc620f41e8994079eb8532ed12631863d4c6
Cr-Commit-Position: refs/heads/master@{#363218}
Patch Set 1 #
Total comments: 9
Patch Set 2 : switch to dir_exists #
Total comments: 2
Patch Set 3 : review1 #Messages
Total messages: 17 (7 generated)
Description was changed from ========== Always install default host sysroots from gclient runhooks This removes some of the smarts that relied on GYP_DEFINES being set. With the move to GN, GYP_DEFINES will often not be set. BUG=564869 ========== to ========== Always install default host sysroots from gclient runhooks This removes some of the smarts that relied on GYP_DEFINES being set. With the move to GN, GYP_DEFINES will often not be set. BUG=564869 ==========
agrieve@chromium.org changed reviewers: + dpranke@chromium.org
On 2015/12/02 21:26:46, agrieve wrote: > mailto:agrieve@chromium.org changed reviewers: > + mailto:dpranke@chromium.org ❧❧❧
The code change is fine. I would rework the comments. One possible approach to fixing this w/o needing to look at GYP_DEFINES would be to look at the .gclient instead and see if it sets target_os = ["android"]. That would be a better solution, but even that wouldn't solve the problem for arm or linux cross-compiles, so I'm not sure that I would recommend it here yet. https://codereview.chromium.org/1493913002/diff/1/build/linux/sysroot_scripts... File build/linux/sysroot_scripts/install-sysroot.py (right): https://codereview.chromium.org/1493913002/diff/1/build/linux/sysroot_scripts... build/linux/sysroot_scripts/install-sysroot.py:108: # explicitly download sysroots. I don't think we want a bot-only solution, so this comment is not what I would write. I would change the comment to something like TODO: make this not depend on GYP_DEFINES so that it works w/ GN as well. https://codereview.chromium.org/1493913002/diff/1/build/linux/sysroot_scripts... build/linux/sysroot_scripts/install-sysroot.py:124: # means default setup. I don't think it's really clear what "(the default)" means in this situation. 32-bit android builds are the default *android* build, but in no sense is an android build the default build for linux hosts generally. I would rewrite this comment to something along the lines of "we don't have a good way as a gclient hook to figure out which of the sysroots we need, so install both of them to make both desktop linux builds and android builds happy".
sbc@chromium.org changed reviewers: + sbc@chromium.org
https://codereview.chromium.org/1493913002/diff/1/build/linux/sysroot_scripts... File build/linux/sysroot_scripts/install-sysroot.py (left): https://codereview.chromium.org/1493913002/diff/1/build/linux/sysroot_scripts... build/linux/sysroot_scripts/install-sysroot.py:117: return 0 Why not keep this early out? https://codereview.chromium.org/1493913002/diff/1/build/linux/sysroot_scripts... File build/linux/sysroot_scripts/install-sysroot.py (right): https://codereview.chromium.org/1493913002/diff/1/build/linux/sysroot_scripts... build/linux/sysroot_scripts/install-sysroot.py:121: if target_arch == 'amd64': Won't target_arch be 'arm' here for most android bots?
https://codereview.chromium.org/1493913002/diff/1/build/linux/sysroot_scripts... File build/linux/sysroot_scripts/install-sysroot.py (right): https://codereview.chromium.org/1493913002/diff/1/build/linux/sysroot_scripts... build/linux/sysroot_scripts/install-sysroot.py:121: if target_arch == 'amd64': On 2015/12/02 23:17:46, Sam Clegg wrote: > Won't target_arch be 'arm' here for most android bots? Not necessarily. The bots don't have to set target_arch=arm, because that's the default if os=android. Some might, some might not. That said, we should probably make sure anyone that *does* set target_arch=arm also still gets the host sysroots, so I think you're actually right that this patch is in fact wrong.
https://codereview.chromium.org/1493913002/diff/1/build/linux/sysroot_scripts... File build/linux/sysroot_scripts/install-sysroot.py (left): https://codereview.chromium.org/1493913002/diff/1/build/linux/sysroot_scripts... build/linux/sysroot_scripts/install-sysroot.py:117: return 0 On 2015/12/02 23:17:46, Sam Clegg wrote: > Why not keep this early out? Because you might have built with both GYP and GN in the same checkout, and your GN might require a sysroot while your GYP does not. https://codereview.chromium.org/1493913002/diff/1/build/linux/sysroot_scripts... File build/linux/sysroot_scripts/install-sysroot.py (right): https://codereview.chromium.org/1493913002/diff/1/build/linux/sysroot_scripts... build/linux/sysroot_scripts/install-sysroot.py:108: # explicitly download sysroots. On 2015/12/02 23:09:22, Dirk Pranke wrote: > I don't think we want a bot-only solution, so this comment is not what > I would write. > > I would change the comment to something like TODO: make this not depend on > GYP_DEFINES so that it works w/ GN as well. Done. https://codereview.chromium.org/1493913002/diff/1/build/linux/sysroot_scripts... build/linux/sysroot_scripts/install-sysroot.py:121: if target_arch == 'amd64': On 2015/12/02 23:20:38, Dirk Pranke wrote: > On 2015/12/02 23:17:46, Sam Clegg wrote: > > Won't target_arch be 'arm' here for most android bots? > > Not necessarily. The bots don't have to set target_arch=arm, because that's the > default if os=android. Some might, some might not. > > That said, we should probably make sure anyone that *does* set target_arch=arm > also still gets the host sysroots, > so I think you're actually right that this patch is in fact wrong. Reworked this to look for the presence of //third_party/android_tools instead. https://codereview.chromium.org/1493913002/diff/1/build/linux/sysroot_scripts... build/linux/sysroot_scripts/install-sysroot.py:124: # means default setup. On 2015/12/02 23:09:22, Dirk Pranke wrote: > I don't think it's really clear what "(the default)" means in this situation. > 32-bit android builds are the default *android* build, but in no sense is an > android build the default build for linux hosts generally. > > I would rewrite this comment to something along the lines of "we don't have a > good way as a gclient hook to figure out which of the sysroots we need, so > install both of them to make both desktop linux builds and android builds > happy". reworded.
lgtm w/ nit. https://codereview.chromium.org/1493913002/diff/20001/build/linux/sysroot_scr... File build/linux/sysroot_scripts/install-sysroot.py (right): https://codereview.chromium.org/1493913002/diff/20001/build/linux/sysroot_scr... build/linux/sysroot_scripts/install-sysroot.py:120: if os.path.exists(ANDROID_ONLY_DIR): nit: I'd change line 119 to 'elif os.path.exist(...) and dedent lines 121-123 and 128-132.
https://codereview.chromium.org/1493913002/diff/20001/build/linux/sysroot_scr... File build/linux/sysroot_scripts/install-sysroot.py (right): https://codereview.chromium.org/1493913002/diff/20001/build/linux/sysroot_scr... build/linux/sysroot_scripts/install-sysroot.py:120: if os.path.exists(ANDROID_ONLY_DIR): On 2015/12/04 00:55:08, Dirk Pranke wrote: > nit: I'd change line 119 to 'elif os.path.exist(...) and dedent lines 121-123 > and 128-132. Done.
The CQ bit was checked by agrieve@chromium.org
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/1493913002/#ps40001 (title: "review1")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1493913002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1493913002/40001
Message was sent while issue was closed.
Description was changed from ========== Always install default host sysroots from gclient runhooks This removes some of the smarts that relied on GYP_DEFINES being set. With the move to GN, GYP_DEFINES will often not be set. BUG=564869 ========== to ========== Always install default host sysroots from gclient runhooks This removes some of the smarts that relied on GYP_DEFINES being set. With the move to GN, GYP_DEFINES will often not be set. BUG=564869 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Always install default host sysroots from gclient runhooks This removes some of the smarts that relied on GYP_DEFINES being set. With the move to GN, GYP_DEFINES will often not be set. BUG=564869 ========== to ========== Always install default host sysroots from gclient runhooks This removes some of the smarts that relied on GYP_DEFINES being set. With the move to GN, GYP_DEFINES will often not be set. BUG=564869 Committed: https://crrev.com/38d1fc620f41e8994079eb8532ed12631863d4c6 Cr-Commit-Position: refs/heads/master@{#363218} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/38d1fc620f41e8994079eb8532ed12631863d4c6 Cr-Commit-Position: refs/heads/master@{#363218} |
