Rock: hip-tests installation path to remain same for linux/windows by agunashe · Pull Request #2187 · ROCm/rocm-systems

agunashe · 2025-12-04T23:47:34Z

On theRock - installation path remains same linux/windows as share/hip/catch_tests
On internal win build - hip-tests will be installed to catch_tests depending on a flag that is passed internally from amd_build.py script

There is no change to linux packaging. This PR mainly aligns windows installation path with linux

Motivation

hip-tests installation paths were different before since there was no requirement.
This PR aligns linux and windows to have same path

Technical Details

hip-tests installation paths were different before since there was no requirement.
This PR aligns linux and windows to have same path since Rock is a common setup.

This PR was tested with ROCK PR to enable hip-tests execution
ROCm/TheRock#2001
ROCK builds passing with this PR+ Rock PR
https://github.com/ROCm/TheRock/actions/runs/20078078455/job/57610400448?pr=2001

JIRA ID

SWDEV-566268

Test Plan

hip-tests built and passes rocm-ci and windows builds

Test Result

NA

Submission Checklist

Look over the contributing guidelines at https://github.com/ROCm/ROCm/blob/develop/CONTRIBUTING.md#pull-requests.

## Motivation This change enables hip-tests to be built on TheRock. This is the first step in getting hip-tests building/running with TheRock. ## Technical Details Enable hip-tests to be built with TheRock, when user enables testing via flag `THEROCK_BUILD_TESTING`. [hip-tests](https://github.com/ROCm/rocm-systems/tree/develop/projects/hip-tests/catch) are collection of tests that are used to test the hip-runtime. A corresponding PR is also required in hip-tests to make it compatible with TheRock, which are being done [here](ROCm/rocm-systems#1624). Subsequent PRs are required to run the built hip-tests which will follow after this PR. ROCm/rocm-systems#2187 - PR will fix the paths to be consistent across linux and windows. Below issues will be fixed with it https://github.com/ROCm/TheRock/pull/2373/files#r2582804642 #2373 (comment) #2001 (comment) #2001 (comment) ## Test Plan The build should complete end to end without failures, next set of changes will enable the running of tests. ## Test Result NA ## Submission Checklist - [x] Look over the contributing guidelines at https://github.com/ROCm/ROCm/blob/develop/CONTRIBUTING.md#pull-requests.

On theRock - installation path remains same linux/windows share/hip/catch_tests On internal win build - hip-tests will be installed to catch_tests flag is passed internally which controls the path.

removed the else part

Copilot

Pull request overview

This PR unifies the installation paths for hip-tests between Linux and Windows platforms, setting both to use share/hip/catch_tests on ROCK, while maintaining backward compatibility for internal Windows builds through the HIPTESTS_INTERNAL_BUILD flag.

Key Changes:

Consolidated CPACK_PKG_DIRS path construction to use a single unified path format for both platforms
Set INSTALL_DIR to ${CMAKE_INSTALL_DATADIR}/hip for both Linux and Windows (with exception for internal builds)
Moved platform-agnostic packaging configuration outside of platform-specific blocks

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.

File	Description
projects/hip-tests/catch/packaging/catch_package.cmake.in	Unified CPACK_PKG_DIRS path construction to use consistent format across Linux and Windows
projects/hip-tests/catch/packaging/CMakeLists.txt	Consolidated INSTALL_DIR logic and moved packaging configurations to be platform-agnostic

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

raramakr

Many of the CMake statements are not required on Windows. A conditional check for NOT WIN32 might be better.

agunashe · 2025-12-11T23:59:46Z

Many of the CMake statements are not required on Windows. A conditional check for NOT WIN32 might be better.

agreed, Will clean up and add os checks were its required in the next PR.
This PR is required to enable the testing on ROCK.

agunashe · 2025-12-12T00:10:59Z

Many of the CMake statements are not required on Windows. A conditional check for NOT WIN32 might be better.

