🎫 Fail write_build_config.py if current_toolchain != default_toolchain

Fail write_build_config.py if current_toolchain != default_toolchain

There are a few spots in the Android templates that assume appending
__build_config to a given target label will result in the label for a
write_build_config action. When toolchains exist in a label, appending
like this doesn't work. E.g.: :some_label(//toolchain)__build_config

Previously, we added a GN assert to enforce default toolchain, but that
was too strict, as it made it so no other targets that are defined in
the same BUILD.gn file could use a non-default toolchain.

This change make the error a build-time error rather than a gn gen
error.

BUG=629371
Committed: https://crrev.com/83bcd862038586061feea4b390ac524ab0a9a358
Cr-Commit-Position: refs/heads/master@{#408576}

[agrieve, 2016-07-19 14:16:40]

Description was changed from

==========
Fail write_build_config.py if current_toolchain != default_toolchain

There are a few spots in the Android templates that assume appending
__build_config to a given target label will result in the label for a
write_build_config action. When toolchains exist in a label, appending
like this doesn't work. E.g.: :some_label(//toolchain)__build_config

Previously, we added a GN assert to enforce default toolchain, but that
was too strict, as it made it so no other targets that are defined in
the same BUILD.gn file could use a non-default toolchain.

This change make the error a build-time error rather than a gn gen
error.

BUG=629371
==========

to

==========
Fail write_build_config.py if current_toolchain != default_toolchain

There are a few spots in the Android templates that assume appending
__build_config to a given target label will result in the label for a
write_build_config action. When toolchains exist in a label, appending
like this doesn't work. E.g.: :some_label(//toolchain)__build_config

Previously, we added a GN assert to enforce default toolchain, but that
was too strict, as it made it so no other targets that are defined in
the same BUILD.gn file could use a non-default toolchain.

This change make the error a build-time error rather than a gn gen
error.

BUG=629371
==========

[agrieve, 2016-07-19 14:16:41]

agrieve@chromium.org changed reviewers:
+ michaelbai@chromium.org

[agrieve, 2016-07-19 14:16:58]

The CQ bit was checked by agrieve@chromium.org to run a CQ dry run

[agrieve, 2016-07-19 14:17:02]

On 2016/07/19 14:16:58, agrieve wrote:
> The CQ bit was checked by mailto:agrieve@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-19 14:17:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-19 15:03:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-19 15:03:09]

Dry run: This issue passed the CQ dry run.

[michaelbai, 2016-07-19 18:13:30]

On 2016/07/19 14:17:02, agrieve wrote:
> On 2016/07/19 14:16:58, agrieve wrote:
> > The CQ bit was checked by mailto:agrieve@chromium.org to run a CQ dry run

Still make monochrome not work with non default toolchain

