GN: Add support for prebuilt instrumented libraries.

GN: Add support for prebuilt instrumented libraries.

TBR=glider@chromium.org

Committed: https://crrev.com/74f1d187956e64c25724b116ed06ca7d00b89917
Cr-Commit-Position: refs/heads/master@{#332148}

[Sam McNally, 2015-05-22 03:38:45]

Patchset #1 (id:1) has been deleted

[Sam McNally, 2015-05-22 03:39:32]

Patchset #1 (id:20001) has been deleted

[Sam McNally, 2015-05-22 04:06:23]

Patchset #1 (id:40001) has been deleted

[Sam McNally, 2015-05-22 04:22:11]

Patchset #1 (id:60001) has been deleted

[Sam McNally, 2015-05-22 05:16:52]

Patchset #1 (id:80001) has been deleted

[Sam McNally, 2015-05-22 05:18:27]

Patchset #1 (id:100001) has been deleted

[Sam McNally, 2015-05-22 05:56:49]

Patchset #1 (id:120001) has been deleted

[Sam McNally, 2015-05-22 06:03:54]

Patchset #1 (id:140001) has been deleted

[Sam McNally, 2015-05-22 06:27:49]

Patchset #1 (id:160001) has been deleted

[Sam McNally, 2015-05-22 06:32:45]

Patchset #1 (id:180001) has been deleted

[Sam McNally, 2015-05-22 06:36:20]

Patchset #1 (id:200001) has been deleted

[Sam McNally, 2015-05-22 08:56:14]

sammc@chromium.org changed reviewers:
+ dpranke@chromium.org, glider@chromium.org

[Sam McNally, 2015-05-22 08:56:14]

glider@chromium.org: Please review changes in third_party/instrumented_libraries

dpranke@chromium.org: Please review GN changes

[Dirk Pranke, 2015-05-22 20:19:37]

lgtm

[Sam McNally, 2015-05-26 01:16:04]

glider: ping

[earthdok, 2015-05-26 16:11:41]

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

[earthdok, 2015-05-28 18:27:46]

I'm the de-facto owner in third_party/instrumented_libraries (we never bothered
to add me to the list because I'm the only person who commits there anyway).
Unfortunately I'll be gone after tomorrow, and glider@ is currently in the
middle of a relocation.

https://codereview.chromium.org/1150193002/diff/250001/build/config/compiler/...
File build/config/compiler/BUILD.gn (left):

https://codereview.chromium.org/1150193002/diff/250001/build/config/compiler/...
build/config/compiler/BUILD.gn:178: # TODO(GYP): Support instrumented libraries
and custom libc++.
This isn't resolved. We need to support source builds in addition to binary
downloads.

It probably makes sense to decouple source builds from Chromium's build system
completely and support them only as a way to produce pre-built binaries for GCS.
But one way or another, this must be resolved before GYP builds stop working.

https://codereview.chromium.org/1150193002/diff/250001/third_party/instrument...
File third_party/instrumented_libraries/BUILD.gn (right):

https://codereview.chromium.org/1150193002/diff/250001/third_party/instrument...
third_party/instrumented_libraries/BUILD.gn:26:
":extract_prebuilt_instrumented_libraries",
You have to download the archive before you can extract it. See:

https://code.google.com/p/chromium/codesearch#chromium/src/DEPS&l=758

https://codereview.chromium.org/1150193002/diff/250001/third_party/instrument...
third_party/instrumented_libraries/BUILD.gn:50:
"$target_out_dir/$archive_name.txt",
The stamp file no longer references the Ubuntu release name. This means that the
user must clobber the build after performing an in-place upgrade. Unfortunately,
there won't be any user-friendly warning about this. Instead, the build will
fail at runtime in an unpredictable way, most likely with a bogus MSan report.
Can we do something about this?

https://codereview.chromium.org/1150193002/diff/250001/third_party/instrument...
File third_party/instrumented_libraries/scripts/unpack_binaries.py (right):

https://codereview.chromium.org/1150193002/diff/250001/third_party/instrument...
third_party/instrumented_libraries/scripts/unpack_binaries.py:17: return
'%s-%s.tgz' % (sanitizer_type, download_binaries.get_ubuntu_release())
Can we use consistent naming everywhere?

Here we have: sanitizer_type="msan-no-origins",
archive_name="msan-no-origins-trusty.tgz".

In GYP: archive_name="msan-no-origins".

In download_binaries.py: configuration="msan-no-origins",
archive_name="msan-no-origins-trusty.tgz".

In third_party/instrumented_libraries/BUILD.gn: sanitizer_type="msan",
archive_name="msan-no-origins".

[Sam McNally, 2015-05-29 08:33:00]

https://codereview.chromium.org/1150193002/diff/250001/build/config/compiler/...
File build/config/compiler/BUILD.gn (left):

https://codereview.chromium.org/1150193002/diff/250001/build/config/compiler/...
build/config/compiler/BUILD.gn:178: # TODO(GYP): Support instrumented libraries
and custom libc++.
On 2015/05/28 18:27:45, earthdok wrote:
> This isn't resolved. We need to support source builds in addition to binary
> downloads.
> 
> It probably makes sense to decouple source builds from Chromium's build system
> completely and support them only as a way to produce pre-built binaries for
GCS.
> But one way or another, this must be resolved before GYP builds stop working.

Moved the TODO into the instrumented_libraries BUILD.gn.

https://codereview.chromium.org/1150193002/diff/250001/third_party/instrument...
File third_party/instrumented_libraries/BUILD.gn (right):

https://codereview.chromium.org/1150193002/diff/250001/third_party/instrument...
third_party/instrumented_libraries/BUILD.gn:26:
":extract_prebuilt_instrumented_libraries",
On 2015/05/28 18:27:46, earthdok wrote:
> You have to download the archive before you can extract it. See:
> 
> https://code.google.com/p/chromium/codesearch#chromium/src/DEPS&l=758

This assumes that hook would still download the archive. However, I'm not sure
what the replacement would be for GYP_DEFINES for deciding whether and what to
download. Added a TODO(GYP) for now.

https://codereview.chromium.org/1150193002/diff/250001/third_party/instrument...
third_party/instrumented_libraries/BUILD.gn:50:
"$target_out_dir/$archive_name.txt",
On 2015/05/28 18:27:46, earthdok wrote:
> The stamp file no longer references the Ubuntu release name. This means that
the
> user must clobber the build after performing an in-place upgrade.
Unfortunately,
> there won't be any user-friendly warning about this. Instead, the build will
> fail at runtime in an unpredictable way, most likely with a bogus MSan report.
> Can we do something about this?

I don't think we can detect this unless ninja can detect an OS version change
and rerun GN.

https://codereview.chromium.org/1150193002/diff/250001/third_party/instrument...
File third_party/instrumented_libraries/scripts/unpack_binaries.py (right):

https://codereview.chromium.org/1150193002/diff/250001/third_party/instrument...
third_party/instrumented_libraries/scripts/unpack_binaries.py:17: return
'%s-%s.tgz' % (sanitizer_type, download_binaries.get_ubuntu_release())
On 2015/05/28 18:27:46, earthdok wrote:
> Can we use consistent naming everywhere?
> 
> Here we have: sanitizer_type="msan-no-origins",
> archive_name="msan-no-origins-trusty.tgz".
> 
> In GYP: archive_name="msan-no-origins".
> 
> In download_binaries.py: configuration="msan-no-origins",
> archive_name="msan-no-origins-trusty.tgz".
> 
> In third_party/instrumented_libraries/BUILD.gn: sanitizer_type="msan",
> archive_name="msan-no-origins".

Done.

[earthdok, 2015-05-29 15:27:36]

On 2015/05/29 08:33:00, Sam McNally wrote:
>
https://codereview.chromium.org/1150193002/diff/250001/build/config/compiler/...
> File build/config/compiler/BUILD.gn (left):
> 
>
https://codereview.chromium.org/1150193002/diff/250001/build/config/compiler/...
> build/config/compiler/BUILD.gn:178: # TODO(GYP): Support instrumented
libraries
> and custom libc++.
> On 2015/05/28 18:27:45, earthdok wrote:
> > This isn't resolved. We need to support source builds in addition to binary
> > downloads.
> > 
> > It probably makes sense to decouple source builds from Chromium's build
system
> > completely and support them only as a way to produce pre-built binaries for
> GCS.
> > But one way or another, this must be resolved before GYP builds stop
working.
> 
> Moved the TODO into the instrumented_libraries BUILD.gn.
> 
>
https://codereview.chromium.org/1150193002/diff/250001/third_party/instrument...
> File third_party/instrumented_libraries/BUILD.gn (right):
> 
>
https://codereview.chromium.org/1150193002/diff/250001/third_party/instrument...
> third_party/instrumented_libraries/BUILD.gn:26:
> ":extract_prebuilt_instrumented_libraries",
> On 2015/05/28 18:27:46, earthdok wrote:
> > You have to download the archive before you can extract it. See:
> > 
> > https://code.google.com/p/chromium/codesearch#chromium/src/DEPS&l=758
> 
> This assumes that hook would still download the archive. However, I'm not sure
> what the replacement would be for GYP_DEFINES for deciding whether and what to
> download. Added a TODO(GYP) for now.
> 
>
https://codereview.chromium.org/1150193002/diff/250001/third_party/instrument...
> third_party/instrumented_libraries/BUILD.gn:50:
> "$target_out_dir/$archive_name.txt",
> On 2015/05/28 18:27:46, earthdok wrote:
> > The stamp file no longer references the Ubuntu release name. This means that
> the
> > user must clobber the build after performing an in-place upgrade.
> Unfortunately,
> > there won't be any user-friendly warning about this. Instead, the build will
> > fail at runtime in an unpredictable way, most likely with a bogus MSan
report.
> > Can we do something about this?
> 
> I don't think we can detect this unless ninja can detect an OS version change
> and rerun GN.
> 
>
https://codereview.chromium.org/1150193002/diff/250001/third_party/instrument...
> File third_party/instrumented_libraries/scripts/unpack_binaries.py (right):
> 
>
https://codereview.chromium.org/1150193002/diff/250001/third_party/instrument...
> third_party/instrumented_libraries/scripts/unpack_binaries.py:17: return
> '%s-%s.tgz' % (sanitizer_type, download_binaries.get_ubuntu_release())
> On 2015/05/28 18:27:46, earthdok wrote:
> > Can we use consistent naming everywhere?
> > 
> > Here we have: sanitizer_type="msan-no-origins",
> > archive_name="msan-no-origins-trusty.tgz".
> > 
> > In GYP: archive_name="msan-no-origins".
> > 
> > In download_binaries.py: configuration="msan-no-origins",
> > archive_name="msan-no-origins-trusty.tgz".
> > 
> > In third_party/instrumented_libraries/BUILD.gn: sanitizer_type="msan",
> > archive_name="msan-no-origins".
> 
> Done.

lgtm w/comments

Feel free to TBR glider@.

[earthdok, 2015-05-29 15:28:03]

Oops, forgot to publish the comments.

https://codereview.chromium.org/1150193002/diff/250001/third_party/instrument...
File third_party/instrumented_libraries/BUILD.gn (right):

https://codereview.chromium.org/1150193002/diff/250001/third_party/instrument...
third_party/instrumented_libraries/BUILD.gn:26:
":extract_prebuilt_instrumented_libraries",
On 2015/05/29 08:33:00, Sam McNally wrote:
> On 2015/05/28 18:27:46, earthdok wrote:
> > You have to download the archive before you can extract it. See:
> > 
> > https://code.google.com/p/chromium/codesearch#chromium/src/DEPS&l=758
> 
> This assumes that hook would still download the archive. However, I'm not sure
> what the replacement would be for GYP_DEFINES for deciding whether and what to
> download. Added a TODO(GYP) for now.

You're probably aware of this, but just in case, I want to point out that
there're a number of hooks that look at GYP defines - this is not unique to
instrumented libraries.

https://codereview.chromium.org/1150193002/diff/250001/third_party/instrument...
third_party/instrumented_libraries/BUILD.gn:50:
"$target_out_dir/$archive_name.txt",
On 2015/05/29 08:33:00, Sam McNally wrote:
> On 2015/05/28 18:27:46, earthdok wrote:
> > The stamp file no longer references the Ubuntu release name. This means that
> the
> > user must clobber the build after performing an in-place upgrade.
> Unfortunately,
> > there won't be any user-friendly warning about this. Instead, the build will
> > fail at runtime in an unpredictable way, most likely with a bogus MSan
report.
> > Can we do something about this?
> 
> I don't think we can detect this unless ninja can detect an OS version change
> and rerun GN.

Ninja doesn't rerun GYP either, but GYP would at least extract the correct
archive if you rerun it explicitly. With GN you'd have to clobber or delete the
stamp file.

Anyway, this is a bit of a corner case, so I'll accept this if you can't or
don't want to run an external command from GN to get Ubuntu version. Please
leave a comment here at least (e.g. "This requires a clobber whenever Ubuntu
version changes.")

[Sam McNally, 2015-06-01 01:05:15]

https://codereview.chromium.org/1150193002/diff/250001/third_party/instrument...
File third_party/instrumented_libraries/BUILD.gn (right):

https://codereview.chromium.org/1150193002/diff/250001/third_party/instrument...
third_party/instrumented_libraries/BUILD.gn:50:
"$target_out_dir/$archive_name.txt",
On 2015/05/29 15:28:03, earthdok wrote:
> On 2015/05/29 08:33:00, Sam McNally wrote:
> > On 2015/05/28 18:27:46, earthdok wrote:
> > > The stamp file no longer references the Ubuntu release name. This means
that
> > the
> > > user must clobber the build after performing an in-place upgrade.
> > Unfortunately,
> > > there won't be any user-friendly warning about this. Instead, the build
will
> > > fail at runtime in an unpredictable way, most likely with a bogus MSan
> report.
> > > Can we do something about this?
> > 
> > I don't think we can detect this unless ninja can detect an OS version
change
> > and rerun GN.
> 
> Ninja doesn't rerun GYP either, but GYP would at least extract the correct
> archive if you rerun it explicitly. With GN you'd have to clobber or delete
the
> stamp file.
> 
> Anyway, this is a bit of a corner case, so I'll accept this if you can't or
> don't want to run an external command from GN to get Ubuntu version. Please
> leave a comment here at least (e.g. "This requires a clobber whenever Ubuntu
> version changes.")

Done.

[Sam McNally, 2015-06-01 01:06:55]

The CQ bit was checked by sammc@chromium.org

[Sam McNally, 2015-06-01 01:06:55]

The patchset sent to the CQ was uploaded after l-g-t-m from
dpranke@chromium.org, earthdok@chromium.org
Link to the patchset: https://codereview.chromium.org/1150193002/#ps310001
(title: " ")

[commit-bot: I haz the power, 2015-06-01 01:07:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1150193002/310001

[commit-bot: I haz the power, 2015-06-01 01:09:45]

Committed patchset #6 (id:310001)

[commit-bot: I haz the power, 2015-06-01 01:10:43]

Patchset 6 (id:??) landed as
https://crrev.com/74f1d187956e64c25724b116ed06ca7d00b89917
Cr-Commit-Position: refs/heads/master@{#332148}