Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2103193002:
🎏 Fix google_play_services_default_java including legacy_http_javalib as a dep rather tha (Closed)
|
Created:
4 years, 5 months ago by agrieve Modified:
4 years, 5 months ago Reviewers:
paulmiller CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix google_play_services_default_java including legacy_http_javalib as a dep rather than a classpath-only jar
Also fixes proguard() GN template not rebasing paths when it should be.
BUG=624003
Committed: https://crrev.com/76923ea889e7f418fa2e48866c777e2cc635ab8c
Cr-Commit-Position: refs/heads/master@{#402688}
Patch Set 1 #
Total comments: 7
Messages
Total messages: 17 (7 generated)
The CQ bit was checked by agrieve@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...
Description was changed from ========== Fix google_play_services_default_java including legacy_http_javalib as a dep rather than a classpath-only jar Also fixes proguard() GN template not rebasing paths when it should be. BUG=624003 ========== to ========== Fix google_play_services_default_java including legacy_http_javalib as a dep rather than a classpath-only jar Also fixes proguard() GN template not rebasing paths when it should be. BUG=624003 ==========
agrieve@chromium.org changed reviewers: + paulmiller@chromium.org
On 2016/06/28 17:30:44, commit-bot: I haz the power wrote: > Dry run: CQ is trying da patch. Follow status at > > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2103193002/diff/1/build/config/android/intern... File build/config/android/internal_rules.gni (right): https://codereview.chromium.org/2103193002/diff/1/build/config/android/intern... build/config/android/internal_rules.gni:592: args += [ "--classpath=" + rebase_path(c, root_build_dir) ] I did the rebase in third_party/android/BUILD.gn, downstream. If we're doing it here, then we should remove it there. https://codereview.chromium.org/2103193002/diff/1/build/secondary/third_party... File build/secondary/third_party/android_tools/BUILD.gn (left): https://codereview.chromium.org/2103193002/diff/1/build/secondary/third_party... build/secondary/third_party/android_tools/BUILD.gn:130: ":legacy_http_javalib", If not here, then where is "legacy_http_javalib" being used? Should we remove it? https://codereview.chromium.org/2103193002/diff/1/build/secondary/third_party... File build/secondary/third_party/android_tools/BUILD.gn (right): https://codereview.chromium.org/2103193002/diff/1/build/secondary/third_party... build/secondary/third_party/android_tools/BUILD.gn:154: testonly = true What does "testonly" do?
https://codereview.chromium.org/2103193002/diff/1/build/config/android/intern... File build/config/android/internal_rules.gni (right): https://codereview.chromium.org/2103193002/diff/1/build/config/android/intern... build/config/android/internal_rules.gni:592: args += [ "--classpath=" + rebase_path(c, root_build_dir) ] On 2016/06/28 18:10:20, paulmiller wrote: > I did the rebase in third_party/android/BUILD.gn, downstream. If we're doing it > here, then we should remove it there. :( That's definitely the wrong place for it. We'll have to manually roll to fix it I think. (I don't see how to change this without breaking an existing already-rebased usage). https://codereview.chromium.org/2103193002/diff/1/build/secondary/third_party... File build/secondary/third_party/android_tools/BUILD.gn (left): https://codereview.chromium.org/2103193002/diff/1/build/secondary/third_party... build/secondary/third_party/android_tools/BUILD.gn:130: ":legacy_http_javalib", On 2016/06/28 18:10:21, paulmiller wrote: > If not here, then where is "legacy_http_javalib" being used? Should we remove > it? It's still used by some test targets. Possible that those could also be changed to use input_jars_paths, but we probably don't care enough just for test targets. Also possible that robolectric tests might need it. https://codereview.chromium.org/2103193002/diff/1/build/secondary/third_party... File build/secondary/third_party/android_tools/BUILD.gn (right): https://codereview.chromium.org/2103193002/diff/1/build/secondary/third_party... build/secondary/third_party/android_tools/BUILD.gn:154: testonly = true On 2016/06/28 18:10:21, paulmiller wrote: > What does "testonly" do? It will make gn gen fail if any target not marked as testonly depends on it.
LGTM but I'm not in OWNERS https://codereview.chromium.org/2103193002/diff/1/build/config/android/intern... File build/config/android/internal_rules.gni (right): https://codereview.chromium.org/2103193002/diff/1/build/config/android/intern... build/config/android/internal_rules.gni:592: args += [ "--classpath=" + rebase_path(c, root_build_dir) ] On 2016/06/28 18:35:05, agrieve wrote: > On 2016/06/28 18:10:20, paulmiller wrote: > > I did the rebase in third_party/android/BUILD.gn, downstream. If we're doing > it > > here, then we should remove it there. > > :( That's definitely the wrong place for it. We'll have to manually roll to fix > it I think. (I don't see how to change this without breaking an existing > already-rebased usage). I would have thought that rebase_path would be smart enough to work on paths that are already rebased. Is that not the case?
On 2016/06/28 18:40:48, paulmiller wrote: > LGTM but I'm not in OWNERS > > https://codereview.chromium.org/2103193002/diff/1/build/config/android/intern... > File build/config/android/internal_rules.gni (right): > > https://codereview.chromium.org/2103193002/diff/1/build/config/android/intern... > build/config/android/internal_rules.gni:592: args += [ "--classpath=" + > rebase_path(c, root_build_dir) ] > On 2016/06/28 18:35:05, agrieve wrote: > > On 2016/06/28 18:10:20, paulmiller wrote: > > > I did the rebase in third_party/android/BUILD.gn, downstream. If we're doing > > it > > > here, then we should remove it there. > > > > :( That's definitely the wrong place for it. We'll have to manually roll to > fix > > it I think. (I don't see how to change this without breaking an existing > > already-rebased usage). > > I would have thought that rebase_path would be smart enough to work on paths > that are already rebased. Is that not the case? Hmm, actually, it's not smart enough for relative paths, but it is smart enough for absolute paths. I'll make a downstream change first to convert to absolute paths so we don't need to break the auto-roller.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by agrieve@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 ========== Fix google_play_services_default_java including legacy_http_javalib as a dep rather than a classpath-only jar Also fixes proguard() GN template not rebasing paths when it should be. BUG=624003 ========== to ========== Fix google_play_services_default_java including legacy_http_javalib as a dep rather than a classpath-only jar Also fixes proguard() GN template not rebasing paths when it should be. BUG=624003 ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== Fix google_play_services_default_java including legacy_http_javalib as a dep rather than a classpath-only jar Also fixes proguard() GN template not rebasing paths when it should be. BUG=624003 ========== to ========== Fix google_play_services_default_java including legacy_http_javalib as a dep rather than a classpath-only jar Also fixes proguard() GN template not rebasing paths when it should be. BUG=624003 Committed: https://crrev.com/76923ea889e7f418fa2e48866c777e2cc635ab8c Cr-Commit-Position: refs/heads/master@{#402688} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/76923ea889e7f418fa2e48866c777e2cc635ab8c Cr-Commit-Position: refs/heads/master@{#402688} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
