CI: Refactor most yaml into python#84
Conversation
mebersol
left a comment
mebersol
left a comment
There was a problem hiding this comment.
[Copilot] Nice refactor — moving CI logic into testable Python with matrix.yml as the single source of truth and a local invoke_ci_locally.py is a real ergonomics win. A few inline comments:
- release_common.py:
load_release_config()is defined twice (duplicate block) — should be deleted. - CreateRelease.yml: the
workflow_dispatchrun-id resolution can emit an emptyrun_idand fail later in download-artifact; suggest a fast-fail guard. - stage_artifacts.py: flattening logs by basename can silently overwrite same-named logs from different subfolders.
- Nits: missing trailing newline in
release.ymlandtest_release.py.
None are blockers; the duplicate function and the run-id guard are the two worth addressing before merge.
mebersol
left a comment
mebersol
left a comment
There was a problem hiding this comment.
[Copilot] Re-review pass (full diff). The earlier four comments still stand (duplicate load_release_config, empty run_id guard, log-flatten overwrite, trailing newlines). One additional diff-level observation added inline: the old workflow installed VC.ASAN and the new per-variant component lists drop it — likely intentional, flagged to confirm.
Otherwise the refactor looks solid: matrix parity with the old Platform-Build.yml (same 6 variants, AARCH64+VS2022 correctly excluded) is preserved and enforced by a unit test; build/release logic is now testable Python with matrix.yml/release.yml as single sources of truth; concurrency group + cancel-in-progress: false correctly serializes releases; and the workflow_run + head_branch == 'main' gating keeps fork-PR builds from triggering the privileged release job. Nice work.
chris-oo
left a comment
chris-oo
left a comment
There was a problem hiding this comment.
seems reasonable to me, but you probably want another set of reviews from someone more familiar with python
|
|
||
|
|
||
| def _vs_install_path() -> Path: | ||
| vswhere = Path(r"C:\Program Files (x86)\Microsoft Visual Studio\Installer\vswhere.exe") |
There was a problem hiding this comment.
This is recreating the functionality provided in edk2-pytool-library pip module
Edk2-pytool-library would download a copy of vswhere (https://github.com/tianocore/edk2-pytool-library/blob/4d0b5009580b1c43eb0ce275eaa2a0bcbcefe286/edk2toollib/windows/locate_tools.py#L43) and use that, but this is relying on vshwere being installed at a specific location on the system, which makes this script very brittle
4 participants
Refactors away most of our YAML to be simple wrappers into python scripts. Allows us to invoke CI steps locally and verify things without much iteration.