Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
321773002:
[PowerProfiler] Make sure correct version of pySerial is imported with monsoon profiler. (Closed)
|
Created:
6 years, 6 months ago by vivekg_samsung Modified:
6 years, 6 months ago CC:
chromium-reviews, telemetry+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
Description[PowerProfiler] Make sure correct version of pySerial is imported with monsoon profiler.
Python 2.7.x includes pySerial version 2.5 as a distribution package. This conflicts with
our repository version of pySerial i.e. 2.7. The submodule serial.tools.* was included in
pySerial after 2.6 as per [1].
The utility function util.AddDirToPythonPath(...) appends the path at the end of the path
list. Due to this, the python interpreter picks up the pySerial library from system include
path at /usr/lib/python2.7/dist-packages/serial. This CL modifies AddDirToPythonPath(...) API to always prepend the path.
[1] http://pyserial.sourceforge.net/pyserial_api.html#module-serial.tools.list_ports
NOTRY=true
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=276531
Patch Set 1 #
Total comments: 3
Patch Set 2 : Patch for landing! #Patch Set 3 : Added missing license text #
Total comments: 1
Patch Set 4 : #
Total comments: 6
Patch Set 5 : #Patch Set 6 : Updated unit test to remove version number hardcoding! #
Total comments: 1
Patch Set 7 : Patch for landing! #
Messages
Total messages: 38 (0 generated)
PTAL, thanks!
https://codereview.chromium.org/321773002/diff/1/tools/telemetry/telemetry/co... File tools/telemetry/telemetry/core/platform/power_monitor/monsoon_power_monitor_unittest.py (right): https://codereview.chromium.org/321773002/diff/1/tools/telemetry/telemetry/co... tools/telemetry/telemetry/core/platform/power_monitor/monsoon_power_monitor_unittest.py:23: def testPySerialVersionNumber(self): I think a better test would be to just import monsoon and then check monsoon.serial.VERSION. My reasoning is that this test should be testing that the monsoon is working correctly. Not that Add/InsertDirInPythonPath is working correctly. Tests for those methods should be in util_unittest.py. And a test there would want to be generic rather than dependent on pyserial. https://codereview.chromium.org/321773002/diff/1/tools/telemetry/telemetry/co... File tools/telemetry/telemetry/core/util.py (right): https://codereview.chromium.org/321773002/diff/1/tools/telemetry/telemetry/co... tools/telemetry/telemetry/core/util.py:38: def InsertDirInPythonPath(index, *path_parts): Dave should review this part. I wonder whether we should change AddDirToPythonPath to always prepend instead of append.
an entirely orthogonal suggestion :) in another life, I used "gnu's screen" to talk to a serial-over-usb, mostly: screen /dev/ttyUSB0 baudrate and it opens a process with stdin / stdout more details: http://playground.arduino.cc/Interfacing/LinuxTTY I'm not suggesting as part of this path, and I have no idea if it's available on mac, but it may be an alternative to avoid depending on pyserial. On 2014/06/09 16:13:35, tonyg wrote: > https://codereview.chromium.org/321773002/diff/1/tools/telemetry/telemetry/co... > File > tools/telemetry/telemetry/core/platform/power_monitor/monsoon_power_monitor_unittest.py > (right): > > https://codereview.chromium.org/321773002/diff/1/tools/telemetry/telemetry/co... > tools/telemetry/telemetry/core/platform/power_monitor/monsoon_power_monitor_unittest.py:23: > def testPySerialVersionNumber(self): > I think a better test would be to just import monsoon and then check > monsoon.serial.VERSION. > > My reasoning is that this test should be testing that the monsoon is working > correctly. Not that Add/InsertDirInPythonPath is working correctly. Tests for > those methods should be in util_unittest.py. And a test there would want to be > generic rather than dependent on pyserial. > > https://codereview.chromium.org/321773002/diff/1/tools/telemetry/telemetry/co... > File tools/telemetry/telemetry/core/util.py (right): > > https://codereview.chromium.org/321773002/diff/1/tools/telemetry/telemetry/co... > tools/telemetry/telemetry/core/util.py:38: def InsertDirInPythonPath(index, > *path_parts): > Dave should review this part. I wonder whether we should change > AddDirToPythonPath to always prepend instead of append.
https://codereview.chromium.org/321773002/diff/1/tools/telemetry/telemetry/co... File tools/telemetry/telemetry/core/util.py (right): https://codereview.chromium.org/321773002/diff/1/tools/telemetry/telemetry/co... tools/telemetry/telemetry/core/util.py:38: def InsertDirInPythonPath(index, *path_parts): On 2014/06/09 16:13:35, tonyg wrote: > Dave should review this part. I wonder whether we should change > AddDirToPythonPath to always prepend instead of append. +1 to this suggestion, we don't need an extra function for this.
Thank you all. I have incorporated the review comments. PTAL, thanks.
https://codereview.chromium.org/321773002/diff/40001/tools/telemetry/telemetr... File tools/telemetry/telemetry/core/platform/profiler/monsoon_unittest.py (right): https://codereview.chromium.org/321773002/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/platform/profiler/monsoon_unittest.py:10: self.assertEqual(monsoon.serial.VERSION, '2.7') Shouldn't this be in some util_unittest.py ? You should import serial from there and test VERSION, instead of relying on internal implementation details of the monsoon file.
On 2014/06/11 08:21:51, qsr wrote: > https://codereview.chromium.org/321773002/diff/40001/tools/telemetry/telemetr... > File tools/telemetry/telemetry/core/platform/profiler/monsoon_unittest.py > (right): > > https://codereview.chromium.org/321773002/diff/40001/tools/telemetry/telemetr... > tools/telemetry/telemetry/core/platform/profiler/monsoon_unittest.py:10: > self.assertEqual(monsoon.serial.VERSION, '2.7') > Shouldn't this be in some util_unittest.py ? You should import serial from there > and test VERSION, instead of relying on internal implementation details of the > monsoon file. Done.
https://codereview.chromium.org/321773002/diff/60001/tools/telemetry/telemetr... File tools/telemetry/telemetry/core/util.py (right): https://codereview.chromium.org/321773002/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/util.py:43: sys.path.remove(path) Do we really want to reinsert if it is already in the sys path? https://codereview.chromium.org/321773002/diff/60001/tools/telemetry/telemetr... File tools/telemetry/telemetry/core/util_unittest.py (right): https://codereview.chromium.org/321773002/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/util_unittest.py:66: class PySerialVersionTest(unittest.TestCase): Don't you need to call AddDirToPythonPath? Is it only working because something else has been doing this before you?
https://codereview.chromium.org/321773002/diff/60001/tools/telemetry/telemetr... File tools/telemetry/telemetry/core/util.py (right): https://codereview.chromium.org/321773002/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/util.py:43: sys.path.remove(path) On 2014/06/11 12:24:21, qsr wrote: > Do we really want to reinsert if it is already in the sys path? The reason for removing the existing path is to assure the path is indeed added at the beginning. https://codereview.chromium.org/321773002/diff/60001/tools/telemetry/telemetr... File tools/telemetry/telemetry/core/util_unittest.py (right): https://codereview.chromium.org/321773002/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/util_unittest.py:66: class PySerialVersionTest(unittest.TestCase): On 2014/06/11 12:24:21, qsr wrote: > Don't you need to call AddDirToPythonPath? Is it only working because something > else has been doing this before you? Yes you are right. We should call AddDirToPythonPath for the completeness of the test. I will change it. The reason its working is because of telemetry/core/platform/profiler/monsoon.py is calling this function at the import statement.
Uploaded new patch with explicit path addition in the unit test.
https://codereview.chromium.org/321773002/diff/60001/tools/telemetry/telemetr... File tools/telemetry/telemetry/core/util.py (right): https://codereview.chromium.org/321773002/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/util.py:43: sys.path.remove(path) On 2014/06/11 12:41:41, vivekg_ wrote: > On 2014/06/11 12:24:21, qsr wrote: > > Do we really want to reinsert if it is already in the sys path? > > The reason for removing the existing path is to assure the path is indeed added > at the beginning. What is the use case where it would be in the sys path at the wrong position?
https://chromiumcodereview.appspot.com/321773002/diff/60001/tools/telemetry/t... File tools/telemetry/telemetry/core/util_unittest.py (right): https://chromiumcodereview.appspot.com/321773002/diff/60001/tools/telemetry/t... tools/telemetry/telemetry/core/util_unittest.py:66: class PySerialVersionTest(unittest.TestCase): If your test was not failing, it is probably not the right one. Moreover, it will break when we update pyserial for no good reason. What about creating 2 directories with 2 stupid module with the same name and a variable with 2 different values, and checking that the order of import matters.
On 2014/06/11 13:20:54, qsr wrote: > https://codereview.chromium.org/321773002/diff/60001/tools/telemetry/telemetr... > File tools/telemetry/telemetry/core/util.py (right): > > https://codereview.chromium.org/321773002/diff/60001/tools/telemetry/telemetr... > tools/telemetry/telemetry/core/util.py:43: sys.path.remove(path) > On 2014/06/11 12:41:41, vivekg_ wrote: > > On 2014/06/11 12:24:21, qsr wrote: > > > Do we really want to reinsert if it is already in the sys path? > > > > The reason for removing the existing path is to assure the path is indeed > added > > at the beginning. > > What is the use case where it would be in the sys path at the wrong position? If the developer has set the PYTHONPATH="/usr/lib/python2.7:...:/home/user/chromium/src/tools/telemetry/third_party/pyserial" for some reason, then sys module would insert it "as is" at the beginning in sys.path. With this, as the path already exists, AddDirToPythonPath() would do nothing and wrong pyserial will be picked up.
On 2014/06/11 13:31:03, vivekg_ wrote: > On 2014/06/11 13:20:54, qsr wrote: > > > https://codereview.chromium.org/321773002/diff/60001/tools/telemetry/telemetr... > > File tools/telemetry/telemetry/core/util.py (right): > > > > > https://codereview.chromium.org/321773002/diff/60001/tools/telemetry/telemetr... > > tools/telemetry/telemetry/core/util.py:43: sys.path.remove(path) > > On 2014/06/11 12:41:41, vivekg_ wrote: > > > On 2014/06/11 12:24:21, qsr wrote: > > > > Do we really want to reinsert if it is already in the sys path? > > > > > > The reason for removing the existing path is to assure the path is indeed > > added > > > at the beginning. > > > > What is the use case where it would be in the sys path at the wrong position? > > If the developer has set the > PYTHONPATH="/usr/lib/python2.7:...:/home/user/chromium/src/tools/telemetry/third_party/pyserial" > for some reason, then sys module would insert it "as is" at the beginning in > sys.path. With this, as the path already exists, AddDirToPythonPath() would do > nothing and wrong pyserial will be picked up. If the developer has set PYTHONPATH itself, I see it as a good reason not to change this. What if it is trying a newer version of the library?
On 2014/06/11 13:38:24, qsr wrote: > On 2014/06/11 13:31:03, vivekg_ wrote: > > On 2014/06/11 13:20:54, qsr wrote: > > > > > > https://codereview.chromium.org/321773002/diff/60001/tools/telemetry/telemetr... > > > File tools/telemetry/telemetry/core/util.py (right): > > > > > > > > > https://codereview.chromium.org/321773002/diff/60001/tools/telemetry/telemetr... > > > tools/telemetry/telemetry/core/util.py:43: sys.path.remove(path) > > > On 2014/06/11 12:41:41, vivekg_ wrote: > > > > On 2014/06/11 12:24:21, qsr wrote: > > > > > Do we really want to reinsert if it is already in the sys path? > > > > > > > > The reason for removing the existing path is to assure the path is indeed > > > added > > > > at the beginning. > > > > > > What is the use case where it would be in the sys path at the wrong > position? > > > > If the developer has set the > > > PYTHONPATH="/usr/lib/python2.7:...:/home/user/chromium/src/tools/telemetry/third_party/pyserial" > > for some reason, then sys module would insert it "as is" at the beginning in > > sys.path. With this, as the path already exists, AddDirToPythonPath() would do > > nothing and wrong pyserial will be picked up. > > If the developer has set PYTHONPATH itself, I see it as a good reason not to > change this. What if it is trying a newer version of the library? Agreed. I will restore the logic in AddDirToPythonPath with append being replaced with insert(0,..). With the given scenario about the pyserial version, do we really need to have the unit test case for it?
On 2014/06/11 14:21:10, vivekg_ wrote: > On 2014/06/11 13:38:24, qsr wrote: > > On 2014/06/11 13:31:03, vivekg_ wrote: > > > On 2014/06/11 13:20:54, qsr wrote: > > > > > > > > > > https://codereview.chromium.org/321773002/diff/60001/tools/telemetry/telemetr... > > > > File tools/telemetry/telemetry/core/util.py (right): > > > > > > > > > > > > > > https://codereview.chromium.org/321773002/diff/60001/tools/telemetry/telemetr... > > > > tools/telemetry/telemetry/core/util.py:43: sys.path.remove(path) > > > > On 2014/06/11 12:41:41, vivekg_ wrote: > > > > > On 2014/06/11 12:24:21, qsr wrote: > > > > > > Do we really want to reinsert if it is already in the sys path? > > > > > > > > > > The reason for removing the existing path is to assure the path is > indeed > > > > added > > > > > at the beginning. > > > > > > > > What is the use case where it would be in the sys path at the wrong > > position? > > > > > > If the developer has set the > > > > > > PYTHONPATH="/usr/lib/python2.7:...:/home/user/chromium/src/tools/telemetry/third_party/pyserial" > > > for some reason, then sys module would insert it "as is" at the beginning in > > > sys.path. With this, as the path already exists, AddDirToPythonPath() would > do > > > nothing and wrong pyserial will be picked up. > > > > If the developer has set PYTHONPATH itself, I see it as a good reason not to > > change this. What if it is trying a newer version of the library? > > Agreed. I will restore the logic in AddDirToPythonPath with append being > replaced with insert(0,..). Thanks. > With the given scenario about the pyserial version, do we really need to have > the unit test case for it? I'll defer to the owners of this code to decide on this. I would be inclined to say that we do, just because it is now part of the API of this function to handle library conflicts.
On 2014/06/11 14:26:13, qsr wrote: > On 2014/06/11 14:21:10, vivekg_ wrote: > > On 2014/06/11 13:38:24, qsr wrote: > > > On 2014/06/11 13:31:03, vivekg_ wrote: > > > > On 2014/06/11 13:20:54, qsr wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/321773002/diff/60001/tools/telemetry/telemetr... > > > > > File tools/telemetry/telemetry/core/util.py (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/321773002/diff/60001/tools/telemetry/telemetr... > > > > > tools/telemetry/telemetry/core/util.py:43: sys.path.remove(path) > > > > > On 2014/06/11 12:41:41, vivekg_ wrote: > > > > > > On 2014/06/11 12:24:21, qsr wrote: > > > > > > > Do we really want to reinsert if it is already in the sys path? > > > > > > > > > > > > The reason for removing the existing path is to assure the path is > > indeed > > > > > added > > > > > > at the beginning. > > > > > > > > > > What is the use case where it would be in the sys path at the wrong > > > position? > > > > > > > > If the developer has set the > > > > > > > > > > PYTHONPATH="/usr/lib/python2.7:...:/home/user/chromium/src/tools/telemetry/third_party/pyserial" > > > > for some reason, then sys module would insert it "as is" at the beginning > in > > > > sys.path. With this, as the path already exists, AddDirToPythonPath() > would > > do > > > > nothing and wrong pyserial will be picked up. > > > > > > If the developer has set PYTHONPATH itself, I see it as a good reason not > to > > > change this. What if it is trying a newer version of the library? > > > > Agreed. I will restore the logic in AddDirToPythonPath with append being > > replaced with insert(0,..). > > Thanks. > > > With the given scenario about the pyserial version, do we really need to have > > the unit test case for it? > > I'll defer to the owners of this code to decide on this. I would be inclined to > say that we do, just because it is now part of the API of this function to > handle library conflicts. One more attempt at writing the unit test but this time without hardcoding the version number. This way, with the pyserial roll, we need not update this unit test.
https://codereview.chromium.org/321773002/diff/100001/tools/telemetry/telemet... File tools/telemetry/telemetry/core/util_unittest.py (right): https://codereview.chromium.org/321773002/diff/100001/tools/telemetry/telemet... tools/telemetry/telemetry/core/util_unittest.py:67: class PySerialVersionTest(unittest.TestCase): This is getting convoluted. Stepping back, what you really care about is that monsoon has a new enough pyserial to work properly. So let's just drop this test and verify that by adding "assert float(serial.VERSION) >= 2.7, 'Monsoon requires pyserial version 2.7 or later. You have %s' % serial.VERSION" to monsoon.py's __init__ method. With that, I think this patch will be ready to land.
On 2014/06/11 15:44:00, tonyg wrote: > https://codereview.chromium.org/321773002/diff/100001/tools/telemetry/telemet... > File tools/telemetry/telemetry/core/util_unittest.py (right): > > https://codereview.chromium.org/321773002/diff/100001/tools/telemetry/telemet... > tools/telemetry/telemetry/core/util_unittest.py:67: class > PySerialVersionTest(unittest.TestCase): > This is getting convoluted. Stepping back, what you really care about is that > monsoon has a new enough pyserial to work properly. > > So let's just drop this test and verify that by adding "assert > float(serial.VERSION) >= 2.7, 'Monsoon requires pyserial version 2.7 or later. > You have %s' % serial.VERSION" to monsoon.py's __init__ method. > > With that, I think this patch will be ready to land. Done. PTAL, thanks!
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vivek.vg@samsung.com/321773002/120001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
The CQ bit was checked by tonyg@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vivek.vg@samsung.com/321773002/120001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
The CQ bit was checked by vivekg@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vivek.vg@samsung.com/321773002/120001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
The CQ bit was checked by vivekg@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vivek.vg@samsung.com/321773002/120001
Message was sent while issue was closed.
Change committed as 276531 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