[75/2298] ACTION //third_party/android...(//build/toolchain/android:arm_v8_arm)
FAILED:
arm_v8_arm/gen/third_party/android_protobuf/protobuf_nano_javalib__build_config.d
arm_v8_arm/gen/third_party/android_protobuf/protobuf_nano_javalib.build_config 
python ../../build/android/gyp/write_build_config.py --type java_library
--depfile
arm_v8_arm/gen/third_party/android_protobuf/protobuf_nano_javalib__build_config.d
--deps-configs=\[\] --build-config
arm_v8_arm/gen/third_party/android_protobuf/protobuf_nano_javalib.build_config
--jar-path
arm_v8_arm/lib.java/third_party/android_protobuf/protobuf_nano_javalib.jar
--dex-path
arm_v8_arm/gen/third_party/android_protobuf/protobuf_nano_javalib.dex.jar
--supports-android --requires-android --java-sources-file
arm_v8_arm/gen/third_party/android_protobuf/protobuf_nano_javalib.sources
--fail=\[\"Tried\ to\ build\ an\ Android\ target\ in\ a\ non-default\
toolchain.\",\ \"target:\
//third_party/android_protobuf:protobuf_nano_javalib__build_config\(//build/toolchain/android:arm_v8_arm\)\",\
\"default_toolchain:\ //build/toolchain/android:arm64\"\]
Usage: write_build_config.py [options]

[agrieve, 2016-07-20 02:17:37]

On 2016/07/19 18:13:30, michaelbai wrote:
> On 2016/07/19 14:17:02, agrieve wrote:
> > On 2016/07/19 14:16:58, agrieve wrote:
> > > The CQ bit was checked by mailto:agrieve@chromium.org to run a CQ dry run
> 
> Still make monochrome not work with non default toolchain
> 
> [75/2298] ACTION
//third_party/android...(//build/toolchain/android:arm_v8_arm)
> FAILED:
>
arm_v8_arm/gen/third_party/android_protobuf/protobuf_nano_javalib__build_config.d
> arm_v8_arm/gen/third_party/android_protobuf/protobuf_nano_javalib.build_config

> python ../../build/android/gyp/write_build_config.py --type java_library
> --depfile
>
arm_v8_arm/gen/third_party/android_protobuf/protobuf_nano_javalib__build_config.d
> --deps-configs=\[\] --build-config
> arm_v8_arm/gen/third_party/android_protobuf/protobuf_nano_javalib.build_config
> --jar-path
> arm_v8_arm/lib.java/third_party/android_protobuf/protobuf_nano_javalib.jar
> --dex-path
> arm_v8_arm/gen/third_party/android_protobuf/protobuf_nano_javalib.dex.jar
> --supports-android --requires-android --java-sources-file
> arm_v8_arm/gen/third_party/android_protobuf/protobuf_nano_javalib.sources
> --fail=\[\"Tried\ to\ build\ an\ Android\ target\ in\ a\ non-default\
> toolchain.\",\ \"target:\
>
//third_party/android_protobuf:protobuf_nano_javalib__build_config\(//build/toolchain/android:arm_v8_arm\)\",\
> \"default_toolchain:\ //build/toolchain/android:arm64\"\]
> Usage: write_build_config.py [options]

This is likely a valid bug in what monochrome is doing. It shouldn't be
depending on any java targets in the secondary toolchain. Might be worth running
"gn path" to see what dep is pulling it in.

[agrieve, 2016-07-22 17:09:05]

The CQ bit was checked by agrieve@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-22 17:09:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-22 17:52:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-22 17:52:13]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[agrieve, 2016-07-25 17:43:38]

agrieve@chromium.org changed reviewers:
+ brettw@chromium.org

[agrieve, 2016-07-25 17:43:39]

Looks like there were some native targets that depended on java targets
unnecessarily. 

+brettw: Please review changes in random BUILD.gn files

[brettw, 2016-07-26 19:24:50]

If we know a step is going to fail, can we just make that a GN assert instead of
failing at buildtime?

[agrieve, 2016-07-26 20:05:48]

On 2016/07/26 19:24:50, brettw (ping on IM after 24h) wrote:
> If we know a step is going to fail, can we just make that a GN assert instead
of
> failing at buildtime?

Noted about that in the CL description. It doesn't work because of cases like:

shared_library("foo") {
}

android_library("bar") {
  deps = [":foo(//baz)"]
  assert(current_toolchain == default_toolchain)  # This will fail even though
the target is never depended upon.
}

Due to the large number of existing files that contain both native and android
targets, I deemed it too much effort / unwieldy to wrap them all in if
(current_toolchain != default_toolchain) guards.

[brettw, 2016-07-26 20:29:48]

lgtm

https://codereview.chromium.org/2161063003/diff/20001/build/config/android/in...
File build/config/android/internal_rules.gni (right):

https://codereview.chromium.org/2161063003/diff/20001/build/config/android/in...
build/config/android/internal_rules.gni:337: _msg = [
Can you add a comment here about why we fail with a build message rather than
assert. If I saw this code I would "fix" it.

[agrieve, 2016-07-27 02:27:13]

https://codereview.chromium.org/2161063003/diff/20001/build/config/android/in...
File build/config/android/internal_rules.gni (right):

https://codereview.chromium.org/2161063003/diff/20001/build/config/android/in...
build/config/android/internal_rules.gni:337: _msg = [
On 2016/07/26 20:29:48, brettw (ping on IM after 24h) wrote:
> Can you add a comment here about why we fail with a build message rather than
> assert. If I saw this code I would "fix" it.

Good point. Future me would be tempted to do the same.

[agrieve, 2016-07-27 02:27:20]

The CQ bit was checked by agrieve@chromium.org

[agrieve, 2016-07-27 02:27:20]

The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org
Link to the patchset: https://codereview.chromium.org/2161063003/#ps40001
(title: "add comment")

[commit-bot: I haz the power, 2016-07-27 02:27:40]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-27 03:38:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-27 03:38:03]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[agrieve, 2016-07-27 13:52:26]

The CQ bit was checked by agrieve@chromium.org

[commit-bot: I haz the power, 2016-07-27 13:52:36]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-27 14:55:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-27 14:55:43]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[agrieve, 2016-07-27 19:05:29]

The CQ bit was checked by agrieve@chromium.org

[commit-bot: I haz the power, 2016-07-27 19:06:00]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-27 20:17:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-27 20:18:02]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[agrieve, 2016-07-28 00:33:52]

