factory: utils: Make utility functions useful for remote DUT. |
|||
Issue descriptionCurrently, most of the utility functions are implemented to support local DUT only. Some of the functions support remote DUT by allowing callers to provide an instance of the system interface to use. It becomes messy because if the caller function is also an utility function, the only way to support remote DUT is to let the caller function to propagate the "dut" argument. Since the current implementation lets most of the utility functions unfriendly to remote DUT cases, station based tests have to implement a lot of their own helper functions. This bug is also related to chromium:703036
,
Dec 11
We had some prior work as in utils/sys_interface.py pihsun may also had some work by using thread variables set by context managers, but the cost and impact to performance would be huge (and some how not easy to identify if multiple context / layers were introduced. But well, you can try if there's some better way. Remember to check with stimim & pihsun for what has been tried...
,
Dec 11
A way to prevent argument propagation is to let the caller set the system interface instance as a thread local variables. CL:637554 shows an example:
## In a station based pytest ##
def runTest(self):
with sys_interface.SetSystemInterface(self.dut):
dut_version = json_utils.LoadFile('/blablabla/a.json')
## In json_utils.py ##
def LoadFile(path):
return json.loads(file_utils.ReadFile(path))
## In file_utils.py ##
def ReadFile(path):
dut = sys_interface.GetSystemInterface()
return dut.ReadFile(path)
However, developers then have to explicitly comment on the utility functions that they rely on the system interface. Moreover, when a developer implements a new utility function, which is designed to be a helper function for some local tasks, they need to be aware that the relied utility functions might depend on the system interface. For example, following code might behave not like what the developers think:
## In a station based pytest ##
...
def runTest(self):
with sys_interface.SetSystemInterface(self.dut):
dut_version = json_utils.LoadFile('/blablabla/a.json')
...
logger = log_utils.FileLogger(...) # bang!
logger.record_some_thing(...)
...
log_utils.FileLogger calls file_utils.TryMakeDirs to make the log file ready, but in the above case the directories will be created on the remote DUT. To solve the problem, I think we can provide a decorator:
@sys_interface.SystemAPI
def ReadFile(path):
dut = sys_interface.GetSystemInterface()
@sys_interface.SystemAPI
def LoadFile(json_path):
content = file_utils.ReadFile(json_path)
def FileLogger(...):
with sys_interface.SetSystemInterface(
sys_interface.SystemInterface()):
file_utils.TryMakeDirs()
The function with the decorator now calls "system utility function". And the decorator can do some check that: a system utility function can be invoked either:
1) from other packages
2) from system utility function
3) in the with-statement in any utility function
Following statements will trigger assertion errors.
def ReadFile(path):
dut = sys_interface.GetSystemInterface()
# assertion error, only system utility functions
# can get the system interface.
def LoadFile(json_path):
content = file_utils.ReadFile(json_path)
# assertion error, LoadFile should also be decorated.
def FileLogger(...):
file_utils.TryMakeDirs()
# assertion error, TryMakeDirs should be called in a with-statement.
,
Dec 11
,
Dec 11
Sounds like a good idea. The only concern is we are not sure if it will largely impact the runtime performance or not. Maybe we can change to checking this in unittest only if we found it's too slow in runtime.
,
Dec 29
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/platform/factory/+/65bda3120d57b591ef1d6faaf280d11c93bffb43 commit 65bda3120d57b591ef1d6faaf280d11c93bffb43 Author: Yong Hong <yhong@chromium.org> Date: Sat Dec 29 00:13:57 2018 sys_utils,gooftool/vpd: Move `VPDTool` into gooftool package. `cros.factory.utils` shouldn't have ChromeOS device specific logic so the code to manipulate the VPD data shouldn't locate in that package. And consider that both `gooftool` and device board needs to access the VPD data but `gooftool` can't depend on the device APIs, we move the code into gooftool package and let the device API imports them. BUG=chromium:913869 TEST=make test Change-Id: Ib1373e62c7171799c9e604cab8b21a0507323bb3 Reviewed-on: https://chromium-review.googlesource.com/1390079 Commit-Ready: ChromeOS CL Exonerator Bot <chromiumos-cl-exonerator@appspot.gserviceaccount.com> Tested-by: Yong Hong <yhong@google.com> Reviewed-by: Cheng-Han Yang <chenghan@chromium.org> [modify] https://crrev.com/65bda3120d57b591ef1d6faaf280d11c93bffb43/py/gooftool/gooftool_unittest.py [modify] https://crrev.com/65bda3120d57b591ef1d6faaf280d11c93bffb43/py/utils/sys_utils_unittest.py [add] https://crrev.com/65bda3120d57b591ef1d6faaf280d11c93bffb43/py/gooftool/vpd.py [modify] https://crrev.com/65bda3120d57b591ef1d6faaf280d11c93bffb43/py/device/vpd.py [modify] https://crrev.com/65bda3120d57b591ef1d6faaf280d11c93bffb43/py/gooftool/core.py [modify] https://crrev.com/65bda3120d57b591ef1d6faaf280d11c93bffb43/py/probe/functions/vpd.py [modify] https://crrev.com/65bda3120d57b591ef1d6faaf280d11c93bffb43/py/device/vpd_unittest.py [modify] https://crrev.com/65bda3120d57b591ef1d6faaf280d11c93bffb43/py/gooftool/common.py [modify] https://crrev.com/65bda3120d57b591ef1d6faaf280d11c93bffb43/py/hwid/v3/hwid_utils.py [modify] https://crrev.com/65bda3120d57b591ef1d6faaf280d11c93bffb43/py/probe/functions/vpd_unittest.py [modify] https://crrev.com/65bda3120d57b591ef1d6faaf280d11c93bffb43/py/gooftool/commands.py [add] https://crrev.com/65bda3120d57b591ef1d6faaf280d11c93bffb43/py/gooftool/vpd_unittest.py [modify] https://crrev.com/65bda3120d57b591ef1d6faaf280d11c93bffb43/py/utils/sys_utils.py
,
Dec 29
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/platform/factory/+/40a9e244f1951213414341af999d27738ddc1091 commit 40a9e244f1951213414341af999d27738ddc1091 Author: Yong Hong <yhong@chromium.org> Date: Sat Dec 29 00:13:58 2018 device/boards: Move `GetStartupMessages` from sys_utils to device APIs. The function `GetStartupMessages` calls device APIs like `ec.GetECConsoleLog` to collect log messages from various of parts. Besides, different platform actually contains different sets of components. Therefore, it's more suitable for the device board to implement their own method to obtain the startup messages. BUG=chromium:913869 TEST=make test Change-Id: Iec5129f9b1238889d699f5867c58c75b9c51c25e Reviewed-on: https://chromium-review.googlesource.com/1390080 Commit-Ready: ChromeOS CL Exonerator Bot <chromiumos-cl-exonerator@appspot.gserviceaccount.com> Tested-by: Yong Hong <yhong@google.com> Reviewed-by: Cheng-Han Yang <chenghan@chromium.org> [modify] https://crrev.com/40a9e244f1951213414341af999d27738ddc1091/py/test/pytests/shutdown.py [modify] https://crrev.com/40a9e244f1951213414341af999d27738ddc1091/py/utils/file_utils.py [modify] https://crrev.com/40a9e244f1951213414341af999d27738ddc1091/py/device/boards/chromeos.py [modify] https://crrev.com/40a9e244f1951213414341af999d27738ddc1091/py/utils/sys_utils_unittest.py [modify] https://crrev.com/40a9e244f1951213414341af999d27738ddc1091/py/goofy/hooks.py [add] https://crrev.com/40a9e244f1951213414341af999d27738ddc1091/py/device/boards/chromeos_unittest.py [modify] https://crrev.com/40a9e244f1951213414341af999d27738ddc1091/py/device/types.py [modify] https://crrev.com/40a9e244f1951213414341af999d27738ddc1091/py/device/boards/linux_unittest.py [modify] https://crrev.com/40a9e244f1951213414341af999d27738ddc1091/py/device/boards/linux.py [modify] https://crrev.com/40a9e244f1951213414341af999d27738ddc1091/py/utils/sys_utils.py
,
Dec 29
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/platform/factory/+/1037799210e46c05f69cac8574653946993df3c7 commit 1037799210e46c05f69cac8574653946993df3c7 Author: Yong Hong <yhong@chromium.org> Date: Sat Dec 29 00:13:58 2018 factory_bug: Check if ectool available or not by itself. This CL moves `cros.factory.utils.sys_utils.HasEC` into `cros.factory.tools.factory_bug` because: - `sys_utils` shouldn't include chromeos system specific logic. - No any other module calls that function. BUG=chromium:913869 TEST=make test Change-Id: I4b8666aeae8e1d378788b01ca9023ecf1852905a Reviewed-on: https://chromium-review.googlesource.com/1390081 Commit-Ready: ChromeOS CL Exonerator Bot <chromiumos-cl-exonerator@appspot.gserviceaccount.com> Tested-by: Yong Hong <yhong@google.com> Reviewed-by: Cheng-Han Yang <chenghan@chromium.org> [modify] https://crrev.com/1037799210e46c05f69cac8574653946993df3c7/py/utils/sys_utils.py [modify] https://crrev.com/1037799210e46c05f69cac8574653946993df3c7/py/tools/factory_bug.py |
|||
►
Sign in to add a comment |
|||
Comment 1 by yhong@chromium.org
, Dec 11