GN: Add an assert that sysroot exists

GN: Add an assert that sysroot exists

BUG=564869
Committed: https://crrev.com/45c8e9fdd57960affdfc7712faad605920eed3be
Cr-Commit-Position: refs/heads/master@{#363053}

[agrieve, 2015-12-02 21:07:45]

agrieve@chromium.org changed reviewers:
+ brettw@chromium.org, dpranke@google.com

[agrieve, 2015-12-02 21:07:45]

dpranke - plz review.

brettw - for .gn OWNERS.

[Dirk Pranke, 2015-12-02 22:55:44]

dpranke@chromium.org changed reviewers:
+ dpranke@chromium.org

[Dirk Pranke, 2015-12-02 22:55:44]

lgtm. my comment applies to cleanup we might want to do in a subsequent CL.

It might be nice if GN had a built in primitive to test for file or directory
existence, so we didn't need to shell out for this. I don't know how many other
places might want this, though, and whether this might be prone to "abuse" if we
provide it.

https://codereview.chromium.org/1493043002/diff/1/build/linux/sysroot_scripts...
File build/linux/sysroot_scripts/install-sysroot.py (right):

https://codereview.chromium.org/1493043002/diff/1/build/linux/sysroot_scripts...
build/linux/sysroot_scripts/install-sysroot.py:58: valid_archs = ['arm', 'i386',
'amd64', 'mips'] + ARCH_MAP.keys()
why do we need to support both 'i386' and 'x86' ? Can we just require users to
pass in 'x86' or 'x64' instead?

[agrieve, 2015-12-03 01:49:29]

https://codereview.chromium.org/1493043002/diff/1/build/linux/sysroot_scripts...
File build/linux/sysroot_scripts/install-sysroot.py (right):

https://codereview.chromium.org/1493043002/diff/1/build/linux/sysroot_scripts...
build/linux/sysroot_scripts/install-sysroot.py:58: valid_archs = ['arm', 'i386',
'amd64', 'mips'] + ARCH_MAP.keys()
On 2015/12/02 22:55:44, Dirk Pranke wrote:
> why do we need to support both 'i386' and 'x86' ? Can we just require users to
> pass in 'x86' or 'x64' instead?

I didn't want to have to apply the mapping within the GN assert(). My lame
reasoning:  I'd need to use a variable to do so, and since it's a .gni file, it
would leak into including files.

Added a comment saying that it's for accepting GN target_cpu values.

[brettw, 2015-12-03 17:57:53]

On 2015/12/03 01:49:29, agrieve wrote:
>
https://codereview.chromium.org/1493043002/diff/1/build/linux/sysroot_scripts...
> File build/linux/sysroot_scripts/install-sysroot.py (right):
> 
>
https://codereview.chromium.org/1493043002/diff/1/build/linux/sysroot_scripts...
> build/linux/sysroot_scripts/install-sysroot.py:58: valid_archs = ['arm',
'i386',
> 'amd64', 'mips'] + ARCH_MAP.keys()
> On 2015/12/02 22:55:44, Dirk Pranke wrote:
> > why do we need to support both 'i386' and 'x86' ? Can we just require users
to
> > pass in 'x86' or 'x64' instead?
> 
> I didn't want to have to apply the mapping within the GN assert(). My lame
> reasoning:  I'd need to use a variable to do so, and since it's a .gni file,
it
> would leak into including files.
> 
> Added a comment saying that it's for accepting GN target_cpu values.

In .gni files you can prefix variables with _ and they won't be exported to the
including file.

[agrieve, 2015-12-03 18:06:56]

On 2015/12/03 17:57:53, brettw wrote:
> On 2015/12/03 01:49:29, agrieve wrote:
> >
>
https://codereview.chromium.org/1493043002/diff/1/build/linux/sysroot_scripts...
> > File build/linux/sysroot_scripts/install-sysroot.py (right):
> > 
> >
>
https://codereview.chromium.org/1493043002/diff/1/build/linux/sysroot_scripts...
> > build/linux/sysroot_scripts/install-sysroot.py:58: valid_archs = ['arm',
> 'i386',
> > 'amd64', 'mips'] + ARCH_MAP.keys()
> > On 2015/12/02 22:55:44, Dirk Pranke wrote:
> > > why do we need to support both 'i386' and 'x86' ? Can we just require
users
> to
> > > pass in 'x86' or 'x64' instead?
> > 
> > I didn't want to have to apply the mapping within the GN assert(). My lame
> > reasoning:  I'd need to use a variable to do so, and since it's a .gni file,
> it
> > would leak into including files.
> > 
> > Added a comment saying that it's for accepting GN target_cpu values.
> 
> In .gni files you can prefix variables with _ and they won't be exported to
the
> including file.

fixed! TIL :)

[brettw, 2015-12-03 20:11:00]

lgtm

[agrieve, 2015-12-03 20:13:42]

The CQ bit was checked by agrieve@chromium.org

[agrieve, 2015-12-03 20:13:43]

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/1493043002/#ps20001
(title: "remove script changes")

[commit-bot: I haz the power, 2015-12-03 20:14:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1493043002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1493043002/20001

[commit-bot: I haz the power, 2015-12-03 20:56:58]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-12-03 20:57:58]

Description was changed from

==========
GN: Add an assert that sysroot exists

BUG=564869
==========

to

==========
GN: Add an assert that sysroot exists

BUG=564869

Committed: https://crrev.com/45c8e9fdd57960affdfc7712faad605920eed3be
Cr-Commit-Position: refs/heads/master@{#363053}
==========

[commit-bot: I haz the power, 2015-12-03 20:57:58]

Patchset 2 (id:??) landed as
https://crrev.com/45c8e9fdd57960affdfc7712faad605920eed3be
Cr-Commit-Position: refs/heads/master@{#363053}

[brettw, 2015-12-03 21:10:00]

For the Windows toolchain setup, I was thinking it would be good to have a
function that checks the timestamp of a file, or compares two file's
timetamps. That way we can avoid looking for VC and copying CRT files
around every time if they're up-to-date. Such a function might also return
0 if the file doesn't exist, allowing existence checks. I haven't thought
of any horrifying ways that this will be misused, but I've been wrong
before.

On Wed, Dec 2, 2015 at 2:55 PM, <dpranke@chromium.org> wrote:

> lgtm. my comment applies to cleanup we might want to do in a subsequent CL.
>
> It might be nice if GN had a built in primitive to test for file or
> directory
> existence, so we didn't need to shell out for this. I don't know how many
> other
> places might want this, though, and whether this might be prone to "abuse"
> if we
> provide it.
>
>
>
>
https://codereview.chromium.org/1493043002/diff/1/build/linux/sysroot_scripts...
> File build/linux/sysroot_scripts/install-sysroot.py (right):
>
>
>
https://codereview.chromium.org/1493043002/diff/1/build/linux/sysroot_scripts...
> build/linux/sysroot_scripts/install-sysroot.py:58: valid_archs = ['arm',
> 'i386', 'amd64', 'mips'] + ARCH_MAP.keys()
> why do we need to support both 'i386' and 'x86' ? Can we just require
> users to pass in 'x86' or 'x64' instead?
>
> https://codereview.chromium.org/1493043002/
>

-- 
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.