Repository navigation
feat(ci): wire the amdrocm-repo package into native package CI - #7280
lsudarsh-amd wants to merge 6 commits into
Conversation
✅ All Checks Passed — Ready for Review
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
🎉 All checks passed! This PR is ready for review. |
45b1e5a to
f7750a8
Compare
|
Please review. |
ScottTodd
left a comment
ScottTodd
left a comment
There was a problem hiding this comment.
Please review.
Please mark as ready for review if you want a code review: https://github.com/ROCm/TheRock/blob/main/CONTRIBUTING.md#requesting-a-code-review
This is the fourth of four pull requests:
It stays a draft until #7269 and #7276 merge. Until then unit-tests, Build DEB Packages and Build RPM Packages fail,
You can use stacked pull requests for this: https://docs.github.com/en/pull-requests/how-tos/stacked-pull-requests
| - name: Derive amdrocm-repo parameters | ||
| id: derive | ||
| run: | | ||
| profiles="$(python ./build_tools/packaging/linux/build_repo_package.py list-profiles \ | ||
| --pkg-type '${{ inputs.native_package_type }}')" | ||
| echo "os_profiles=${profiles}" >> "$GITHUB_OUTPUT" | ||
| python ./build_tools/packaging/linux/get_url_repo_params.py get-container-image-map \ | ||
| --os-profiles "${profiles}" | ||
| python ./build_tools/packaging/linux/get_url_repo_params.py get-public-repo-base-url \ | ||
| --release-type '${{ inputs.release_type }}' | ||
| python ./build_tools/packaging/linux/get_url_repo_params.py get-nightly-sub-folder \ | ||
| --release-type '${{ inputs.release_type }}' \ | ||
| --run-id '${{ inputs.artifact_run_id != '' && inputs.artifact_run_id || github.run_id }}' |
There was a problem hiding this comment.
Why so many calls to get_url_repo_params.py? It looks like a single script call could handle all of this, including writing to GITHUB_OUTPUT (see gha_set_output in https://github.com/ROCm/TheRock/blob/main/build_tools/github_actions/github_actions_api.py)
There was a problem hiding this comment.
Yeah, that's a fair point. I've updated the code to use one get-repo-params call to set all six outputs with gha_set_output. The subcommand itself is in #7276, since that PR owns get_url_repo_params.py.
| # Build the amdrocm-repo package for every OS profile of this package type | ||
| # and check that its payload is the expected files, at the expected paths, | ||
| # owned by root. This container can build both package types but has | ||
| # neither dnf nor zypper, so the packages are inspected rather than | ||
| # installed; per-distro install coverage lives in | ||
| # test_native_linux_packages_install.yml. | ||
| # | ||
| # Restricted to the pull-request line. It is a gate there, and a gate has | ||
| # to be able to fail the job -- which on a release line would also stop the | ||
| # content packages being uploaded. The released lines build the real | ||
| # package in stage_repo_package instead, which cannot block them. | ||
| # | ||
| # The repository URL is a placeholder and is never contacted: the build | ||
| # needs no key it has to fetch and no repository it has to reach. | ||
| - name: Build and inspect amdrocm-repo packages | ||
| if: inputs.enable_repo_package && inputs.release_type == 'ci' | ||
| run: | | ||
| python ./build_tools/packaging/linux/inspect_repo_package.py \ | ||
| --pkg-type "${{ inputs.native_package_type }}" \ | ||
| --rocm-version "${{ inputs.rocm_version }}" \ | ||
| --repo-base-url https://example.com/rocm/core/packages \ | ||
| --repo-sub-folder 20000101-inspect |
There was a problem hiding this comment.
CI and release builds should run the same code whenever possible. See the pinned issue in the repository: #3177.
(also please be careful with AI authored code commenting on "gates" - the terminology used by github is "required check" and multi-arch CI is not currently a required check in this repository, https://github.com/ROCm/TheRock/blob/main/TESTING.md#therock-feature-area-packaging is what this code should follow)
There was a problem hiding this comment.
Good catch, thanks. I'd previously limited this step to ci because I thought it was a required check that shouldn't block releases. It now runs on every line and I've removed the "gate" wording.
| # Build the amdrocm-repo package for each OS profile of this package type on the | ||
| # public release lines and stage it as a workflow artifact. Best-effort: a build | ||
| # failure here must never block the release, so the job cannot fail the workflow. | ||
| # Publishing to a public download location is handled separately. | ||
| stage_repo_package: | ||
| name: Stage amdrocm-repo (${{ matrix.os_profile }}) | ||
| needs: [setup_repo_params] | ||
| if: inputs.enable_repo_package && needs.setup_repo_params.outputs.repo_base_url != '' | ||
| continue-on-error: true |
There was a problem hiding this comment.
Build failures should absolutely block releases. What are you trying to do here?
There was a problem hiding this comment.
When I first implemented this, I thought we wouldn't want a repo package build failure to hold up a release, since it technically sits outside the main ROCm stack. That said, I do agree with you. It ships as part of a finished product, so a failure here should block the release. Code has been updated accordingly.
| - name: Upload amdrocm-repo package | ||
| uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 | ||
| with: | ||
| name: amdrocm-repo-${{ inputs.native_package_type }}-${{ matrix.os_profile }} | ||
| path: repo-package-out/*.${{ inputs.native_package_type }} | ||
| if-no-files-found: error |
There was a problem hiding this comment.
No using actions/upload-artifact for core behavior. Use S3:
There was a problem hiding this comment.
This is fixed now. The artifact upload, download and the separate publish job are gone. The package is now built and published to S3 in the same job.
| - name: Publish amdrocm-repo package | ||
| id: publish | ||
| run: | | ||
| # Bucket and per-run prefix are derived by WorkflowOutputRoot, the | ||
| # same way the content upload resolves them, so the file lands beside | ||
| # the content packages. ARTIFACT_RUN_ID matches what that upload used. | ||
| # Resolution reads RELEASE_TYPE, GITHUB_REPOSITORY and the event | ||
| # payload, all present in this job. | ||
| # | ||
| # One artifact holds one package. Match explicitly rather than globbing | ||
| # into a variable: several matches would be joined by newlines and | ||
| # passed on as a single filename. | ||
| mapfile -t pkgs < <(find repo-package-out -maxdepth 1 -type f \ | ||
| -name "*.${{ inputs.native_package_type }}") | ||
| if [ "${#pkgs[@]}" -ne 1 ]; then | ||
| echo "::error::expected exactly one .${{ inputs.native_package_type }} in repo-package-out, found ${#pkgs[@]}" | ||
| exit 1 | ||
| fi | ||
| pkg="${pkgs[0]}" | ||
| python build_tools/packaging/linux/publish_repo_package.py \ | ||
| --file "$pkg" \ | ||
| --run-id "${ARTIFACT_RUN_ID}" \ | ||
| --os-profile "${{ matrix.os_profile }}" \ | ||
| --pkg-type "${{ inputs.native_package_type }}" |
There was a problem hiding this comment.
There's some scary inline bash here and comments that are better suited inside script files. Please follow the style guide: https://github.com/ROCm/TheRock/blob/main/docs/development/style_guides/github_actions_style_guide.md#prefer-python-scripts-over-inline-bash
There was a problem hiding this comment.
Cleaned this up. The shell that found the built package is now inspect_repo_package.py locate, and I've trimmed the workflow comments, with the detail now in the scripts' docstrings.
Add three jobs to multi_arch_build_native_linux_packages.yml: setup_repo_params resolves the profile matrix, images and repository URLs; stage_repo_package builds the package per profile on the released lines; publish_repo_package uploads it beside the native content packages. The latter two are continue-on-error, so a repo-package failure cannot block a release. Add a build-and-inspect step to build_native_packages, on the ci release line only. It builds every profile unsigned and signed and checks the payload: expected files at expected paths, owned by root, with the signing key only at its current path. The build is offline against a placeholder URL and never contacts a published repository; the container has no dnf or zypper, so packages are inspected rather than installed. Gate all of it behind enable_repo_package, which defaults to true so existing callers need no change.
Pass the resolved stream, packages base and signing-key URL through to the builder in place of the release line, matching the retargeted CLI. Drop prerelease from the allowlist. Its stream is rc.repo.amd.com, which resolves but serves no content on any distro, so the job would produce a package that cannot refresh. The job now skips rather than staging a broken artifact; restoring it is one entry in the stream table plus this list. The inspector follows the same rename, and takes the stream when deriving the repo file path now that the filename carries it. Add timeout-minutes to the three checkout steps this workflow introduces, as workflow_step_timeouts_test.py requires of new steps. The pre-existing checkout is left alone as tracked debt.
f7750a8 to
4b5e805
Compare
Build and publish the package in one job straight to S3, without upload-artifact, and let failures fail the run. Parameters come from a single get-repo-params call, and the inspect step runs on every line for every stream. Also: - enable the prerelease line (rc stream); skip ASAN builds for now - add inspect_repo_package.py locate in place of inline shell - configure artifacts credentials only after the build; give the setup job read-only permissions - move expressions out of run: bodies; stop persisting credentials
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| - name: Checking out repository | ||
| timeout-minutes: 15 | ||
| uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 | ||
| with: | ||
| persist-credentials: false | ||
| repository: ${{ inputs.repository || github.repository }} | ||
| ref: ${{ inputs.ref || '' }} | ||
|
|
||
| - name: Set up Python |
…o shared/amdrocm-repo-ci
@ScottTodd thanks for the review and the pointers. I've stacked the PRs so each diff only shows its own changes (this one sits on #7276, which sits on #7269), and with that the unit tests and package builds pass here too. I've marked this ready for review. I've pushed fixes for all five of your comments. To quickly sum up:
Also, zizmor flags this workflow's existing |
ScottTodd
left a comment
ScottTodd
left a comment
There was a problem hiding this comment.
GitHub Actions workflow code looks reasonable enough now. Will defer the rest of the review to native linux packaging codeowners.
| container: | ||
| image: ${{ fromJSON(needs.setup_repo_params.outputs.images || '{}')[matrix.os_profile] }} |
There was a problem hiding this comment.
Please suppress the zizmor unpinned-images audit finding here: https://github.com/ROCm/TheRock/actions/runs/36768360954/job/110068432039?pr=7280#step:6:95
error[unpinned-images]: unpinned image references
--> .scan-target/.github/workflows/multi_arch_build_native_linux_packages.yml:299:7
|
299 | image: ${{ fromJSON(needs.setup_repo_params.outputs.images || '{}')[matrix.os_profile] }}
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ container image may be unpinned
|
= note: audit confidence → Low
= help: audit documentation → https://docs.zizmor.sh/audits/#unpinned-images
See #8684 for example:
container:
- image: ${{ needs.prepare_install_context.outputs.container_image }}
+ # get_url_repo_params.py manages image references for each OS profile.
+ # zizmor cannot inspect the dynamically selected image.
+ image: ${{ needs.prepare_install_context.outputs.container_image }} # zizmor: ignore[unpinned-images]There was a problem hiding this comment.
Done, I've suppressed it the same way as #8684. While in this part of the code, I noticed that #8637 moved id-token: write from the workflow level to the jobs that need it, so I also gave this job its own contents: read / id-token: write. Otherwise it would lose the id-token permission it needs for AWS now that the stack includes main.
…o shared/amdrocm-repo-ci
This branch has not been deployed
4 participants
Motivation
Setting up a ROCm package repository currently takes a user several manual steps. The
amdrocm-repopackage reduces that to a single install: it ships the repository configuration file and, on the signed release lines, the signing key.This pull request wires the package into the existing native package pipeline, so that it is built and checked on CI runs, built for real on the released lines, and uploaded alongside the native content packages.
This is the fourth of four pull requests:
packaging/linux/teststotestpaths, so PR 2's tests under that directory run in CI.It stays a draft until #7269 and #7276 merge. Until then
unit-tests,Build DEB PackagesandBuild RPM Packagesfail, because the new unit test and the new CI step both callbuild_repo_package.py, which #7269 adds.therock-pr-botwaits on those checks, so it fails too.Closes #6986.
Technical Details
Everything is in
.github/workflows/multi_arch_build_native_linux_packages.yml, apart from one new script and its unit tests.Jobs
setup_repo_paramsstage_repo_packagecontinue-on-errorpublish_repo_package<run_id>-linux/packages/<format>/repo/<os_profile>/, beside the content packages but outside their indexcontinue-on-errorThe release line has no artifacts bucket, so publishing is skipped there. The other two are
continue-on-error, so this package cannot fail a release build.Build and inspect step
A new step in
build_native_packagesbuilds each OS profile twice, unsigned and signed, and asserts the payload: expected files, at expected paths, owned by root, with the signing key only at its current path. It runs offline against a placeholder URL. The logic is inbuild_tools/packaging/linux/inspect_repo_package.py.Three things the diff does not make obvious:
ciline only.build_native_packagesis on the release path, so a gate that can fail the job would otherwise let this package stop a release. The released lines build for real instage_repo_package, which cannot block.test_native_linux_packages_install.yml, which is blocked on its GPU runner and a requiredpackage_install_url.--gpg-key-file, so no key is fetched.enable_repo_package, aworkflow_callboolean defaulting totrue, gates the three jobs and the step. Existing callers are unchanged.Reads of a published repository
stage_repo_packagekeeps--verify-repo-urland fetches the repository index on the prerelease and release lines. It catches a package that installs but cannot refresh, and those runs publish to the line they read. Nightly is skipped, because its dated folder is published by the same run.No pull request reaches a published repository.
cihas no entry in_PUBLIC_REPO_BASE_URLS, sorepo_base_urlis empty andstage_repo_packageskips. The earliertest_repo_packagejob, which refreshed against live prerelease on every pull request, is deleted.Note for reviewers
That job was the only CI caller of PR 2's (#7276)
--repo-package-dirand--repo-config-only. They keep their unit tests and gain a CI caller in the follow-up above.Test Plan
The branch cannot test itself yet, because
build_repo_package.pybelongs to PR 1 (#7269). Verification used a local scratch tree with PR 1 and PR 2 merged in, entirely in throwaway containers.dpkg-deb -candrpm -qpoutput as fixturesbuild_toolssuite as CI runs it, diffing failure names with and without this pull request, not counts--network noneubuntu:24.04,redhat/ubi8,ubi10andbci-base:16.0actionlint,pre-commit, and the unit tests that walk the workflow directoryTest Result
Everything passes. The rows below match the table above.
scan_tools31 passed,test_tools49 passedactionlint,pre-commitand the 23 workflow tests passedThe manual install confirmed three things payload inspection cannot. The deb keyring is dearmored binary at mode 0644, which apt requires when a source pins
Signed-By. The rpm key is ASCII armored andrpm --importaccepts it. Removing the package cleans up every file it installed.Four things cannot be verified outside GitHub Actions and are worth watching on the first CI run:
setup_repo_paramsproduces clean skips in the jobs reading its outputs. Those reads are guarded with the pattern already used elsewhere in this repository, butactionlintcannot check the behaviour either way.stage_repo_packageandpublish_repo_packagerunning for real, including the AWS credentials action and the upload.windows-2022leg, which was simulated locally rather than run.Submission Checklist