Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2294823002:
Remove lint variables from SDK version and path. (Closed)
|
Created:
4 years, 3 months ago by estevenson Modified:
4 years, 3 months ago Reviewers:
jbudorick CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove lint variables from SDK version and path.
The SDK used shouldn't be dependent on lint in any way, so make the lint variables dependent on the SDK instead.
BUG=640267
Committed: https://crrev.com/4fd1a48929eb71c7769a2b07299438397d039bca
Cr-Commit-Position: refs/heads/master@{#415800}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Address jbudorick comments #Messages
Total messages: 18 (9 generated)
The CQ bit was checked by estevenson@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...
estevenson@chromium.org changed reviewers: + jbudorick@chromium.org
ptal jbudorick!
https://codereview.chromium.org/2294823002/diff/1/build/config/android/config... File build/config/android/config.gni (right): https://codereview.chromium.org/2294823002/diff/1/build/config/android/config... build/config/android/config.gni:42: default_android_sdk_root = "//third_party/android_tools/sdk" This seems odd too. Why are lint_android_sdk_root and _version separate from the android_sdk_root and _version that are configurable below?
https://codereview.chromium.org/2294823002/diff/1/build/config/android/config... File build/config/android/config.gni (right): https://codereview.chromium.org/2294823002/diff/1/build/config/android/config... build/config/android/config.gni:42: default_android_sdk_root = "//third_party/android_tools/sdk" On 2016/08/30 15:49:10, jbudorick wrote: > This seems odd too. Why are lint_android_sdk_root and _version separate from the > android_sdk_root and _version that are configurable below? Because the defaults below can be set in other places and might not be "//third_party/android_tools/sdk". I hardcoded it here instead so that we don't need to make an additional downstream change. If you'd prefer, I can make the downstream change and use the same pattern for defaults in this file.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2294823002/diff/1/build/config/android/config... File build/config/android/config.gni (right): https://codereview.chromium.org/2294823002/diff/1/build/config/android/config... build/config/android/config.gni:42: default_android_sdk_root = "//third_party/android_tools/sdk" On 2016/08/30 16:43:51, estevenson wrote: > On 2016/08/30 15:49:10, jbudorick wrote: > > This seems odd too. Why are lint_android_sdk_root and _version separate from > the > > android_sdk_root and _version that are configurable below? > > Because the defaults below can be set in other places and might not be > "//third_party/android_tools/sdk". I hardcoded it here instead so that we don't > need to make an additional downstream change. If you'd prefer, I can make the > downstream change and use the same pattern for defaults in this file. I think I'd prefer a downstream change. Having a hard-coded lint path doesn't seem like the right default behavior to me.
Description was changed from ========== Remove lint variables from SDK version and path. The SDK used shouldn't be dependent on lint in any way, so it's better to just hardcode the lint paths. BUG=640267 ========== to ========== Remove lint variables from SDK version and path. The SDK used shouldn't be dependent on lint in any way, so make the lint variables dependent on the SDK instead. BUG=640267 ==========
ptal! https://codereview.chromium.org/2294823002/diff/1/build/config/android/config... File build/config/android/config.gni (right): https://codereview.chromium.org/2294823002/diff/1/build/config/android/config... build/config/android/config.gni:42: default_android_sdk_root = "//third_party/android_tools/sdk" On 2016/08/31 18:53:19, jbudorick wrote: > On 2016/08/30 16:43:51, estevenson wrote: > > On 2016/08/30 15:49:10, jbudorick wrote: > > > This seems odd too. Why are lint_android_sdk_root and _version separate from > > the > > > android_sdk_root and _version that are configurable below? > > > > Because the defaults below can be set in other places and might not be > > "//third_party/android_tools/sdk". I hardcoded it here instead so that we > don't > > need to make an additional downstream change. If you'd prefer, I can make the > > downstream change and use the same pattern for defaults in this file. > > I think I'd prefer a downstream change. Having a hard-coded lint path doesn't > seem like the right default behavior to me. Done.
lgtm
The CQ bit was checked by estevenson@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 ========== Remove lint variables from SDK version and path. The SDK used shouldn't be dependent on lint in any way, so make the lint variables dependent on the SDK instead. BUG=640267 ========== to ========== Remove lint variables from SDK version and path. The SDK used shouldn't be dependent on lint in any way, so make the lint variables dependent on the SDK instead. BUG=640267 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Remove lint variables from SDK version and path. The SDK used shouldn't be dependent on lint in any way, so make the lint variables dependent on the SDK instead. BUG=640267 ========== to ========== Remove lint variables from SDK version and path. The SDK used shouldn't be dependent on lint in any way, so make the lint variables dependent on the SDK instead. BUG=640267 Committed: https://crrev.com/4fd1a48929eb71c7769a2b07299438397d039bca Cr-Commit-Position: refs/heads/master@{#415800} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/4fd1a48929eb71c7769a2b07299438397d039bca Cr-Commit-Position: refs/heads/master@{#415800} |
