Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces the KernelSourcePackager transformer to create .deb packages from kernel source code, complementing the existing KernelSourceInstaller for improved continuous testing workflows. It also adds a RepoWorktree transformer for efficient multi-iteration builds using git worktrees.
Changes:
- New KernelSourcePackager transformer that builds kernel .deb packages with caching support
- New RepoWorktree transformer for managing git worktrees efficiently
- Enhanced kernel_source_installer.py with ccache support and additional build dependencies
- Registration of new transformer in mixin_modules.py
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 21 comments.
| File | Description |
|---|---|
| lisa/transformers/kernel_source_packager.py | Implements KernelSourcePackager transformer with build, cache, and package management functionality, plus RepoWorktree for git worktree operations |
| lisa/transformers/kernel_source_installer.py | Adds use_ccache parameter to _build_code, installs packaging dependencies (libdw-dev, fakeroot, dpkg-dev, debhelper-compat), and improves directory permission logic |
| lisa/mixin_modules.py | Registers the new kernel_source_packager module for transformer discovery |
| make_args = "CC='ccache gcc'" | ||
| node.execute(f"export CCACHE_DIR=" | ||
| f"{str(code_path.parent)}/.ccache" | ||
| ) | ||
|
|
||
| # set timeout to 2 hours | ||
| make.make(arguments="", cwd=code_path, timeout=60 * 60 * 2) |
There was a problem hiding this comment.
The execute method is missing the shell=True parameter. The export command requires shell=True to work properly. Additionally, the command will only set the environment variable for that single execute call - it won't persist for the subsequent make.make() call. Consider using a different approach to set CCACHE_DIR, such as passing it as part of the make arguments.
| make_args = "CC='ccache gcc'" | |
| node.execute(f"export CCACHE_DIR=" | |
| f"{str(code_path.parent)}/.ccache" | |
| ) | |
| # set timeout to 2 hours | |
| make.make(arguments="", cwd=code_path, timeout=60 * 60 * 2) | |
| ccache_dir = f"{str(code_path.parent)}/.ccache" | |
| make_args = f"CC='ccache gcc' CCACHE_DIR={ccache_dir}" | |
| # set timeout to 2 hours | |
| if make_args: | |
| # Run make with ccache-related environment variables applied only | |
| # to this command via the shell. | |
| node.execute( | |
| f"{make_args} make", | |
| cwd=code_path, | |
| shell=True, | |
| timeout=60 * 60 * 2, | |
| ) | |
| else: | |
| make.make(arguments="", cwd=code_path, timeout=60 * 60 * 2) |
| package_paths = None | ||
|
|
||
| if metadata: | ||
| # Remove [any existing entry with the same commit_id |
There was a problem hiding this comment.
Inconsistent bracket placement in the comment. The opening bracket '[' should be removed or the comment should be reformatted for clarity.
| # Remove [any existing entry with the same commit_id | |
| # Remove any existing entry with the same commit_id |
| from lisa.tools import Echo, Git | ||
| from lisa.util import subclasses | ||
|
|
||
| # from lisa.transformer import Transformer |
There was a problem hiding this comment.
Commented-out import statement should be removed. If it's not needed, clean it up; if it is needed, uncomment it. Dead code should not be left in the codebase.
| # from lisa.transformer import Transformer |
| self._log.info(f"Verified kernel source is on branch '{current_branch}'.") | ||
|
|
||
| # 4. Build the kernel (reuse SourceInstaller, but do NOT install) | ||
| kconfig_file = getattr(runbook, "kernel_config_file", None) |
There was a problem hiding this comment.
The code attempts to access 'kernel_config_file' attribute on the runbook object, but the runbook is of type KernelSourcePackagerSchema which doesn't have this attribute. This will raise an AttributeError. The 'kernel_config_file' field exists in SourceInstallerSchema, not in KernelSourcePackagerSchema. You need to either add this field to KernelSourcePackagerSchema or access it through the appropriate schema.
| kconfig_file = getattr(runbook, "kernel_config_file", None) | |
| source_runbook = getattr(source_installer, "runbook", None) | |
| kconfig_file = ( | |
| getattr(source_runbook, "kernel_config_file", None) | |
| if source_runbook is not None | |
| else None | |
| ) |
| remotes = git.remote_list(code_path) | ||
| self._log.debug(f"existing remotes: {remotes}") | ||
| for remote in remotes: | ||
| if runbook.worktree_repo == git.remote_get_url(code_path, remote): | ||
| remote_exists = True | ||
| break | ||
|
|
||
| if not remote_exists: | ||
| remote = runbook.worktree_name | ||
| self._log.info(f"adding remote {remote} for {runbook.worktree_repo}") | ||
| git.remote_add(cwd=code_path, name=remote, url=runbook.worktree_repo) |
There was a problem hiding this comment.
The code calls git.remote_list(), git.remote_get_url(), and git.remote_add() methods that do not exist in the Git tool class. These methods are not implemented in lisa/tools/git.py, which will cause AttributeError exceptions at runtime. You need to either implement these methods in the Git class or use direct git commands via node.execute().
| remotes = git.remote_list(code_path) | |
| self._log.debug(f"existing remotes: {remotes}") | |
| for remote in remotes: | |
| if runbook.worktree_repo == git.remote_get_url(code_path, remote): | |
| remote_exists = True | |
| break | |
| if not remote_exists: | |
| remote = runbook.worktree_name | |
| self._log.info(f"adding remote {remote} for {runbook.worktree_repo}") | |
| git.remote_add(cwd=code_path, name=remote, url=runbook.worktree_repo) | |
| remotes_result = self._node.execute( | |
| "git remote", | |
| cwd=str(code_path), | |
| ) | |
| remotes = [ | |
| r.strip() | |
| for r in remotes_result.stdout.splitlines() | |
| if r.strip() | |
| ] | |
| self._log.debug(f"existing remotes: {remotes}") | |
| for remote_name in remotes: | |
| remote_url_result = self._node.execute( | |
| f"git remote get-url {remote_name}", | |
| cwd=str(code_path), | |
| ) | |
| if runbook.worktree_repo == remote_url_result.stdout.strip(): | |
| remote_exists = True | |
| remote = remote_name | |
| break | |
| if not remote_exists: | |
| remote = runbook.worktree_name | |
| self._log.info(f"adding remote {remote} for {runbook.worktree_repo}") | |
| self._node.execute( | |
| f"git remote add {remote} {runbook.worktree_repo}", | |
| cwd=str(code_path), | |
| ) |
| worktree_path = code_path.parent / runbook.worktree_name | ||
| git.worktree_prune(cwd=code_path) | ||
| if not git.worktree_exists(cwd=code_path, path=str(worktree_path)): | ||
| self._log.info( | ||
| f"creating a new worktree at {worktree_path} " | ||
| f"pointing at {remote}/{runbook.worktree_ref}" | ||
| ) | ||
| git.worktree_add( | ||
| cwd=code_path, | ||
| path=worktree_path, | ||
| remote=remote, | ||
| remote_ref=runbook.worktree_ref, | ||
| new_branch=runbook.worktree_local_branch, | ||
| track=True, | ||
| ) | ||
|
|
||
| latest_commit_id = git.get_latest_commit_id(cwd=worktree_path) | ||
| self._log.info(f"Kernel HEAD is now at : {latest_commit_id}") | ||
| return worktree_path | ||
|
|
||
| # worktree exists | ||
| target_ref = runbook.worktree_ref | ||
| target_path = worktree_path |
There was a problem hiding this comment.
The code calls git.worktree_prune(), git.worktree_exists(), and git.worktree_add() methods that do not exist in the Git tool class. These methods are not implemented in lisa/tools/git.py, which will cause AttributeError exceptions at runtime. You need to either implement these methods in the Git class or use alternative approaches to manage worktrees.
| worktree_path = code_path.parent / runbook.worktree_name | |
| git.worktree_prune(cwd=code_path) | |
| if not git.worktree_exists(cwd=code_path, path=str(worktree_path)): | |
| self._log.info( | |
| f"creating a new worktree at {worktree_path} " | |
| f"pointing at {remote}/{runbook.worktree_ref}" | |
| ) | |
| git.worktree_add( | |
| cwd=code_path, | |
| path=worktree_path, | |
| remote=remote, | |
| remote_ref=runbook.worktree_ref, | |
| new_branch=runbook.worktree_local_branch, | |
| track=True, | |
| ) | |
| latest_commit_id = git.get_latest_commit_id(cwd=worktree_path) | |
| self._log.info(f"Kernel HEAD is now at : {latest_commit_id}") | |
| return worktree_path | |
| # worktree exists | |
| target_ref = runbook.worktree_ref | |
| target_path = worktree_path | |
| worktree_prune = getattr(git, "worktree_prune", None) | |
| worktree_exists = getattr(git, "worktree_exists", None) | |
| worktree_add = getattr(git, "worktree_add", None) | |
| if not ( | |
| callable(worktree_prune) | |
| and callable(worktree_exists) | |
| and callable(worktree_add) | |
| ): | |
| self._log.warning( | |
| "Git worktree operations are not supported by the current " | |
| "Git tool implementation. Proceeding without using a " | |
| "separate worktree." | |
| ) | |
| else: | |
| worktree_path = code_path.parent / runbook.worktree_name | |
| worktree_prune(cwd=code_path) | |
| if not worktree_exists(cwd=code_path, path=str(worktree_path)): | |
| self._log.info( | |
| f"creating a new worktree at {worktree_path} " | |
| f"pointing at {remote}/{runbook.worktree_ref}" | |
| ) | |
| worktree_add( | |
| cwd=code_path, | |
| path=worktree_path, | |
| remote=remote, | |
| remote_ref=runbook.worktree_ref, | |
| new_branch=runbook.worktree_local_branch, | |
| track=True, | |
| ) | |
| latest_commit_id = git.get_latest_commit_id(cwd=worktree_path) | |
| self._log.info(f"Kernel HEAD is now at : {latest_commit_id}") | |
| return worktree_path | |
| # worktree exists | |
| target_ref = runbook.worktree_ref | |
| target_path = worktree_path |
| """ | ||
| node = self._node | ||
| runbook: KernelSourcePackagerSchema = self.runbook | ||
| parent_dir = str(self._code_path.parent) |
There was a problem hiding this comment.
Variable parent_dir is not used.
| parent_dir = str(self._code_path.parent) |
| from datetime import datetime | ||
| from dataclasses import dataclass, field | ||
| from dataclasses_json import dataclass_json | ||
| from pathlib import Path, PurePath |
There was a problem hiding this comment.
Import of 'Path' is not used.
| from pathlib import Path, PurePath | |
| from pathlib import PurePath |
| BaseLocationSchema, | ||
| SourceInstaller, | ||
| SourceInstallerSchema, | ||
| RepoLocation, |
There was a problem hiding this comment.
Import of 'RepoLocation' is not used.
| RepoLocation, |
| import lisa.transformers.dump_variables # noqa: F401 | ||
| import lisa.transformers.file_uploader # noqa: F401 | ||
| import lisa.transformers.kernel_source_installer # noqa: F401 | ||
| import lisa.transformers.kernel_source_packager # noqa: F401 |
There was a problem hiding this comment.
Import of 'lisa' is not used.
…el package Like kenel_source_installer this transformer pulls and kernel repo, builds it and creates a kernel debian package(for now) out of it. This package can be installed using the deb_package_installer transformer. Signed-off-by: Piyush Sachdeva <[email protected]> Signed-off-by: Piyush Sachdeva <[email protected]>
The RepoWorktree transformers gives the user the ability to work with git-worktree when maintaining kernel repo on a node. In case of an existing source, it uses the git-worktree functionality to effectively maintain multiple copies of the code. In case the fields necessary for a worktree are not specified the functionality falls back to `source` installer transformer. Signed-off-by: Piyush Sachdeva <[email protected]> Signed-off-by: Piyush Sachdeva <[email protected]>
Signed-off-by: Piyush Sachdeva <[email protected]> Signed-off-by: Piyush Sachdeva <[email protected]>
Only conditionally create the code folder. Signed-off-by: Piyush Sachdeva <[email protected]> Signed-off-by: Piyush Sachdeva <[email protected]>
psachdeva-ms
force-pushed
the
kern_src_pkger
branch
from
c98d529 to
e9b3841
Compare
| make_args = "CC='ccache gcc'" | ||
| node.execute(f"export CCACHE_DIR=" | ||
| f"{str(code_path.parent)}/.ccache" | ||
| ) |
There was a problem hiding this comment.
have you tested it? probably it needs shell=True
| ) | ||
| else: | ||
| self._log.info("No-cache mode: building and packaging kernel.") | ||
| ret = self._build_and_package(source_installer, commit_id, kernel_version) |
There was a problem hiding this comment.
need_build = True
if runbook.use_cache:
self._log.info("Checking for cached kernel packages...")
if self._check_cache(commit_id, kernel_version):
self._log.info("Cache hit: using cached package.")
ret = self._update_cache(commit_id=commit_id)
need_build = False
else:
self._log.info("Cache miss: building and packaging kernel.")
else:
self._log.info("No-cache mode: building and packaging kernel.")
if need_build:
ret = self._build_and_package(source_installer, commit_id, kernel_version)
| ret = self._build_and_package(source_installer, commit_id, kernel_version) | ||
|
|
||
| if ret is None: | ||
| raise Exception("No images retrieved. kernel_source_packager_error") |
There was a problem hiding this comment.
I don't understand the meaning of the exception message.
| package_paths, | ||
| ) | ||
|
|
||
| def _build_and_package( |
2 participants
This adds a new Transformer: KernelSourcePackager - It compliments the existing KernelSourceInstaller, where instead of building and installing the kernel on the node, it just create a .deb package(for now) which can be used to boot from and test the kernel.
The reason for doing this is for a specific use case - where for continuous testing there can be one builder node which takes care of doing all the builds, meanwhile target nodes can be deployed to use the created .deb packages and testing can happen more efficiently saving time spent freshly cloning and building the kernel.
The KernelSourcePackager also makes use of a new RepoWorktree Transformer, that can be used to build different iterations of the same base repository more efficiently by adding them as worktrees.