Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2150933007:
[Cronet] Use gn desc to find third party licenses on Android. (Closed)
Description[Cronet] Use gn desc to find third party licenses on Android.
BUG=606859
Committed: https://crrev.com/ccb7680f041642d1c9f957c335996bd239e611c4
Cr-Commit-Position: refs/heads/master@{#406331}
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
Patch Set 1 #
Total comments: 5
Patch Set 2 : Added comment. #
Total comments: 3
Patch Set 3 : Added TODO. #Patch Set 4 : Sync #Patch Set 5 : Sync #Patch Set 6 : Pass gn-path for gn inside of //buildtools. #Messages
Total messages: 23 (12 generated)
Description was changed from ========== [Cronet] Use gn deps to find third party licenses on Android. BUG=606859 ========== to ========== [Cronet] Use gn desc to find third party licenses on Android. BUG=606859 ==========
mef@chromium.org changed reviewers: + xunjieli@chromium.org
Hi Helen, PTAL.
Sorry for the delay! I was on bug triage last Friday. https://codereview.chromium.org/2150933007/diff/1/components/cronet/tools/cro... File components/cronet/tools/cronet_licenses.py (right): https://codereview.chromium.org/2150933007/diff/1/components/cronet/tools/cro... components/cronet/tools/cronet_licenses.py:83: if ("third_party" in build_dep and not "android_tools" in build_dep and Could you add a brief comment here on why we are excluding "android_tools" ? https://codereview.chromium.org/2150933007/diff/1/components/cronet/tools/cro... components/cronet/tools/cronet_licenses.py:98: parser.add_option('--gn', help='Use gn deps to find third party dependencies', The "--gn" option is added here, why do we need it in components/cronet/android/BUILD.gn line 820 too?
Thanks for your comments! https://codereview.chromium.org/2150933007/diff/1/components/cronet/tools/cro... File components/cronet/tools/cronet_licenses.py (right): https://codereview.chromium.org/2150933007/diff/1/components/cronet/tools/cro... components/cronet/tools/cronet_licenses.py:83: if ("third_party" in build_dep and not "android_tools" in build_dep and On 2016/07/18 14:54:50, xunjieli wrote: > Could you add a brief comment here on why we are excluding "android_tools" ? Done. There is no license in build/secondary/third_party/android_tools. https://codereview.chromium.org/2150933007/diff/1/components/cronet/tools/cro... components/cronet/tools/cronet_licenses.py:98: parser.add_option('--gn', help='Use gn deps to find third party dependencies', On 2016/07/18 14:54:50, xunjieli wrote: > The "--gn" option is added here, why do we need it in > components/cronet/android/BUILD.gn line 820 too? It is added here to option parser, so it is recognized. We need it in components/cronet/android/BUILD.gn line 820 to pass it to the script (otherwise it will use hard-coded list defined at line 26).
LGTM! one question below. https://codereview.chromium.org/2150933007/diff/1/components/cronet/tools/cro... File components/cronet/tools/cronet_licenses.py (right): https://codereview.chromium.org/2150933007/diff/1/components/cronet/tools/cro... components/cronet/tools/cronet_licenses.py:98: parser.add_option('--gn', help='Use gn deps to find third party dependencies', On 2016/07/18 15:45:20, mef wrote: > On 2016/07/18 14:54:50, xunjieli wrote: > > The "--gn" option is added here, why do we need it in > > components/cronet/android/BUILD.gn line 820 too? > > It is added here to option parser, so it is recognized. > We need it in components/cronet/android/BUILD.gn line 820 to pass it to the > script (otherwise it will use hard-coded list defined at line 26). You are right! I missed that part of the code. Thanks for explaining. https://codereview.chromium.org/2150933007/diff/20001/components/cronet/tools... File components/cronet/tools/cronet_licenses.py (right): https://codereview.chromium.org/2150933007/diff/20001/components/cronet/tools... components/cronet/tools/cronet_licenses.py:26: third_party_dirs = [ Can this be removed now? since the dependencies are being read from gn --desc? or are we waiting for iOS to switch to GN?
Thanks! https://codereview.chromium.org/2150933007/diff/20001/components/cronet/tools... File components/cronet/tools/cronet_licenses.py (right): https://codereview.chromium.org/2150933007/diff/20001/components/cronet/tools... components/cronet/tools/cronet_licenses.py:26: third_party_dirs = [ On 2016/07/18 21:33:12, xunjieli wrote: > Can this be removed now? since the dependencies are being read from gn --desc? > or are we waiting for iOS to switch to GN? Exactly, I would wait until GYP support is deprecated and removed. I've added a TODO.
makes sense. LGTM https://codereview.chromium.org/2150933007/diff/20001/components/cronet/tools... File components/cronet/tools/cronet_licenses.py (right): https://codereview.chromium.org/2150933007/diff/20001/components/cronet/tools... components/cronet/tools/cronet_licenses.py:26: third_party_dirs = [ On 2016/07/18 22:01:04, mef wrote: > On 2016/07/18 21:33:12, xunjieli wrote: > > Can this be removed now? since the dependencies are being read from gn --desc? > > or are we waiting for iOS to switch to GN? > > Exactly, I would wait until GYP support is deprecated and removed. I've added a > TODO. Acknowledged.
The CQ bit was checked by mef@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [Cronet] Use gn desc to find third party licenses on Android. BUG=606859 ========== to ========== [Cronet] Use gn desc to find third party licenses on Android. BUG=606859 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [Cronet] Use gn desc to find third party licenses on Android. BUG=606859 ========== to ========== [Cronet] Use gn desc to find third party licenses on Android. BUG=606859 Committed: https://crrev.com/ccb7680f041642d1c9f957c335996bd239e611c4 Cr-Commit-Position: refs/heads/master@{#406331} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/ccb7680f041642d1c9f957c335996bd239e611c4 Cr-Commit-Position: refs/heads/master@{#406331}
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.chromium.org/2162213002/ by mef@chromium.org. The reason for reverting is: Broke the builders by not finding the gn..
Message was sent while issue was closed.
Description was changed from ========== [Cronet] Use gn desc to find third party licenses on Android. BUG=606859 Committed: https://crrev.com/ccb7680f041642d1c9f957c335996bd239e611c4 Cr-Commit-Position: refs/heads/master@{#406331} ========== to ========== [Cronet] Use gn desc to find third party licenses on Android. BUG=606859 Committed: https://crrev.com/ccb7680f041642d1c9f957c335996bd239e611c4 Cr-Commit-Position: refs/heads/master@{#406331} ==========
Description was changed from ========== [Cronet] Use gn desc to find third party licenses on Android. BUG=606859 Committed: https://crrev.com/ccb7680f041642d1c9f957c335996bd239e611c4 Cr-Commit-Position: refs/heads/master@{#406331} ========== to ========== [Cronet] Use gn desc to find third party licenses on Android. BUG=606859 Committed: https://crrev.com/ccb7680f041642d1c9f957c335996bd239e611c4 Cr-Commit-Position: refs/heads/master@{#406331} CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ==========
The CQ bit was checked by mef@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.
On 2016/07/19 22:25:26, mef wrote: > A revert of this CL (patchset #3 id:40001) has been created in > https://codereview.chromium.org/2162213002/ by mailto:mef@chromium.org. > > The reason for reverting is: Broke the builders by not finding the gn.. This CL is replaced by https://codereview.chromium.org/2959303002/ so I'll close it.
Description was changed from ========== [Cronet] Use gn desc to find third party licenses on Android. BUG=606859 Committed: https://crrev.com/ccb7680f041642d1c9f957c335996bd239e611c4 Cr-Commit-Position: refs/heads/master@{#406331} CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ========== to ========== [Cronet] Use gn desc to find third party licenses on Android. BUG=606859 Committed: https://crrev.com/ccb7680f041642d1c9f957c335996bd239e611c4 Cr-Commit-Position: refs/heads/master@{#406331} CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ========== |
