Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2904813005:
Roll src/third_party/android_tools/ cb6bc2110..023e2f654 (1 commit). (Closed)
|
Created:
3 years, 7 months ago by DmitrySkiba Modified:
3 years, 6 months ago CC:
chromium-reviews Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionRoll src/third_party/android_tools/ cb6bc2110..023e2f654 (1 commit).
https://chromium.googlesource.com/android_tools.git/+log/cb6bc2110..023e2f654
023e2f6 Roll NDK to pick std::deque patch.
BUG=674287
Review-Url: https://codereview.chromium.org/2904813005
Cr-Commit-Position: refs/heads/master@{#478703}
Committed: https://chromium.googlesource.com/chromium/src/+/293876a7b9dcaa1a8d74d63b0ab272c54f7a8dcc
Patch Set 1 #Patch Set 2 : Force rebuild #Patch Set 3 : ANDROID_NDK_VERSION_ROLL #
Total comments: 1
Patch Set 4 : Rebase #Messages
Total messages: 31 (14 generated)
dskiba@chromium.org changed reviewers: + jbudorick@chromium.org
PTAL
lgtm
The CQ bit was checked by dskiba@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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...)
The CQ bit was checked by dskiba@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by jbudorick@google.com
On 2017/05/26 17:41:49, wrong jbudorick wrote: > The CQ bit was unchecked by mailto:jbudorick@google.com This is failing a lot of things on linux_android_rel_ng in ways that I think merit further investigation.
On 2017/05/26 17:42:52, wrong jbudorick wrote: > On 2017/05/26 17:41:49, wrong jbudorick wrote: > > The CQ bit was unchecked by mailto:jbudorick@google.com > > This is failing a lot of things on linux_android_rel_ng in ways that I think > merit further investigation. I'm investigating... But first I want to make sure that everything is rebuilt when this change is tried. I don't think we have a dependency from changed NDK deque file to everything, which means that I need to somehow force full rebuild.
On 2017/05/26 17:50:40, DmitrySkiba wrote: > On 2017/05/26 17:42:52, wrong jbudorick wrote: > > On 2017/05/26 17:41:49, wrong jbudorick wrote: > > > The CQ bit was unchecked by mailto:jbudorick@google.com > > > > This is failing a lot of things on linux_android_rel_ng in ways that I think > > merit further investigation. > > I'm investigating... But first I want to make sure that everything is rebuilt > when this change is tried. I don't think we have a dependency from changed NDK > deque file to everything, which means that I need to somehow force full rebuild. John, all tests in linux_android_rel_ng passes once I forced everything to rebuild, see the latest patchset. Previously not all files were rebuilding (e.g. 11K, 17K out of 27K), causing all sorts of mysterious bugs. Is there a way to force-rebuild everything without resorting to hacks?
On 2017/05/30 05:05:03, DmitrySkiba wrote: > On 2017/05/26 17:50:40, DmitrySkiba wrote: > > On 2017/05/26 17:42:52, wrong jbudorick wrote: > > > On 2017/05/26 17:41:49, wrong jbudorick wrote: > > > > The CQ bit was unchecked by mailto:jbudorick@google.com > > > > > > This is failing a lot of things on linux_android_rel_ng in ways that I think > > > merit further investigation. > > > > I'm investigating... But first I want to make sure that everything is rebuilt > > when this change is tried. I don't think we have a dependency from changed NDK > > deque file to everything, which means that I need to somehow force full > rebuild. > > John, all tests in linux_android_rel_ng passes once I forced everything to > rebuild, see the latest patchset. Previously not all files were rebuilding (e.g. > 11K, 17K out of 27K), causing all sorts of mysterious bugs. Is there a way to > force-rebuild everything without resorting to hacks? There are a couple of ways we could handle this: - we already define the ndk version as a way of forcing rebuild on NDK version updates (https://codesearch.chromium.org/chromium/src/build/config/android/BUILD.gn?rc...); we could do something similar but more granular here. - landmine, though I'd prefer we not use one here.
On 2017/05/30 14:23:38, jbudorick wrote: > On 2017/05/30 05:05:03, DmitrySkiba wrote: > > On 2017/05/26 17:50:40, DmitrySkiba wrote: > > > On 2017/05/26 17:42:52, wrong jbudorick wrote: > > > > On 2017/05/26 17:41:49, wrong jbudorick wrote: > > > > > The CQ bit was unchecked by mailto:jbudorick@google.com > > > > > > > > This is failing a lot of things on linux_android_rel_ng in ways that I > think > > > > merit further investigation. > > > > > > I'm investigating... But first I want to make sure that everything is > rebuilt > > > when this change is tried. I don't think we have a dependency from changed > NDK > > > deque file to everything, which means that I need to somehow force full > > rebuild. > > > > John, all tests in linux_android_rel_ng passes once I forced everything to > > rebuild, see the latest patchset. Previously not all files were rebuilding > (e.g. > > 11K, 17K out of 27K), causing all sorts of mysterious bugs. Is there a way to > > force-rebuild everything without resorting to hacks? > > There are a couple of ways we could handle this: > - we already define the ndk version as a way of forcing rebuild on NDK version > updates > (https://codesearch.chromium.org/chromium/src/build/config/android/BUILD.gn?rc...); > we could do something similar but more granular here. > - landmine, though I'd prefer we not use one here. Yeah, "zzzzz" change was temporary, just to test the hypothesis. See the last patchset where I included artificial "roll count" into ANDROID_NDK_VERSION and renamed it into ANDROID_NDK_VERSION_ROLL. Does it look better? Should I also change v8/gypfiles/standalone.gypi which has similar ANDROID_NDK_VERSION define?
lgtm https://codereview.chromium.org/2904813005/diff/40001/build/config/android/BU... File build/config/android/BUILD.gn (right): https://codereview.chromium.org/2904813005/diff/40001/build/config/android/BU... build/config/android/BUILD.gn:27: "ANDROID_NDK_VERSION_ROLL=${android_ndk_version}_1", I guess I'm ok with this as a short term fix. On thinking about it, I'd like to do eventually something a bit more automatic here, but doing so shouldn't block your change.
On 2017/06/08 18:01:21, jbudorick wrote: > lgtm > > https://codereview.chromium.org/2904813005/diff/40001/build/config/android/BU... > File build/config/android/BUILD.gn (right): > > https://codereview.chromium.org/2904813005/diff/40001/build/config/android/BU... > build/config/android/BUILD.gn:27: > "ANDROID_NDK_VERSION_ROLL=${android_ndk_version}_1", > I guess I'm ok with this as a short term fix. On thinking about it, I'd like to > do eventually something a bit more automatic here, but doing so shouldn't block > your change. Thanks! What about v8/gypfiles/standalone.gypi - should I change it too in the similar fashion?
On 2017/06/08 18:02:45, DmitrySkiba wrote: > On 2017/06/08 18:01:21, jbudorick wrote: > > lgtm > > > > > https://codereview.chromium.org/2904813005/diff/40001/build/config/android/BU... > > File build/config/android/BUILD.gn (right): > > > > > https://codereview.chromium.org/2904813005/diff/40001/build/config/android/BU... > > build/config/android/BUILD.gn:27: > > "ANDROID_NDK_VERSION_ROLL=${android_ndk_version}_1", > > I guess I'm ok with this as a short term fix. On thinking about it, I'd like > to > > do eventually something a bit more automatic here, but doing so shouldn't > block > > your change. > > Thanks! What about v8/gypfiles/standalone.gypi - should I change it too in the > similar fashion? gosh, I hope v8 isn't still using gyp, but +machenbach for that.
jbudorick@chromium.org changed reviewers: + machenbach@chromium.org
+machenbach re ^^^
The CQ bit was checked by dskiba@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm from v8 p-o-v. Our android paths in the gyp files have no coverage on our side. Maybe some embedders, but it'd be their problem. We're going to delete the files by end of year.
The CQ bit was checked by dskiba@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jbudorick@chromium.org Link to the patchset: https://codereview.chromium.org/2904813005/#ps60001 (title: "Rebase")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1497280806887100,
"parent_rev": "9e8cf3f662fb0c0ad5b662046851209f34a4b476", "commit_rev":
"293876a7b9dcaa1a8d74d63b0ab272c54f7a8dcc"}
Message was sent while issue was closed.
Description was changed from ========== Roll src/third_party/android_tools/ cb6bc2110..023e2f654 (1 commit). https://chromium.googlesource.com/android_tools.git/+log/cb6bc2110..023e2f654 023e2f6 Roll NDK to pick std::deque patch. BUG=674287 ========== to ========== Roll src/third_party/android_tools/ cb6bc2110..023e2f654 (1 commit). https://chromium.googlesource.com/android_tools.git/+log/cb6bc2110..023e2f654 023e2f6 Roll NDK to pick std::deque patch. BUG=674287 Review-Url: https://codereview.chromium.org/2904813005 Cr-Commit-Position: refs/heads/master@{#478703} Committed: https://chromium.googlesource.com/chromium/src/+/293876a7b9dcaa1a8d74d63b0ab2... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/293876a7b9dcaa1a8d74d63b0ab2... |