The CQ bit was checked by agrieve@chromium.org

[commit-bot: I haz the power, 2016-07-28 00:34:06]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-28 01:42:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-28 01:42:07]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[agrieve, 2016-07-28 13:39:17]

The CQ bit was checked by agrieve@chromium.org

[commit-bot: I haz the power, 2016-07-28 13:39:31]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-28 13:48:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-28 13:48:56]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[agrieve, 2016-07-29 01:34:57]

The CQ bit was checked by agrieve@chromium.org

[commit-bot: I haz the power, 2016-07-29 01:35:13]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-29 02:39:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-29 02:39:17]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[agrieve, 2016-07-29 02:56:09]

The CQ bit was checked by agrieve@chromium.org

[agrieve, 2016-07-29 02:56:09]

The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org
Link to the patchset: https://codereview.chromium.org/2161063003/#ps60001
(title: "put back _java target removed from test()")

[commit-bot: I haz the power, 2016-07-29 02:56:29]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-29 04:26:33]

Description was changed from

==========
Fail write_build_config.py if current_toolchain != default_toolchain

There are a few spots in the Android templates that assume appending
__build_config to a given target label will result in the label for a
write_build_config action. When toolchains exist in a label, appending
like this doesn't work. E.g.: :some_label(//toolchain)__build_config

Previously, we added a GN assert to enforce default toolchain, but that
was too strict, as it made it so no other targets that are defined in
the same BUILD.gn file could use a non-default toolchain.

This change make the error a build-time error rather than a gn gen
error.

BUG=629371
==========

to

==========
Fail write_build_config.py if current_toolchain != default_toolchain

There are a few spots in the Android templates that assume appending
__build_config to a given target label will result in the label for a
write_build_config action. When toolchains exist in a label, appending
like this doesn't work. E.g.: :some_label(//toolchain)__build_config

Previously, we added a GN assert to enforce default toolchain, but that
was too strict, as it made it so no other targets that are defined in
the same BUILD.gn file could use a non-default toolchain.

This change make the error a build-time error rather than a gn gen
error.

BUG=629371
==========

[commit-bot: I haz the power, 2016-07-29 04:26:35]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-07-29 04:28:23]

Description was changed from

==========
Fail write_build_config.py if current_toolchain != default_toolchain

There are a few spots in the Android templates that assume appending
__build_config to a given target label will result in the label for a
write_build_config action. When toolchains exist in a label, appending
like this doesn't work. E.g.: :some_label(//toolchain)__build_config

Previously, we added a GN assert to enforce default toolchain, but that
was too strict, as it made it so no other targets that are defined in
the same BUILD.gn file could use a non-default toolchain.

This change make the error a build-time error rather than a gn gen
error.

BUG=629371
==========

to

==========
Fail write_build_config.py if current_toolchain != default_toolchain

There are a few spots in the Android templates that assume appending
__build_config to a given target label will result in the label for a
write_build_config action. When toolchains exist in a label, appending
like this doesn't work. E.g.: :some_label(//toolchain)__build_config

Previously, we added a GN assert to enforce default toolchain, but that
was too strict, as it made it so no other targets that are defined in
the same BUILD.gn file could use a non-default toolchain.

This change make the error a build-time error rather than a gn gen
error.

BUG=629371

Committed: https://crrev.com/83bcd862038586061feea4b390ac524ab0a9a358
Cr-Commit-Position: refs/heads/master@{#408576}
==========

[commit-bot: I haz the power, 2016-07-29 04:28:24]

Patchset 4 (id:??) landed as
https://crrev.com/83bcd862038586061feea4b390ac524ab0a9a358
Cr-Commit-Position: refs/heads/master@{#408576}