Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DPDK: install path fixes for meson and Ubuntu 24.04 #3598

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
+131 −26
Open

DPDK: install path fixes for meson and Ubuntu 24.04 #3598

Changes from 1 commit
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
f9d179d
Meson: try both 'meson' and 'python3-meson'
mcgov Jan 13, 2025
08bf094
DPDK: Add needs_reboot option to installer
mcgov Jan 13, 2025
d34f4be
Suse: implement _uninstall_packages
mcgov Jan 14, 2025
b77c65a
Suse: i guess just 0
mcgov Jan 15, 2025
f6be104
Meson: restructure and comment installation options
mcgov Jan 16, 2025
97ba111
package_exists: add minimum_version
mcgov Jan 16, 2025
124c351
rdma-core: check for existence before removal
mcgov Jan 16, 2025
370fc4e
Dpdk: move pyelftools to single installation point
mcgov Jan 16, 2025
b3a37ad
Minimum version add debug log if no match
mcgov Jan 21, 2025
3cfa460
OS SUSE uninstall: use assert_that
mcgov Jan 21, 2025
186221b
OS packages, fix printing of List[str] of packages
mcgov Jan 21, 2025
6bc582c
make package_exists version check simpler
mcgov Jan 27, 2025
adfa8cd
DPDK: prefer newer pyelftools package
mcgov Jan 27, 2025
452df1c
Use helper function instead of instantiating the class directly
mcgov Jan 28, 2025
74d8808
check_exists( ..., minimum_version) considered harmful
mcgov Jan 28, 2025
459acb3
DPDK: linux-modules-extra-azure reboot
mcgov Jan 29, 2025
5530bc8
Meson: rework install flow, plus more and better comments.
mcgov Feb 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
package_exists: add minimum_version
  • Loading branch information
@mcgov
mcgov committed Feb 5, 2025
commit 97ba111b638d311affa5845a7b897f99a2d0da97
15 changes: 13 additions & 2 deletions lisa/operating_system.py
View file
Original file line number Diff line number Diff line change
@@ -404,13 +404,24 @@ def uninstall_packages(
package_names = self._get_package_list(packages)
self._uninstall_packages(package_names, signed, timeout, extra_args)

def package_exists(self, package: Union[str, Tool, Type[Tool]]) -> bool:
def package_exists(
self,
package: Union[str, Tool, Type[Tool]],
minimum_version: Optional[VersionInfo] = None,
) -> bool:
"""
Query if a package/tool is installed on the node.
Return Value - bool
"""
package_name = self.__resolve_package_name(package)
return self._package_exists(package_name)
exists = self._package_exists(package_name)
if exists and minimum_version:
return (
self.get_package_information(package_name=package_name)
>= minimum_version
)
else:
return exists

def is_package_in_repo(self, package: Union[str, Tool, Type[Tool]]) -> bool:
"""
10 changes: 3 additions & 7 deletions lisa/tools/meson.py
View file
Original file line number Diff line number Diff line change
@@ -15,7 +15,7 @@


class Meson(Tool):
_minimum_version = "0.52.0"
_minimum_version = VersionInfo(major=0, minor=52, patch=0)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use lisa.util.parse_version.


@property
def command(self) -> str:
@@ -45,11 +45,7 @@ def _install(self) -> bool:
"python3-meson",
"meson",
]:
if (
posix_os.package_exists(pkg)
and posix_os.get_package_information(pkg, use_cached=False)
>= self._minimum_version
):
if posix_os.package_exists(pkg, minimum_version=self._minimum_version):
package_installed = pkg
break
elif posix_os.is_package_in_repo(pkg):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

elif to if is the same, but it means the two if blocks are actually independent.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you be more specific with your request from me?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
elif posix_os.is_package_in_repo(pkg):
if posix_os.is_package_in_repo(pkg):

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. So making just that change will introduce an unintended side effect of updating the package before we check the version of the currently installed package. Let me see if I can make the decisions more clear in the code. I was intentionally not installing an updated version from the package manager if one was already installed because it was convenient for my meson use cases... But really it would be better to write for the general case; even though it will be a little uglier and use another round trip or two.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It wasn't so bad to rework! Testing the more commented, reworked version now

@@ -62,7 +58,7 @@ def _install(self) -> bool:
if package_available:
posix_os.install_packages(package_available)
# verify version is correct if it's installed from pkg manager
if posix_os.get_package_information(pkg) < self._minimum_version:
if not posix_os.package_exists(pkg, minimum_version=self._minimum_version):
posix_os.uninstall_packages(pkg)
package_available = ""