clp-package: Package building platform's /etc/os-release and determine execution_container from the file.
#322
Conversation
…n_container` from the file.
| parsed[var] = val | ||
|
|
||
| self.execution_container = "ghcr.io/y-scope/clp/" | ||
| if "ubuntu" == parsed["ID"]: |
There was a problem hiding this comment.
can we store parsed["ID"] in a local variable?
There was a problem hiding this comment.
When I drafted the code, I thought about extracting those into variables parsed_id, parsed_version_id / parsed_version_codename, and even better we break the for-loop if all info we need has been parsed. However, that might be less clean and less extensible if we (ever) plan to support CentOS / RHEL in the future. That's because VERSION_ID is common across all Linux distributions, but VERSION_CODENAME is specific to Ubuntu / Debian distributions.
There was a problem hiding this comment.
Should we add Debian into the if condition?
There was a problem hiding this comment.
I thought about adding Debian and other RHEL distributions here, but that does not seem meaningful until we add (building support and most importantly,) execution container support in tools/docker-images. For now, I think raising an NotImplementedError here is a good notification to users.
| with open(self.os_release_file_path) as os_release_file: | ||
| parsed = {} | ||
| for line in os_release_file: | ||
| var, val = line.strip().split("=") |
There was a problem hiding this comment.
technically we can add a guard to ensure split does return two items, but since the os release file should have a relatively fixed format, this code should be fine
There was a problem hiding this comment.
Right, I was thinking about adding try ... except blocks to handle such errors / FileNotFoundError, but since users are not expected to modify this <clp-package>/etc/os-release file, such handlings could be redundant. In any case, users will get notified by the Exception stack.
| @@ -355,6 +359,27 @@ def validate_logs_dir(self): | |||
| except ValueError as ex: | |||
| raise ValueError(f"logs_directory is invalid: {ex}") | |||
|
|
|||
| def load_execution_container_name(self): | |||
| if self.execution_container is not None: | |||
There was a problem hiding this comment.
Just for sanity check, have you tested and made sure that the container specified in the clp-config will be properly picked up?
There was a problem hiding this comment.
Yes. I have updated the "Validation" section in the PR description to include the related instructions.
Users can configure a string field |
| parsed[var] = val | ||
|
|
||
| self.execution_container = "ghcr.io/y-scope/clp/" | ||
| if "ubuntu" == parsed["ID"]: |
There was a problem hiding this comment.
Should we add Debian into the if condition?
…ncy `package` already has the file on the sources list; Reorder `os-release` in tasks sources.
There was a problem hiding this comment.
How about:
clp-package: Include build-platform's /etc/os-release and determine execution_container from it. (#322)
References
Internally, it was found that when the CLP Package is built with container clp-core-dependencies-x86-ubuntu-jammy:main, running the package starting script (entry point
<clp-package>/sbin/start-clp.shleads to missing Python dependencies failures. Further investigation found that the failures indicates Python version discrepancy in the built package virtual environment and execution container. Previously, users could mitigate such discrepancy in the building environment versus the execution container by specifying a matchingexecution_containerinclp-config.yml. However, the configuration field is not publicly documented and encouraged to be used. Instead, the CLP Package should include information about the building platform and determine which Docker image to generate theexecution_container.This PR depends on #321 which adds a GitHub workflow to build and push Ubuntu Jammy execution container to the GitHub Docker Registry.
Description
/etc/os-release.execution_containername from the<clp-package>/etc/os-releasefile./etc/os-releaseto Taskfile building platform dependent tasks to trigger rebuild when the building container changes.Validation performed
Sanity test with Ubuntu Focal Built Package
taskunder project root to build the package.<clp-package>/sbin/start-clp.sh.docker psand observed all CLP component dockers created by IMAGEghcr.io/y-scope/clp/clp-execution-x86-ubuntu-focal:main.Sanity test with Custom
exexcution_containertaskunder project root to build the package.<clp-package>/etc/clp-config.ymlto include a string fieldexecution_containerwith valueclp-execution-x86-ubuntu-focal:dev. e.g.,<clp-package>/sbin/start-clp.sh.docker psand observed all CLP component dockers created by IMAGEclp-execution-x86-ubuntu-focal:dev.Test with Ubuntu Jammy Built Package
taskunder project root to build the package.<clp-package>/sbin/start-clp.sh.docker psand observe all CLP component dockers created by IMAGEghcr.io/y-scope/clp/clp-execution-x86-ubuntu-jammy:main.