## Motivation This change enables hip-tests to be built on TheRock. This is the first step in getting hip-tests building/running with TheRock. ## Technical Details Enable hip-tests to be built with TheRock, when user enables testing via flag `THEROCK_BUILD_TESTING`. [hip-tests](https://github.com/ROCm/rocm-systems/tree/develop/projects/hip-tests/catch) are collection of tests that are used to test the hip-runtime. A corresponding PR is also required in hip-tests to make it compatible with TheRock, which are being done [here](ROCm/rocm-systems#1624). Subsequent PRs are required to run the built hip-tests which will follow after this PR. ROCm/rocm-systems#2187 - PR will fix the paths to be consistent across linux and windows. Below issues will be fixed with it https://github.com/ROCm/TheRock/pull/2373/files#r2582804642 #2373 (comment) #2001 (comment) #2001 (comment) ## Test Plan The build should complete end to end without failures, next set of changes will enable the running of tests. ## Test Result NA ## Submission Checklist - [x] Look over the contributing guidelines at https://github.com/ROCm/ROCm/blob/develop/CONTRIBUTING.md#pull-requests.

linux/windows (#2187) * Rock: hip-tests installation path remains same for linux and windows On theRock - installation path remains same linux/windows share/hip/catch_tests On internal win build - hip-tests will be installed to catch_tests flag is passed internally which controls the path. [rocm-systems] ROCm/rocm-systems#2187 (commit 0bb5638)

## Motivation This change enables hip-tests to be built on TheRock. This is the first step in getting hip-tests building/running with TheRock. ## Technical Details Enable hip-tests to be built with TheRock, when user enables testing via flag `THEROCK_BUILD_TESTING`. [hip-tests](https://github.com/ROCm/rocm-systems/tree/develop/projects/hip-tests/catch) are collection of tests that are used to test the hip-runtime. A corresponding PR is also required in hip-tests to make it compatible with TheRock, which are being done [here](ROCm/rocm-systems#1624). Subsequent PRs are required to run the built hip-tests which will follow after this PR. ROCm/rocm-systems#2187 - PR will fix the paths to be consistent across linux and windows. Below issues will be fixed with it https://github.com/ROCm/TheRock/pull/2373/files#r2582804642 #2373 (comment) #2001 (comment) #2001 (comment) ## Test Plan The build should complete end to end without failures, next set of changes will enable the running of tests. ## Test Result NA ## Submission Checklist - [x] Look over the contributing guidelines at https://github.com/ROCm/ROCm/blob/develop/CONTRIBUTING.md#pull-requests.

## Motivation This change enables hip-tests to be built on TheRock. This is the first step in getting hip-tests building/running with TheRock. ## Technical Details Enable hip-tests to be built with TheRock, when user enables testing via flag `THEROCK_BUILD_TESTING`. [hip-tests](https://github.com/ROCm/rocm-systems/tree/develop/projects/hip-tests/catch) are collection of tests that are used to test the hip-runtime. A corresponding PR is also required in hip-tests to make it compatible with TheRock, which are being done [here](ROCm/rocm-systems#1624). Subsequent PRs are required to run the built hip-tests which will follow after this PR. ROCm/rocm-systems#2187 - PR will fix the paths to be consistent across linux and windows. Below issues will be fixed with it https://github.com/ROCm/TheRock/pull/2373/files#r2582804642 ROCm#2373 (comment) ROCm#2001 (comment) ROCm#2001 (comment) ## Test Plan The build should complete end to end without failures, next set of changes will enable the running of tests. ## Test Result NA ## Submission Checklist - [x] Look over the contributing guidelines at https://github.com/ROCm/ROCm/blob/develop/CONTRIBUTING.md#pull-requests.

## Motivation Enable hip-tests execution on TheRock linux and windows. Aligns installation path of hip-tests on linux and Windows enables sharding of hip-tests ## Details Enable hip-tests execution on TheRock linux and windows. **This PR fixes -** #1354 **Hip-tests source that gets built/executed as part of this PR** https://github.com/ROCm/rocm-systems/tree/develop/projects/hip-tests/catch **Related PRs to enable build/execution of tests -** #2373 - enables building of tests ROCm/rocm-systems#1624 - prepares rocm-systems to be built with catchlib from upstream , compiler/hip detection, packaging change ROCm/rocm-systems#2187 - aligns installation path for linux and windows **few other script changes to have compatible builds on older build frameworks.** https://github.com/AMD-ROCm-Internal/build-infra/pull/316 https://github.com/AMD-Radeon-Driver/compute-utils/pull/169 https://github.com/AMD-Radeon-Driver/compute-utils/pull/171 hip-tests executed and verified successfully ## Submission Checklist - [x] Look over the contributing guidelines at https://github.com/ROCm/ROCm/blob/develop/CONTRIBUTING.md#pull-requests. --------- Co-authored-by: Geo Min <geomin12@amd.com>

Add host-only microtests for the MR-registration-type fix in ncclRmaProxyCtxAllocGraph (src/rma/rma_proxy.cc). When GDRCopy is disabled -- the default on ROCm, since NCCL_GDRCOPY_ENABLE defaults to 0 -- allocMemCPUAccessible hands back host-NUMA pinned memory rather than device memory. The pre-fix code unconditionally registered cpuAccessSignalsDev as NCCL_PTR_CUDA, so the net plugin rejected the MR. Upstream fixed this in NCCL 2.30.7 (NVIDIA/nccl PR #2187) by deriving the type from the GDR handle: non-NULL means GDRCopy-mapped device memory (NCCL_PTR_CUDA), NULL means host memory (NCCL_PTR_HOST). The line was previously uncovered, and because the host path is the default path on AMD, a regression would silently break every RMA-proxy run without GDRCopy. The tests pivot the real allocMemCPUAccessible on the extern global ncclGdrCopy rather than mocking the allocator, so the GDR handle the production ternary branches on is the one production actually produced. A FakeRma vtable records every (addr, size, type, mrFlags) registration; in the strict-plugin test it rejects NCCL_PTR_CUDA for any pointer that the hipHostMalloc seam really returned, so the failure mode mirrors the defect instead of restating the assertion. Five cases: both GDR arms of the cpuAccessSignals registration, a strict plugin that refuses a mistyped MR, flushBufDev staying NCCL_PTR_CUDA on both arms, and NCCL_NET_MR_FLAG_FORCE_SO preserved on both arms. This lands as its own binary, rccl-UnitTestsMicroRmaProxy, rather than joining rccl-UnitTestsMicro: that target already resolves ncclRmaProxyConnectOnce, ncclRmaProxyRegister and ncclRmaProxyDeregister from fakes/dev_runtime_micro_fakes.cc, and rma_proxy.cc defines all three. Also upgrades the fail-loud hipMemset stub in fakes/hip_fakes.cc to a g_hipMemset seam whose default really memsets, so a buffer production zeroes reads back as zero. No existing host test referenced hipMemset.

#12756) ## Motivation `ncclRmaProxyCtxAllocGraph` registers `cpuAccessSignalsDev` with the net plugin. That buffer comes from `allocMemCPUAccessible`, which returns **host-NUMA pinned memory** whenever GDRCopy is off — and `NCCL_GDRCOPY_ENABLE` defaults to `0`, so on ROCm the host path is the *default* path, not an edge case. Before the fix the registration type was hardcoded `NCCL_PTR_CUDA`, so the net plugin rejected the MR. Upstream fixed this in NCCL 2.30.7 (NVIDIA/nccl PR #2187) by deriving the type from the GDR handle. RCCL `develop` is at NCCL 2.31.2 and already carries the fix, so **no production code changes here** — the line was simply uncovered, and a regression would silently break every RMA-proxy run without GDRCopy. RCCL has already had to patch two neighbouring symptoms of the same "these signals may be host memory" confusion (`rcclParamRmaProxyUseDMABUF` and the `props_tmp` DMA-BUF strip in the same function), which is why pinning this behaviour down is worth doing. This PR adds the regression coverage. ## Technical Details New host-only microtest `test/host/rma-proxy-test.cc` plus its own binary, `rccl-UnitTestsMicroRmaProxy`. Host-only: it links gtest + fmt, **not** `librccl.so`, `libamdhip64.so` or `libhsa-runtime64.so`, and needs no GPU. It gets a dedicated binary rather than joining `rccl-UnitTestsMicro` because that target already resolves `ncclRmaProxyConnectOnce`, `ncclRmaProxyRegister` and `ncclRmaProxyDeregister` from `fakes/dev_runtime_micro_fakes.cc`, and `rma_proxy.cc` defines all three — linking the unit under test there is a duplicate symbol. `MICROTEST_README.md` sanctions one binary per unit, and `rccl-UnitTestsMicroInit` / `...Enqueue` are the precedent. `ncclRmaProxyCtxAllocGraph` is file-`static`, so the test TU `#include`s the hipified `.cc` via a build-time `RMA_PROXY_CC_PATH` macro — the documented micro idiom, same as the sibling `rma-proxy-progress-test.cc`. Two seams make this a test of the fix rather than a restatement of it: 1. `allocMemCPUAccessible` is a `static` template in `gdrwrap.h`, so it cannot be stubbed at link time. That is a feature here: the test runs the **real** helper and pivots it on the real discriminator, the extern global `ncclGdrCopy`. The GDR handle the production ternary branches on is the one production actually produced, not a mocked premise. 2. In `StrictPluginRejectsWrongMemoryType_AllocGraphStillSucceeds`, `FakeRma` rejects `NCCL_PTR_CUDA` for any pointer present in a set populated by the `hipHostMalloc` seam. "Is this host memory?" is answered by real provenance, so the test models the defect rather than asserting the conclusion. Also in this PR: - `fakes/hip_fakes.{h,cc}`: `hipMemset` was a fail-loud stub with no hook, which would have aborted this function's `cudaMemset(flushBufDev, ...)` under `CUDACHECK`. Upgraded to a `g_hipMemset` seam whose default really memsets. No existing host test referenced `hipMemset`. - `run_host_tests.sh`: added the new binary to the run list. Without this the CI host-test job would build it and never run it, and still go green. - `test/test_categories_micro_rma_proxy.yaml`: category registration, structurally identical to `test_categories_micro_enqueue.yaml`. ## Issue Tracking JIRA ID: AICOMRCCL-1851 Upstream: NVIDIA/nccl PR #2187 (NCCL 2.30.7) ## Test Plan All compiles and all binary runs were done on compute nodes, never a login node. 1. Build and run the new binary both standalone (`test/host` project) and in-tree. 2. **Prove it catches the defect.** Revert the fix in the *hipified* `rma_proxy.cc` — the exact TU the binary `#include`s — back to the pre-PR-#2187 unconditional `NCCL_PTR_CUDA`, rebuild, rerun, then restore and re-verify green. 3. Branch coverage via `llvm-cov --show-branches=count`: both arms of the `cpuAccessSignalsGdrHandle != NULL` ternary must be non-zero. Line hits alone are not evidence — the bug lives entirely in which arm is taken. 4. Order-independence under `--gtest_shuffle` (CI shuffles), and a JUnit report actually written. ## Test Result **Green, in-tree and standalone:** 5/5, `rc=0`, and `ldd` confirms no `librccl`/`libamdhip`/`libhsa`. **Pre-fix revert — `rc=1`, 2 failed / 3 passed:** ``` [ FAILED ] RmaProxyAllocGraphTest.GdrCopyOff_RegistersCpuAccessSignalsAsHost rma-proxy-test.cc:319: Expected equality of these values: 0x1 (NCCL_PTR_HOST) Which is: 1 reg->type Which is: 2 host-NUMA cpuAccessSignals registered as device memory (NVIDIA/nccl PR #2187) [ FAILED ] RmaProxyAllocGraphTest.StrictPluginRejectsWrongMemoryType_AllocGraphStillSucceeds rma-proxy-test.cc:353: Expected equality of these values: ncclSuccess Which is: 0 AllocGraph() Which is: 3 (ncclInternalError) net plugin rejected the cpuAccessSignals registration due to wrong memory type (NVIDIA/nccl PR #2187) [ OK ] RmaProxyAllocGraphTest.GdrCopyOn_RegistersCpuAccessSignalsAsCuda [ OK ] RmaProxyAllocGraphTest.FlushBufferStaysCudaOnBothGdrArms [ OK ] RmaProxyAllocGraphTest.ForceStrongOrderingPreservedOnBothGdrArms ``` Restoring the line returns 5/5. The second failure is the defect stated verbatim: a strict plugin refusing a mistyped MR takes `ncclRmaProxyCtxAllocGraph` to `ncclInternalError`. The three *passes* are as important as the failures — they show the suite is not simply "everything red when the line changes". The GDRCopy-on arm, the flush buffer and the `FORCE_SO` flag are unaffected, so an over-correction to always `NCCL_PTR_HOST` would be caught too. **Branch coverage:** ``` 229| 7| int cpuAccessSignalsType = (rmaProxyCtx->cpuAccessSignalsGdrHandle != NULL) ? NCCL_PTR_CUDA : NCCL_PTR_HOST; | Branch (229:30): [True: 3, False: 4] ``` **Shuffle:** 5/5 and a valid `<testsuites>` report at seeds 0, 1, 7, 42, 1851. ### Scope limit, stated plainly This is host-microtest evidence. The real-world claim — that a net plugin stops rejecting the MR — is modelled by a `FakeRma` vtable, not exercised against a live IB plugin on multi-node hardware. For a one-line memory-type fix on a path that is already the ROCm default, I judged a host microtest the proportionate gate; a reviewer who wants an MPI/IB functional test should say so. ## Submission Checklist - [x] Look over the contributing guidelines at https://github.com/ROCm/rocm-systems/blob/develop/CONTRIBUTING.md.

agunashe requested review from a team as code owners December 4, 2025 23:47

github-actions Bot added the project: hip-tests label Dec 4, 2025

agunashe changed the title ~~Rock: hip-tests installation path remains same for linux and windows~~ Rock: hip-tests installation path to remain same for linux/windows Dec 5, 2025

systems-assistant Bot added the organization: ROCm label Dec 5, 2025

This was referenced Dec 5, 2025

Add hip-tests to be built with TheRock ROCm/TheRock#2373

Merged

Enable hip-tests on TheRock ROCm/TheRock#2001

Merged

agunashe added 2 commits December 8, 2025 16:36

Rock: hip-tests installation path remains same for linux and windows

f059ff2

On theRock - installation path remains same linux/windows share/hip/catch_tests On internal win build - hip-tests will be installed to catch_tests flag is passed internally which controls the path.

typo in packaging config file.

d39d74c

removed the else part

agunashe force-pushed the users/agunashe/hip-tests_installation branch from 0b0cbed to d39d74c Compare December 9, 2025 00:36

agunashe requested review from chrispaquot, cjatin and mangupta December 10, 2025 02:56

mangupta requested a review from raramakr December 10, 2025 03:23

agunashe requested a review from Copilot December 10, 2025 17:39

Copilot started reviewing on behalf of agunashe December 10, 2025 17:40

View session

Copilot AI reviewed Dec 10, 2025

View reviewed changes

raramakr requested a review from nunnikri December 10, 2025 23:14

raramakr reviewed Dec 11, 2025

View reviewed changes

Comment thread projects/hip-tests/catch/packaging/CMakeLists.txt Resolved

raramakr approved these changes Dec 11, 2025

View reviewed changes

agunashe closed this Dec 12, 2025

agunashe reopened this Dec 12, 2025

mangupta approved these changes Dec 12, 2025

View reviewed changes

agunashe merged commit 0bb5638 into develop Dec 12, 2025
39 of 41 checks passed

agunashe deleted the users/agunashe/hip-tests_installation branch December 12, 2025 16:45

prasanna-amd mentioned this pull request Oct 4, 2026

[AICOMRCCL-1851] [RCCL] test: cover rma_proxy cpuAccessSignals MR type #12756

Merged

1 task done

Navigation Menu

Repository navigation

Rock: hip-tests installation path to remain same for linux/windows - #2187

Rock: hip-tests installation path to remain same for linux/windows#2187
agunashe merged 2 commits into
developfrom
users/agunashe/hip-tests_installation

agunashe commented Dec 4, 2025 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

raramakr left a comment

Uh oh!

agunashe commented Dec 11, 2025

Uh oh!

agunashe commented Dec 12, 2025

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

agunashe commented Dec 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Technical Details

JIRA ID

Test Plan

Test Result

Submission Checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

raramakr left a comment

Choose a reason for hiding this comment

Uh oh!

agunashe commented Dec 11, 2025

Uh oh!

agunashe commented Dec 12, 2025

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

agunashe commented Dec 4, 2025 •

edited

Loading