DpdkTestpmd: Installation path cleanup #3426
Conversation
mcgov
force-pushed
the
mcgov/test-install
branch
2 times, most recently
from
b0a06fa
to
0eecb81
Compare
lisa/tools/python.py
Outdated
| @@ -60,9 +60,14 @@ def _install(self) -> bool: | |||
| self.node.os.install_packages(package_name) | |||
| return self._check_exists() | |||
|
|
|||
| def install_packages(self, packages_name: str, install_path: str = "") -> None: | |||
| def install_packages( | |||
| self, packages_name: str, install_path: str = "", user: bool = False | |||
| if not isinstance(node.os, (Debian, Fedora, Suse)): | ||
| return False | ||
| if isinstance(node.os, Ubuntu) and node.os.information.codename == "bionic": | ||
| node.os._update_packages() |
There was a problem hiding this comment.
Don't call private methods out of the class.
|
|
||
|
|
||
| # base class for source installs | ||
| class SourceInstall(Installer): |
There was a problem hiding this comment.
The sourceinstall class has no special implementation for source installation, it looks not needed.
There was a problem hiding this comment.
I'm going to leave it, even though it doesn't do much.
- It sets _checkout_path
- It serves as a common class for checking isinstance(installer, SourceInstall)
I'm going to re-use this code for rdma-core as well, so having common parent classes is going to be useful.
microsoft/testsuites/dpdk/common.py
Outdated
|
|
||
| def _run_installation(self) -> None: | ||
| # define to handle any special special cases | ||
| self._setup_node() |
There was a problem hiding this comment.
It's called in do_installation, why call it again in _run_installation, which is called by do_installation too?
There was a problem hiding this comment.
To be honest, I wrote this one first! I'll fix it up since it probably doesn't even need to be defined now.
There was a problem hiding this comment.
fixed, thanks for taking a look!
lisa/tools/meson.py
Outdated
| return "meson" | ||
|
|
||
| @property | ||
| def exists(self) -> bool: |
There was a problem hiding this comment.
rewrite _check_exists is better to reuse logic.
microsoft/testsuites/dpdk/common.py
Outdated
| self.packages = packages | ||
|
|
||
|
|
||
| class InstallDependencies: |
There was a problem hiding this comment.
I don't see an advantage of defining this as a function, I'll rename it to DependencyInstaller to make the purpose more clear.
There was a problem hiding this comment.
If a class host only one function, and no data used, it should be a function. Some languages like C#, Jave are purely object oriented, so they cannot use function. But Python uses mixed functions and oo. Avoid overusing classes, so the remaining classes are easier to be understood.
There was a problem hiding this comment.
I appreciate all of that, I agree with all of what you've said. But:
The os match -> package list is data. I think it's cleaner to encapsulate these dependencies so I can pass an instance of this class to the constructor for the Dpdk___Installer later. This gives the reader all the info they need to know about what's happening.
I can make a couple of DependencyInstaller instances for source and package manager installs. In our init path I just call self._installer=DpdkGitInstaller(dependencies=dpdk_source_dependencies)
or self._installer=DpdkTarInstaller(dependencies=dpdk_source_dependencies)
or self._installer=DpdkPackageManagerInstaller(dependencies=dpdk_package_manager_dependencies)
Afterwards, in the install function I just call:
self._installer.run_installation()
If you look at the code we're replacing, this is a much more organized way to handle things. If we need to replace this later, I will go back and fix it. For now, this is how I want to handle it.
microsoft/testsuites/dpdk/common.py
Outdated
| # define to handle any special special cases | ||
| self._setup_node() | ||
| # and install everything | ||
| self._install_dependencies() |
There was a problem hiding this comment.
It's not easy to understand, why dependencies to be installed after _run_installation. What depends on those dependencies?
There was a problem hiding this comment.
Fixing this class up, I re-wrote the generic code at one point but didn't go back and fix this class to use the generic _run_installation. Will fix
| if not self.git_ref: | ||
| git = self._node.tools[Git] | ||
| self.git_ref = git.get_tag( | ||
| self._source_path, filter_=r"^v.*" # starts w 'v' |
There was a problem hiding this comment.
It's the only deifference with the DpdkSourceInstall. The name of classes are confusing.
There was a problem hiding this comment.
We don't need to do this git ref filtering for other projects, it's project specific.
It's the only deifference with the DpdkSourceInstall.
Exactly. I only need to overload one or two functions per class, that's expected. There is a lot of shared code between the builds.
mcgov
force-pushed
the
mcgov/test-install
branch
4 times, most recently
from
51917d9
to
5468cae
Compare
The installer path has grown to be unmaintainable. We cover multiple cases: -install from pkg manager -install from source - install from git - install from tar - (implicit) use already installed version for multiple targets: - debian - ubuntu 18.04 special casing - suse - fedora and now need to allow all of these scenarios for both DPDK and rdma-core (and the linux kernel). SO... It's time for a refactor. Commit makes the 'installer' concept object-oriented. Adds a generic class 'Installer' into common.py Adds child class SourceInstall and PackageManagerInstall. Adds DpdkSourceInstall and DpdkPackageManagerInstall classes. Adds DpdkGitInstall and DpdkTarInstall implementations of the generic parent classes.
mcgov
force-pushed
the
mcgov/test-install
branch
from
5468cae
to
cebb660
Compare
The DPDK installer path has grown to be unmaintainable, this PR series cleans it up using some classes instead of just a bunch of if/else trees. The PR looks scary; but, it is just moving code to the object-oriented replacements. This means when we get to
Tool.install()we can just calldpdktestpmd.installer.run_installation()My plan is to re-use the SourceInstaller and PackageManagerInstaller logic for rdma-core, though this series only implents the classes for DPDK.
For the DPDK and rdma-core installation we have to cover multiple cases:
-install from pkg manager
-install from source
And we cover these cases for multiple targets:
and now need to cover all of these scenarios for both DPDK and rdma-core.
Changes:
SourceInstallandPackageManagerInstallto dpdk/common.pyDpdkSourceInstallandDpdkPackageManagerInstallclasses to dpdk/dpdktestpmd.pyDpdkGitInstallandDpdkTarInstallimplementations of the generic DpdkSourceInstall class to dpdk/dpdktestpmd.pyThis required adding some other logic: