Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
143623007:
Fix an errant pylint change. (Closed)
|
Created:
6 years, 10 months ago by jbudorick Modified:
6 years, 10 months ago CC:
chromium-reviews, craigdh+watch_chromium.org, bulach+watch_chromium.org, yfriedman+watch_chromium.org, ilevy-cc_chromium.org, klundberg+watch_chromium.org, frankf+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionI broke something by removing an apparently unused import that actually was used elsewhere. I addressed the issue by having the client code just call android_commands.GetAttachedDevices explicitly.
BUG=168518
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=250206
Patch Set 1 #
Total comments: 3
Patch Set 2 : #
Total comments: 1
Messages
Total messages: 19 (0 generated)
Surprise, surprise.
+cjhopman FYI https://codereview.chromium.org/143623007/diff/1/build/android/gyp/get_device... File build/android/gyp/get_device_configuration.py (right): https://codereview.chromium.org/143623007/diff/1/build/android/gyp/get_device... build/android/gyp/get_device_configuration.py:25: from pylib import android_commands I'm guessing the original motivation here was for //build/android/gyp to only depend on pylib through the util modules. Can we go back the way it was before the lint CL?
https://codereview.chromium.org/143623007/diff/1/build/android/gyp/get_device... File build/android/gyp/get_device_configuration.py (right): https://codereview.chromium.org/143623007/diff/1/build/android/gyp/get_device... build/android/gyp/get_device_configuration.py:25: from pylib import android_commands On 2014/02/10 18:35:12, frankf wrote: > I'm guessing the original motivation here was for //build/android/gyp to only > depend on pylib through the util modules. Can we go back the way it was before > the lint CL? We could, but what's the point in only visibly depending on pylib through the util modules? As is, this is just a sort of invisible dependency, which seems to me to be less clear.
lgtm to get the fix in. I don't have a strong opinion either way (Chris might though). build_device is a wrapper around (one-stop shop for) device operations. I don't know why //build/android/gyp duplicates a lot of utilities that are part of pylib. My guess is it wants to provide a more stable interface.
On 2014/02/10 18:47:00, frankf wrote: > lgtm to get the fix in. I don't have a strong opinion either way (Chris might > though). > > build_device is a wrapper around (one-stop shop for) device operations. I don't > know why //build/android/gyp duplicates a lot of utilities that are part of > pylib. My guess is it wants to provide a more stable interface. Given that, in another git branch, I'm working on blowing up the current interface entirely, I'm not too concerned about its stability...
https://codereview.chromium.org/143623007/diff/1/build/android/gyp/get_device... File build/android/gyp/get_device_configuration.py (right): https://codereview.chromium.org/143623007/diff/1/build/android/gyp/get_device... build/android/gyp/get_device_configuration.py:25: from pylib import android_commands On 2014/02/10 18:39:37, jbudorick wrote: > On 2014/02/10 18:35:12, frankf wrote: > > I'm guessing the original motivation here was for //build/android/gyp to only > > depend on pylib through the util modules. Can we go back the way it was before > > the lint CL? > > We could, but what's the point in only visibly depending on pylib through the > util modules? As is, this is just a sort of invisible dependency, which seems to > me to be less clear Because getting the gyp dependencies correct is much more difficult when we allow build/android/gyp to depend directly on pylib. It would be best to not depend on pylib at all.
On 2014/02/10 18:50:23, jbudorick wrote: > On 2014/02/10 18:47:00, frankf wrote: > > lgtm to get the fix in. I don't have a strong opinion either way (Chris might > > though). > > > > build_device is a wrapper around (one-stop shop for) device operations. I > don't > > know why //build/android/gyp duplicates a lot of utilities that are part of > > pylib. My guess is it wants to provide a more stable interface. > > Given that, in another git branch, I'm working on blowing up the current > interface entirely, I'm not too concerned about its stability... I am.
On 2014/02/10 18:50:46, cjhopman wrote: > https://codereview.chromium.org/143623007/diff/1/build/android/gyp/get_device... > File build/android/gyp/get_device_configuration.py (right): > > https://codereview.chromium.org/143623007/diff/1/build/android/gyp/get_device... > build/android/gyp/get_device_configuration.py:25: from pylib import > android_commands > On 2014/02/10 18:39:37, jbudorick wrote: > > On 2014/02/10 18:35:12, frankf wrote: > > > I'm guessing the original motivation here was for //build/android/gyp to > only > > > depend on pylib through the util modules. Can we go back the way it was > before > > > the lint CL? > > > > We could, but what's the point in only visibly depending on pylib through the > > util modules? As is, this is just a sort of invisible dependency, which seems > to > > me to be less clear > > Because getting the gyp dependencies correct is much more difficult when we > allow build/android/gyp to depend directly on pylib. It would be best to not > depend on pylib at all. Hmm... what would you like to see replace the functionality currently imported from pylib?
Restoring the original pre-pylint version with an inline pylint warning disable.
lgtm. Thanks John.
The CQ bit was checked by jbudorick@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jbudorick@chromium.org/143623007/110001
On 2014/02/10 19:02:01, jbudorick wrote: > On 2014/02/10 18:50:46, cjhopman wrote: > > > https://codereview.chromium.org/143623007/diff/1/build/android/gyp/get_device... > > File build/android/gyp/get_device_configuration.py (right): > > > > > https://codereview.chromium.org/143623007/diff/1/build/android/gyp/get_device... > > build/android/gyp/get_device_configuration.py:25: from pylib import > > android_commands > > On 2014/02/10 18:39:37, jbudorick wrote: > > > On 2014/02/10 18:35:12, frankf wrote: > > > > I'm guessing the original motivation here was for //build/android/gyp to > > only > > > > depend on pylib through the util modules. Can we go back the way it was > > before > > > > the lint CL? > > > > > > We could, but what's the point in only visibly depending on pylib through > the > > > util modules? As is, this is just a sort of invisible dependency, which > seems > > to > > > me to be less clear > > > > Because getting the gyp dependencies correct is much more difficult when we > > allow build/android/gyp to depend directly on pylib. It would be best to not > > depend on pylib at all. > > Hmm... what would you like to see replace the functionality currently imported > from pylib? So, I would love to share code with pylib. The issue is that for the build system, we want to keep the dependencies small and fairly stable (b/a/pylib has ~10x the changes as b/a/gyp). In the build system, we are also much more concerned with speed than the test scripts are. Ideally we could share code in something like b/a/shared, but that directory would then need to meet the requirements of both the build system and those of the test scripts.
https://codereview.chromium.org/143623007/diff/110001/build/android/gyp/util/... File build/android/gyp/util/build_device.py (right): https://codereview.chromium.org/143623007/diff/110001/build/android/gyp/util/... build/android/gyp/util/build_device.py:21: from pylib.android_commands import GetAttachedDevices # pylint: disable=W0611 I think to avoid the pylint warnings you could do something like: GetAttachedDevices = android_commands.GetAttachedDevices Not sure if that works or not.
On 2014/02/10 19:28:49, cjhopman wrote: > https://codereview.chromium.org/143623007/diff/110001/build/android/gyp/util/... > File build/android/gyp/util/build_device.py (right): > > https://codereview.chromium.org/143623007/diff/110001/build/android/gyp/util/... > build/android/gyp/util/build_device.py:21: from pylib.android_commands import > GetAttachedDevices # pylint: disable=W0611 > I think to avoid the pylint warnings you could do something like: > > GetAttachedDevices = android_commands.GetAttachedDevices > > > Not sure if that works or not. Ah, didn't notice it was already in the CQ. Don't stop it on my account.
On 2014/02/10 19:27:33, cjhopman wrote: > On 2014/02/10 19:02:01, jbudorick wrote: > > On 2014/02/10 18:50:46, cjhopman wrote: > > > > > > https://codereview.chromium.org/143623007/diff/1/build/android/gyp/get_device... > > > File build/android/gyp/get_device_configuration.py (right): > > > > > > > > > https://codereview.chromium.org/143623007/diff/1/build/android/gyp/get_device... > > > build/android/gyp/get_device_configuration.py:25: from pylib import > > > android_commands > > > On 2014/02/10 18:39:37, jbudorick wrote: > > > > On 2014/02/10 18:35:12, frankf wrote: > > > > > I'm guessing the original motivation here was for //build/android/gyp to > > > only > > > > > depend on pylib through the util modules. Can we go back the way it was > > > before > > > > > the lint CL? > > > > > > > > We could, but what's the point in only visibly depending on pylib through > > the > > > > util modules? As is, this is just a sort of invisible dependency, which > > seems > > > to > > > > me to be less clear > > > > > > Because getting the gyp dependencies correct is much more difficult when we > > > allow build/android/gyp to depend directly on pylib. It would be best to not > > > depend on pylib at all. > > > > Hmm... what would you like to see replace the functionality currently imported > > from pylib? > > So, I would love to share code with pylib. The issue is that for the build > system, we want to keep the dependencies small and fairly stable (b/a/pylib has > ~10x the changes as b/a/gyp). In the build system, we are also much more > concerned with speed than the test scripts are. Ideally we could share code in > something like b/a/shared, but that directory would then need to meet the > requirements of both the build system and those of the test scripts. Noted. I'll keep that in mind as I make changes in b/a/* in the future.
On 2014/02/10 19:28:49, cjhopman wrote: > https://codereview.chromium.org/143623007/diff/110001/build/android/gyp/util/... > File build/android/gyp/util/build_device.py (right): > > https://codereview.chromium.org/143623007/diff/110001/build/android/gyp/util/... > build/android/gyp/util/build_device.py:21: from pylib.android_commands import > GetAttachedDevices # pylint: disable=W0611 > I think to avoid the pylint warnings you could do something like: > > GetAttachedDevices = android_commands.GetAttachedDevices > > > Not sure if that works or not. pylint is indeed ok with this.
Message was sent while issue was closed.
Change committed as 250206
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/159453002/ by jbudorick@chromium.org. The reason for reverting is: crbug.com/342539 is likely happening because of the patch that this patch depends on. I need to ensure that logcats work on the bots before relanding (and that the corresponding steps fail if they don't.). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
