New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 913869 link

Starred by 1 user

Issue metadata

Status: Assigned
Owner:
Last visit > 30 days ago
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Feature



Sign in to add a comment

factory: utils: Make utility functions useful for remote DUT.

Project Member Reported by yhong@chromium.org, Dec 11

Issue description

Currently, 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
 
Cc: jamesqaq@chromium.org
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...
Labels: -OS-Chrome
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.

Cc: pihsun@chromium.org
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.
Project Member

Comment 6 by bugdroid1@chromium.org, 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

Project Member

Comment 7 by bugdroid1@chromium.org, 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

Project Member

Comment 8 by bugdroid1@chromium.org, 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