Repository navigation
Eliminate repo and switch to submodules. - #128
Merged
Merged
Conversation
ScottTodd
reviewed
Mar 3, 2025
Member
There was a problem hiding this comment.
I can update the docs in https://github.com/nod-ai/TheRock/blob/main/docs/development/windows_support.md#clone-and-fetch-sources once I sync past this and try it out.
Comment on lines
130
to
+138
| # TODO(#36): Enable once rocprofiler-systems can be checked out on Windows | ||
| # error: invalid path 'src/counter_analysis_toolkit/scripts/sample_data/L2_RQSTS:ALL_DEMAND_REFERENCES.data.reads.stat' | ||
| # Upstream issues: | ||
| # https://github.com/ROCm/rocprofiler-systems/issues/105 | ||
| # https://github.com/icl-utk-edu/papi/issues/321 | ||
| if not is_windows(): | ||
| populate_submodules_if_exists(args, args.dir / "rocprofiler-systems") | ||
| populate_submodules_if_exists( | ||
| args, THEROCK_DIR / "profiler" / "rocprofiler-systems" | ||
| ) |
Member
There was a problem hiding this comment.
Glad this is still working after switching from repo to submodules. papi merged a patch to fix the checkout on Windows but I haven't received a response yet on ROCm/rocprofiler-systems#105 to update the papi version pin in rocprofiler-systems
Collaborator
Author
There was a problem hiding this comment.
Well, I also temporarily removed that from the projects since we aren't building it yet.
ScottTodd
added a commit
that referenced
this pull request
Mar 3, 2025
Follow-up to #128. * Drop some references to 'repo' and the 'sources/' directory * Change Windows instructions from requiring symlinks to just recommending them * Drop repo-specific configuration from Windows workflow
1 task done
1 task done
1 task done
lsudarsh-amd
added a commit
that referenced
this pull request
Oct 22, 2026
…#7269) ## Motivation Installing ROCm today needs a manual repository setup. The user has to fetch and dearmor the AMD signing key, write a sources or repo file by hand, and look up the correct URL for their distribution and stream. This change adds `amd-repo-setup`, a repo configuration package that replaces that multi-step manual setup. Once it is installed, ROCm can be installed and updated with the native package manager. This is the second of five pull requests: - PR 0 (#7004) added `packaging/linux/tests` to `testpaths`, so the tests the later ones add actually run in CI. - PR 1 (this one) adds the builder, its templates, its unit tests and the user documentation. - PR 2 (#7276) adds the build parameters (`get-repo-setup-params`), the upload to the run's artifacts bucket and the install-harness package mode. - PR 3 (#7280) wires the CI workflow: build, inspect and upload, off by default. - PR 4 (#9352) adds the `WorkflowOutputRoot` location for the package and publishes it to `v5/amd-repo-setup/` during release promotion. Contributes to #6986. ## Technical Details `build_repo_setup_package.py` renders the package from the Jinja templates under `template/repo/`. It builds one package per OS profile (`ubuntu2404`, `rhel8`, `rhel10`, `sles16`) and stream (`stable`, `rc`, `nightly`). **Repository URLs are derived per OS profile.** The URL is `--repo-base-url` plus the profile, for example `https://stable.repo.amd.com/rocm/core/packages/ubuntu2404/`; rpm adds `x86_64/`. Nightly also adds the folder for one build (`--repo-sub-folder`). **The package is `amd-repo-setup`**, the same name as the `amd-repo-setup/` folder it will be published to (PR 4). The files that carry the name follow it: the builder, its test and the rpm spec template (`amd-repo-setup.spec.j2`, picked by package name). The repository the package configures keeps its `amdrocm` names, the same ones the documented manual setup uses: repo id `amdrocm-stable`, key `amdrocm.gpg`. **Package names include the distro and a build number**: `amd-repo-setup_10.0.0-1~ubuntu2404_all.deb`, `amd-repo-setup-10.0.0-1.stable.el8.noarch.rpm`. The build number is `--version-suffix` (default `1`). The docs have a table of examples. **The package version is `--rocm-version`, as given, on every stream.** CI passes the version TheRock computes for the release type, the same one the ROCm packages in that repository carry (`10.1.0`, `10.1.0~pre3`, `10.2.0~20261013`). So the package sorts the way they do, below stable at the same ROCm version, and no two streams share a package name. rpm also carries the stream in the release field (`1.stable.el8`). **The signing key is fetched at build time.** For signed streams it is retrieved over https and size-bounded. A missing key URL fails at once; only timeouts and rate limiting are retried. The file must contain only the pinned AMD key, and only that key is embedded. It is never stored in the source tree. For offline builds, `--gpg-key-file` embeds a local key as provided. The nightly repository is unsigned upstream, so nightly packages ship no key and disable signature checking. **The package conflicts with `amdgpu-install`**, which configures a different ROCm repository and raises it above the default priority. Its repository files use different names from ours, so nothing else stops both being installed, and its repository then wins resolution silently. On apt the conflict is settled by removing `amdgpu-install`, which leaves its configuration files behind. The deb `postinst` detects that state and tells the user to run `apt purge`. `dnf` and `zypper` refuse the pair outright and remove their `%config` files on erase, so no equivalent is needed there. **The rpm `%post`** prints a refresh message, like the deb `postinst`. **Nothing builds the repo packages yet.** The script writes finished packages to `--dest-dir` and is run once per profile and stream. The workflow that calls it is in #7280; until then it is invoked by hand, as the documentation describes. Each format needs its own tooling, `dpkg-buildpackage` for deb and `rpmbuild` for rpm, so the CI job builds each profile in a container that has it. **`build_tools/.gitignore` is deleted.** Its only entry was an unanchored `repo` pattern, which would hide the entire new `template/repo/` directory from git. The tool it guarded was removed in #128. ### Also in this PR: the `rc` stream This also builds the `rc` stream, which goes beyond the review feedback. RFC0012 requires `stable` and `rc` at v1, while `nightly`, `weekly` and `dev` come at v2. The tool used to build `stable` and `nightly`, so it shipped a v2 stream but was missing a v1 one. `rc` was left out earlier because its repositories were empty. They aren't anymore: `rc.repo.amd.com` now serves more packages than `stable`, is signed on all four OS profiles, and uses the same signing key. A few details: - **The installed file is `amdrocm-stablerc`, not `amdrocm-rc`.** `stablerc` is the name RFC0012 gives the rc tier, so the file uses it while the subdomain stays `rc`. That way a later split into per-tier packages won't need to rename a file on users' machines. - **Versioning.** Like every stream, rc uses the version it's given. CI passes the prerelease version of the ROCm packages (`10.1.0~pre3` on deb, `10.1.0~rc3` on rpm), which sorts below the GA. - **`Suites:` is `stable` for every stream.** That's what the servers publish (`dists/stable/`, `Suite: stable`); `dists/stablerc/` returns 404. CI wiring for `rc` is in #7280, not here. ### Notes for reviewers - The `list-profiles` subcommand has no caller yet; `get-repo-setup-params` in #7276 calls the `list_profiles()` function directly. Unit tests cover both. - The test file sits in `build_tools/tests/` rather than `packaging/linux/tests/`, where the other tests for that module's directory live. `build_tools/tests/` is already in `testpaths`, so these tests run in CI from the first push, including on the Windows leg. - The root-ownership directives in `rules.j2` and the spec match the other ROCm packages. Building as a non-root user already produces `root:root` without them. - No apt pin or `priority=` is shipped; the package sits at each manager's default. Matching `amdgpu-install`'s pin would raise this whole CDN above the distribution's own repositories. The conflict declaration, and the purge the `postinst` points at, address the overlap instead. Happy to add a pin if you would prefer one. - `weekly` and `dev` aren't built: `weekly` has no packages yet, and `dev` isn't for end users. - Switching streams on Debian/Ubuntu isn't handled in the package yet: the old `.sources` stays behind, so the docs say to purge first. A follow-up will handle it in the package before it's published. ## Test Plan All testing ran in containers. - Built the package for all four OS profiles and all three streams with the versions CI computes, and inspected each payload with `dpkg-deb -c` or `rpm -qp`. - Checked with real `dpkg` and `rpm` that each stream's package sorts the same way as the ROCm packages in its repository. - Installed each package on `ubuntu2404`, `rhel8`, `rhel10` and `sles16`, refreshed metadata against the live stable, rc and nightly repositories, and resolved the `amdrocm` package. - Tested the `amdgpu-install` conflict in both directions on both formats, using the real `amdgpu-install` 31.40 and 31.50 packages, then repeated it on each format with the declaration removed as a control. - Installed the deb against all four `amdgpu-install` states: not installed, installed, removed but not purged, and purged. - Tested the key check with real gpg: the real key alone, and with a throwaway extra key. Built with a key URL that returns 404 and with one that serves something other than a key. - Tested switching streams in every direction on apt, dnf and zypper. - Ran the documented install, verification and build commands. - Ran the unit tests with the module's path handling bound to the Windows flavour. - Ran `pre-commit`, `shellcheck`, and the full unit test suite. ## Test Result | Check | Result | | --- | --- | | New unit tests | 183 pass. Tests that need `dpkg`, `rpm`, `gpg` or the de_DE locale skip where those are missing. | | Full suite | No new failures. The delta is exactly the new test file. | | Package payload | Correct on all four profiles. Every file lands at its documented path, owned by `root:root`. | | Version ordering | Each stream's package sorts the same way as the ROCm packages in its repository. At the same ROCm version, stable is above rc and nightly on both formats. | | Signing key | The real key is accepted. The real key plus another key, in either order, is rejected. A key URL that returns 404 fails on the first attempt, and a response that isn't a key fails with gpg's own message. | | Live repository | Metadata refreshes for all three streams on all four profiles, and `amdrocm` resolves. A real signed ROCm package installs on each profile using the shipped key. | | Documented commands | All ran as written. `zypper` needs `--allow-unsigned-rpm` for a locally built file, which the documentation states. | | Conflict enforcement | `dpkg`, `dnf` and `zypper` refuse the pair in both directions. `apt` resolves it by removing the installed package. | | Conflict controls | With the declaration removed, both packages install together and two then own competing repository files. | | `postinst` | Warns only in the removed-but-not-purged state, including after `apt` removes `amdgpu-install` to resolve the conflict, and never fails an install. | | Stream switching | `dnf` switches directly. `apt` needs a purge first and `zypper` needs `--oldpackage`, as documented. | | Windows path handling | Binding the Windows flavour makes the keyring test fail, so a regression there would be caught. | | Template naming | The spec template is picked by package name. A template that doesn't match the package name fails 11 unit tests and the rpm build. | | `pre-commit`, `shellcheck` | Clean. | ## Submission Checklist - [x] Look over the contributing guidelines at https://github.com/ROCm/TheRock/blob/main/CONTRIBUTING.md. --------- Co-authored-by: Nirmal Unnikrishnan <71024015+nunnikri@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I was down a dark path trying to get synchronized changes to pinned repo manifests with the super-project (primarily because repo kind of requires a secondary repository of manifests vs being able to use single in-tree manifest files). The workflow is kind of the opposite of what we want anyway (i.e. floating heads by default, very hard to pin in a hermetic way).
This patch:
update --recursiveto eliminate the ancillary fetches.We can make this nicer and eliminate fetch_sources entirely (or replace it with some nicer helpers for doing partial submodule inits) once we retire our local patches.
I will just manually
update --remotedaily until we get automation to do it.