Repository navigation
feat(packaging): publish amdrocm-repo via workflow outputs - #7276
lsudarsh-amd wants to merge 8 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. |
|
No concerns for the |
|
Its a big change. need some time to review it. |
|
Hi @lsudarsh-amd, reviewers, Learnings: To me, This PR bundles two independent changes:
Thanks |
Thanks @nunnikri for starting to look at these changes. After your initial comment, I double checked the implementation, and no change should be needed in It's carried because It stays out of the package index too, which is why it sits in I've added three tests to ensure proper coverage here. One drives the real promotion into a local backend, and two cover the S3 listing so a |
Thanks for the review @arvindcheru. As requested, the keyring work is now #7332, which carries the Your call-graph trace was right. Isolating that code also turned up a bug, fixed and covered in #7332. The documented |
| AMDROCM_DEB_KEYRING = "/usr/share/keyrings/amdrocm.gpg" | ||
| AMDROCM_RPM_REPO_BASENAME = "amdrocm.repo" | ||
| # Named -amdrocm, not -rocm, for the same collision reason as the deb | ||
| # keyring. Must match build_repo_package.RPM_GPG_KEY_PATH. |
There was a problem hiding this comment.
Thank you for the updating the comments, Will review in detail.
Observation: native_linux_package_install_test.py hardcodes the GPG key paths as strings instead of importing them from build_repo_package.py, relying on a comment to keep them in sync. If the paths change there, this file won't notice.
Is it possible to import: Import DEB_KEYRING_PATH/RPM_GPG_KEY_PATH from build_repo_package.py instead — inspect_repo_package.py already does this, so it's a consistent, low-risk fix.
There was a problem hiding this comment.
I agree with you on the risk you outlined. Two spellings of one path kept in sync by a comment is the same bug #7332 fixes.
I can't make the fix in this PR though. build_repo_package.py is added by #7269, so the import would fail at collection on this branch. inspect_repo_package.py gets away with it because it lands in #7280, after #7269.
I'd rather make this change in #7280, which anyway depends on #7269 and already imports both constants.
## Motivation `native_linux_package_install_test.py` is an existing install test that CI runs today. It downloads a repository signing key and saves it to disk so apt can check package signatures. Three things are wrong with how it does that, and all of them predate the `amdrocm-repo` work. **The import could fail and still report success.** The key was fetched with a single `shell=True` pipeline. A pipeline reports the exit status of its last command, so `check=True` was only ever checking `tee`. If the download returned something that wasn't a key, `gpg --dearmor` failed, `tee` wrote an empty keyring anyway, and the harness printed `[PASS]` and returned `True`. The run then failed much later on a signature error that gave no hint where it came from. This isn't a corner case. `gpg --dearmor` exits 2 on an empty body or on an HTML error page served with a 200, which is a normal thing for a server to return. **The documented `ROCM_APT_KEYRING_FILE` override didn't work.** Only the `signed-by=` line used it. The code that writes the key built its own path and hardcoded `rocm.gpg`, so setting the variable saved the key in one place and told apt to look in another. `apt update` then failed with "The repository is not signed". **The default path clashes with AMD's install instructions.** Those instructions put the ROCm signing key at `/etc/apt/keyrings/rocm.gpg`, and the harness writes over it with `sudo tee`. This doesn't happen in CI, which runs in throwaway containers, but it does on the VMs and workstations the script is also meant to run on. Split out of #7276 because it changes existing code and doesn't depend on the new `amdrocm-repo` feature. Contributes to #6986. ## Technical Details **The shell pipeline is gone.** The import now runs as three `subprocess.run` calls, one each for `wget`, `gpg --dearmor` and `sudo tee`. Each takes its arguments as a list and sets `check=True`, so a failure stops the import there instead of leaving an empty keyring behind. The URL and the keyring path are no longer interpolated into a shell string, so neither can be used for injection. **The keyring path comes from one constant.** `setup_gpg_key()` takes both the file and its parent directory from `APT_KEYRING_FILE`. That fixes the override, and it lets the default move to `/etc/apt/keyrings/{REPO_NAME}.gpg`, the way `APT_SOURCES_LIST` already works. The rename only works because of that. On its own it would have written to `rocm.gpg` while telling apt to check `rocm-test.gpg`. `APT_KEYRING_DIR` goes too, along with its docstring entry, since nothing reads it any more and nothing in the repo sets it. **Paths are `PurePosixPath`.** These are paths on the target Linux filesystem, handed to `mkdir`, `tee` and `chmod`. `Path` follows the local flavour and renders `\etc\apt\keyrings` on Windows, which fails the `windows-2022` job. **A timeout now returns `False`.** `subprocess.TimeoutExpired` is a `SubprocessError`, so neither the `CalledProcessError` nor the `OSError` handler caught it and a timeout killed the run instead of reporting a failed import. That was already true, but splitting the pipeline takes the timed calls in that block from two to four. Each carries `GPG_KEY_TIMEOUT_SEC` of 60, so fetch, dearmor and write together can take up to 180 seconds. **The file runs in CI, but these lines don't.** Every change sits behind `if self.gpg_key_url:` (`:543`, `:551`, `:763`), which covers both `setup_gpg_key()` call sites, and neither caller passes a key URL. `test_native_linux_packages_install.yml:227` wires `GPG_KEY_URL` from an input (`:17`, `:62`, `:151`) nobody sets, and `multi_arch_build_native_linux_packages.yml:138` runs `--test-type simulate`, which skips repository setup. ## Test Plan - **Unit tests.** Five new cases in `ConfiguredPathsTest`: the default doesn't collide with `/etc/apt/keyrings/rocm.gpg`, it derives from `REPO_NAME`, `APT_KEYRING_DIR` is gone, the write target matches what `signed-by=` pins, and no call uses a shell. Two new cases in `SetupGpgKeyTest` for the bad body and the timeout, plus the existing sequence check tightened to compare two argv tokens instead of one. - **End to end against a signed repo.** Build a real GPG-signed apt repository in a container, serve it over localhost, and drive the genuine `setup_deb_repository()`, which runs a real `apt update`. Run the same scenarios against `main` to confirm each one fails there. - **Full suite.** `pytest build_tools/` in the CI container against a freshly measured `origin/main` baseline, comparing failure names rather than counts. - **Negative controls.** For every new assertion, break the code it covers, confirm the test fails, then restore. - **Lint.** `pre-commit` over the changed files. ## Test Result | Check | Result | |---|---| | Unit tests | Pass. 125 in the module, including the 7 new cases. | | End to end against a signed repo | Pass, 12/12. The same suite scores 6/12 on `main`. | | Full suite | **No new failures.** 9 failed / 1761 passed against 9 failed / 1754 on `main`. Identical failure names. | | Negative controls | Pass. Six, each failing only the test it targets. | | Lint | Pass. | The six checks `main` fails are exactly the ones this fixes. It overwrites `/etc/apt/keyrings/rocm.gpg`, ignores the `ROCM_APT_KEYRING_FILE` override, and reports success on a body that isn't a key while leaving an empty keyring behind. The override case is the sharpest. There, `apt update` exits 100 with "The repository is not signed", so `main` doesn't just misplace the key, it breaks apt in a plain container. The nine pre-existing failures are in `compute_rocm_package_version_test.py`, `jax_install_scripts_test.py` and `stage_reuse_decision_test.py`, none of which this touches. **Not covered.** `PurePosixPath` can't be exercised on Linux. `Unit Tests :: windows-2022` covers it and passed on #7276 with this same code. The timeout is covered by unit test rather than end to end, since `GPG_KEY_TIMEOUT_SEC` is 60 and isn't overridable. ## Submission Checklist - [x] Look over the contributing guidelines at https://github.com/ROCm/TheRock/blob/main/CONTRIBUTING.md.
Derives the publish destination from WorkflowOutputRoot and writes through StorageBackend rather than taking a bucket and prefix and driving boto3 directly. The new output type sits beside the content repository rather than inside it: one package is built per OS profile and several share a filename, so indexing them together would collide. Also adds the URL helpers the workflow invokes and an install-harness mode that configures the repository by installing the built package.
b4b4152 to
2d60e4e
Compare
The workflow needs three values to build amdrocm-repo: which stream a build line configures, that stream's packages base, and its signing key URL. Add a single table carrying all three, replacing the base-URL-only map that pointed at the retired packages-multi-arch hosts. The mapping is deliberately not s3_buckets.get_release_stream(). That answers which product bucket a build publishes into, and its answers do not transfer here: it maps prerelease to rc, which serves no content on any distro, and it raises on release, because stable is promoted manually and has no artifacts bucket -- yet stable is the stream users install from. release_type stays the workflow input because it also selects bucket credentials; the stream is derived from it. The key URL is emitted whole rather than as a root for the builder to append to, so where the key lives stays a fact recorded here. Also fix three promotion tests that pinned v4/packages/ as the destination. That prefix moved to v5/rocm/core/packages/, which turned them red without the code under test changing. They now discover the destination from what the promotion actually wrote and assert only the tail, since what they exist to prove is that the copy is recursive enough to carry repo/<profile>/ -- not where the release lands.
|
Please review. |
arvindcheru
left a comment
arvindcheru
left a comment
There was a problem hiding this comment.
LGTM,
Please resolve the Merge conflicts, and wait for CI tests
Approve #7276: Based on GPG split is done (#7332), promotion is covered by tests, scope is publish helper + workflow outputs + URL helpers + opt-in install mode — LGTM to merge after #7269 (and before #7280.)
Since there are multiple PR dependency Kindly wait for other review input also before proceeding to merge.
The install harness looked for a repo called "amdrocm", but the package registers amdrocm-<stream>, so rpm refreshes and deb checks failed on every stream. It now reads the repo id, file names and key paths from build_repo_package instead of hardcoding them. Also: - map the prerelease line to the rc stream - replace the three repo-param subcommands with one get-repo-params - add jinja2 to the harness test requirements - tidy comments and docstrings
…ublish # Conflicts: # docs/development/workflow_outputs.md
@arvindcheru Thanks for the earlier approval. I've made some changes since then, could you please take another look? @nunnikri Could you please review this too? Changes since last review:
The remaining red checks on CI aren't from this PR. |
…o shared/amdrocm-repo-publish
@arvindcheru Thanks for checking. No workflow sets About the error you hit. Those variables are only read under pytest, which is how CI runs the harness. Running the script directly needs With package mode on (stable, rc and nightly on all four distros), repo setup, ROCm install and the basic checks pass. The uninstall test after that fails, because it counts the Here are the steps to test on your end if you'd like: docker run --rm -it -v "$PWD:/src:ro" -w /src ghcr.io/rocm/no_rocm_image_ubuntu24_04:latest bashThen inside the container, run these steps bash build_tools/packaging/linux/setup_repo_build_deps.sh --os-profile ubuntu2404
eval "$(bash build_tools/packaging/linux/setup_python_cmd.sh --os-profile ubuntu2404 --install-runtime --output-format env)"
$PYTHON_CMD -m venv /tmp/venv && . /tmp/venv/bin/activate
pip install -r build_tools/packaging/linux/tests/requirements.txt
# Build the rc repo package
python build_tools/packaging/linux/build_repo_package.py --os-profile ubuntu2404 --stream rc --rocm-version 10.0.0 \
--repo-base-url https://rc.repo.amd.com/rocm/core/packages \
--gpg-key-url https://rc.repo.amd.com/rocm/gpg/packages.gpg --dest-dir /tmp/pkg
# Package mode, run directly...
python build_tools/packaging/linux/native_linux_package_install_test.py \
--os-profile ubuntu2404 --release-type prerelease --repo-package-dir /tmp/pkg --repo-config-only
# ...or the way CI runs it
OS_PROFILE=ubuntu2404 RELEASE_TYPE=prerelease REPO_PACKAGE_DIR=/tmp/pkg REPO_CONFIG_ONLY=1 \
python -m pytest build_tools/packaging/linux/native_linux_package_install_test.py -sTo install ROCm as well (a few GB), swap Here are the logs from my test run: |
This branch has not been deployed
5 participants
Motivation
The
amdrocm-repobootstrap package needs a published location before anyone can fetch and install it. This pull request adds the script that publishes it, the build parameters the packaging workflow reads, and an install-harness mode that installs the built package. The workflow job that runs them comes in the next pull request.It also resolves two review comments on #6901, which this split supersedes. The publish helper imported
boto3inline and did not useWorkflowOutputRoot. It now resolves its destination throughWorkflowOutputRootand writes throughStorageBackend, which takesboto3out of this script entirely rather than hoisting the import to the top of the file. The dependency still exists behindS3StorageBackend, but the helper no longer touches it.This is the third of four pull requests:
packaging/linux/teststotestpaths, so the tests the later ones add actually run in CI.This builds on #7269's branch, because the build parameters and the harness read the package's names and stream rules from
build_repo_package. The diff shows only this PR's files.Contributes to #6986.
Technical Details
What this adds
A new output type.
WorkflowOutputRoot.native_linux_repo_package(pkg_type, os_profile)returns{prefix}/packages/{pkg_type}/repo/{os_profile}/amdrocm-repo.{pkg_type}. It follows the process indocs/development/workflow_outputs.md, which is why this adds the method, its tests and the documentation together.The package sits beside the package index rather than in it. Clients fetch the file directly by URL, so it does not belong in the index. The published name is fixed at
amdrocm-repo.<ext>to keep that URL stable, so the per-profile directory is what stops one profile's package overwriting another's. It also keeps a client from resolving the package built for a different distro, since every profile uses the same package name.Promotion already carries it.
publish_rocm_to_release_buckets.pycopies each run'spackages/<type>/tree to the release bucket, and the package sits inside that tree. New tests run the real promotion into a local backend and check that every profile's package arrives with the repository beside it. TwoS3StorageBackendtests pin the recursive listing the copy depends on.The publish helper derives its own destination.
publish_repo_package.pytakes--run-idand resolves the bucket and prefix throughWorkflowOutputRoot, which is the single source of truth for CI path layout. The #6901 version took--bucket,--prefixand--endpoint-url. Two consequences:releaseline raises. It isn't an artifact release type, so the bucket lookup rejects it. Release uploads happen outside CI, and everytherock-release-*bucket hasiam_role=None. The publishing job does not run forrelease, so the raise is a backstop.GITHUB_REPOSITORY,RELEASE_TYPEand the workflow event payload in its job environment.Nothing on
maincalls this script yet. The workflow that does lands in the next pull request.Build parameters.
get_url_repo_params.py get-repo-paramswrites everything the build matrix needs in one$GITHUB_OUTPUTcall:os_profilesandimages(the matrix), thenstream,repo_base_url,gpg_key_urlandrepo_sub_folder.The build line and the stream are different vocabularies:
releasemaps tostable,prereleasetorcandnightlytonightly. This script holds only that mapping and where each stream is served. Everything else about a stream comes frombuild_repo_package. So the key URL is emitted only for a signed stream, and the dated build folder only for a per-build stream. A line with no public stream (ci,dev) gets those four values empty and still gets the matrix. rc.repo.amd.com serves all four profiles, signed with the same key as stable. The fingerprint matches the builder's pin.build_repo_packageis imported inside the subcommand rather than at module level. The script's other subcommands run on the runner's system Python intest_native_linux_packages_install.yml, in a job that installs no Python packages.Install harness.
native_linux_package_install_test.py --repo-package-dir <dir>configures the repository by installing the built package instead of writing repository files directly. It then refreshes that repository, checks the repo file and signing key are in place, and installs ROCm from it.--repo-config-onlystops before the ROCm install. Nothing in CI uses this mode yet.The repo id, repo file name and key paths come from
build_repo_package(repo_id(),is_signed(),DEB_KEYRING_PATH,RPM_GPG_KEY_PATH), so only the distro's repo directories stay in the harness. Argument parsing rejects--repo-package-dirunless--release-typenames a line with a public stream.packaging/linux/tests/requirements.txtgainsjinja2>=3.0.0, the pin used elsewhere, because this mode importsbuild_repo_package. The harness's other modes still load without it.Smaller changes
Input validation.
--run-idmust be numeric and--os-profilemust be a single safe path segment. Both become path segments of the object key, and a value like../..would escape a local staging directory.get-repo-paramschecks the run id too, because it goes into$GITHUB_OUTPUT, wheregha_set_outputwrites multi-line values with a heredoc whose delimiter it does not check. Both inputs come from CI, so this is defence in depth.One fixture.
RunTestsTestTypeTest._base_argsgainsrepo_package_dirandrepo_config_onlywith their argparse defaults, both of whichrun_tests()reads. Without them those tests fail, since #7004 made that directory collectable.Test Plan
get-repo-paramsand the harness package mode. The publish and promotion tests drive a realLocalStorageBackendand assert the bytes on disk instead of mocking an S3 client. Two tests guard against the harness drifting from the package. One renders the package's own install manifest for every profile and stream and checks it contains every file the harness looks for. The other checks every stream the builder knows is reachable from exactly one build line.packaging/linux/tests/requirements.txt. Also replay the stage job feat(ci): wire the amdrocm-repo package into native package CI #7280 will run (get-repo-params, build, publish) on each distro.pre-commit.Test Result
amdrocmand failed 9/9 on ubuntu2404, rhel10 and sles16: dnfUnknown repo: 'amdrocm', zypperRepository 'amdrocm' not found, and on deb noamdrocm.sources. With the newjinja2line removed, package mode fails withNo module named 'jinja2'. Stage-job replay: 8/8, including the rc key fetch and a check of the live index. Both re-run after #7269's signing-key change.prereleasemapping, a module-level builder import in either script, a build folder chosen by stream name, and a key check that ignores signing each fail their tests.Submission Checklist