[HIPIFY][Infra] Enable dynamic drive allocation to `Windows` build by kirthana14m · Pull Request #2383 · ROCm/HIPIFY

kirthana14m · 2026-02-11T12:38:59Z

Following changes are accumulated in this PR

Dynamic check of available drive on windows machine.
Test cluster name is reverted to linux-mi325-1gpu-ossci-rocm
Base ROCm CI is updated to de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2
aomp smoke tests are added (similar to llvm-project updated)

emankov

Please see my review of #2384.

…aging

emankov

Thanks for addressing some of my comments in the already closed #2384; however, there are still some issues to address in this PR.

[IMPORTANT]
I'd still suggest that you revise the coupled PRs approach for amd-staging and amd-mainline for the following reasons:

error-prone
common source base vs code duplication
difficulties in reviewing them both altogether, keeping in mind different target branches

@searlmc1, @kzhuravl, jfyi.

emankov · 2026-03-11T14:06:47Z

.github/workflows/build_portable_linux_artifacts.yml

        run: |
          ./build_tools/health_status.py

-      - name: Test build_tools


In both the Linux and Windows build workflows, the Test build_tools was removed.

Unless these tests were moved to a separate dedicated workflow not included in this patch, you are losing CI coverage for them.

Yes, tests now live in a dedicated workflow and will be re-enabled once the testing phase is complete in a new patch.

These tests were temporarily removed to accommodate infrastructure changes.

emankov · 2026-03-11T14:10:23Z

.github/workflows/build_windows_artifacts.yml

+          # clear cache before build and after download
+          cd "${{ steps.subst.outputs.drive }}/"
+          ccache -z
+          cmake -B "B:\build" -GNinja -S "${{ steps.subst.outputs.drive }}/" --preset windows-release -DTHEROCK_AMDGPU_FAMILIES=gfx1151 "-DCMAKE_C_COMPILER=C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.44.35207/bin/Hostx64/x64/cl.exe" "-DCMAKE_CXX_COMPILER=C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.44.35207/bin/Hostx64/x64/cl.exe" "-DCMAKE_LINKER=C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.44.35207/bin/Hostx64/x64/link.exe" -DTHEROCK_BACKGROUND_BUILD_JOBS=4


I already saw and reviewed that in 2384: #2384 (comment).
Those comments were left unaddressed:

You should always use "" for paths instead of '', as in Windows CMD, single quotes are not string delimiters.

Targetting a very specific MSVS version, like 14.44.35207, is a bad solution. You should use vcvars64.bat beforehand.

Could you explain why and where the drive letter B has appeared?

Paths: Already using "" for paths on Line 217; happy to fix any remaining spots you flag.

MSVS : We’re using a dedicated Windows runner image that’s built and maintained by ROCKCI DevOps team. That image has a fixed, supported MSVC install (hence the 14.44.35207 path). Relying on that image is a deliberate choice for consistency and supportability in our environment. We’re not invoking a loose MSVC install on a generic host; the compiler path reflects what’s actually on the image. If we move to a different image or setup (e.g. using vcvars64.bat on a generic runner), we’ll revisit this and document it.

Drive letter B: B:\ is the standard build drive on our Windows runner images. For example, BUILD_DIR is set to B:\build in this workflow at line 71 so all builds use that path. It’s defined by the runner image/infrastructure, not by this repo, and keeps paths consistent across runs and easier to support.

As briefly described in the 2384 comment (Point 3)

hence the 14.44.35207 path

FMPOV, it is even worse than magic numbers. All Environment-related variables and constants should be set in one place: the higher, the better. Otherwise, with any environment changes, cloning, whatever else, you'll need to fix those multiple paths, drive letters, and consts in multiple places.

emankov · 2026-03-11T14:12:26Z

.github/workflows/test_aomp_smoke.yml

+    runs-on: ${{ inputs.test_runs_on }}
+    # Running docker with cap-add and -v /lib/modiles, by recommendation of Github: https://rocm.docs.amd.com/projects/amdsmi/en/amd-staging/how-to/setup-docker-container.html
+    container:
+      image: ${{ inputs.platform == 'linux' && 'ghcr.io/rocm/no_rocm_image_ubuntu24_04@sha256:4150afe4759d14822f0e3f8930e1124f26e11f68b5c7b91ec9a02b20b1ebbb98' || null }}


Windows: null.

Just ensure that the script aomp/bin/run_theRockCI.sh includes fail-safes in case it accidentally executes directly on a Windows runner host.

Thanks for the note.
We’ll check and add the fail-safes to aomp/bin/run_theRockCI.sh as suggested in a follow-up change.

.github/workflows/build_windows_artifacts.yml

skganesan008

Please refer to the comments on ROCm/SPIRV-LLVM-Translator#52

skganesan008 · 2026-03-12T16:27:05Z

@emankov
Thanks for taking time to review the github action files.

Just to give you some background, we use the CI framework from the ROCK. The backend, which is the Python framework from the ROCK is used as-is . Our goal is to not diverge from what they are doing on the backend.

The front-end which is all these yaml files, where taken from ROCK CI and leveraged for our needs. We also let temporary workarounds and changes in to these yaml files for our needs. For example, the Windows code has a hardcoded drive (L:) as a map to source code in order to address a command line length issue seen during MIOpen compilation. The Windows builds run on containers where we know from the infrastructure side this drive L: is not already taken up for sure. subst command inside a container is completely invisible to the host and other containers. The "B:" comes from ROCI CI end and that should also be not a problem as it is running inside a container and should not have an issue as these containers are managed internally. When containers change, the change itself goes through a psdb. At a high level, when we look at it from better software development practice, these hardcoding looks bad.

Also, please note that merging changes from Rock would become hard if we combine amd-staging and amd-mainline yaml files. There is always room for improvement and abstraction.

skganesan008 · 2026-03-13T14:38:25Z

@emankov, Let's get this PR landed and Kirthana can help follow up on improvements to this PR.

emankov · 2026-03-17T00:42:47Z

Hello @skganesan008,

please note that merging changes from Rock would become hard if we combine amd-staging and amd-mainline yaml files.

Does a coupled reviewing and merging of two almost-but-not-identical changes look easier/simpler?
Or will all the upcoming changes in the dependent theROC components, including HIPIFY tools, inherit theROC issues implicitly and without review?

[Conclusion]
As long as the PR is marked as do not merge, you can merge it without my approval or anyone else's. I don't mind.
I reviewed it by mistake. It has to be reviewed and fixed, not in HIPIFY, but higher in theROC.

emankov · 2026-03-17T00:43:50Z

@emankov, Let's get this PR landed and Kirthana can help follow up on improvements to this PR.

I don't mind, but the first place to start improvements and fixes looks like theROC itself.

kirthana14m requested review from kzhuravl, lajagapp, ranapratap55, ronlieb and skganesan008 February 11, 2026 12:38

kirthana14m self-assigned this Feb 11, 2026

kirthana14m requested review from emankov and searlmc1 as code owners February 11, 2026 12:39

kirthana14m added infrastructural related to the Repo infrastructure do not review labels Feb 11, 2026

emankov requested changes Feb 11, 2026

View reviewed changes

emankov added the Windows Windows only label Feb 11, 2026

kirthana14m added 2 commits March 11, 2026 03:29

[HIPIFY][Infra] workaround for long paths on Windows build for amd-st…

73b1bb6

…aging

Updating the base image, windows path and test cluster

a656dfa

kirthana14m force-pushed the amd/dev/kirthana14m/windows-path-shrink-staging branch from 6ef1e8b to a656dfa Compare March 11, 2026 10:30

kirthana14m mentioned this pull request Mar 11, 2026

[HIPIFY][Infra] workaround for long paths on Windows build #2384

Closed

kirthana14m changed the title ~~[HIPIFY][Infra] workaround for long paths on Windows build~~ [HIPIFY][Infra] Enable dynamic drive allocation to Windows build Mar 11, 2026

emankov requested changes Mar 11, 2026

View reviewed changes

skganesan008 requested changes Mar 11, 2026

View reviewed changes

kirthana14m added 2 commits March 12, 2026 01:56

Update the aws condition for non forked project

04a1a8d

Update the aws credential configuration for rockci repo

2fd37c1

skganesan008 approved these changes Mar 13, 2026

View reviewed changes

kirthana14m removed the do not review label Mar 17, 2026

kirthana14m merged commit df71038 into amd-staging Mar 17, 2026
34 checks passed

Conversation

kirthana14m commented Feb 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

emankov left a comment

Choose a reason for hiding this comment

Uh oh!

emankov left a comment

Choose a reason for hiding this comment

Uh oh!

emankov Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

kirthana14m Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

emankov Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

kirthana14m Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

emankov Mar 17, 2026

Choose a reason for hiding this comment

Uh oh!

emankov Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

kirthana14m Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

skganesan008 left a comment

Choose a reason for hiding this comment

Uh oh!

skganesan008 commented Mar 12, 2026

Uh oh!

skganesan008 commented Mar 13, 2026 • edited by emankov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

emankov commented Mar 17, 2026

Uh oh!

emankov commented Mar 17, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kirthana14m commented Feb 11, 2026 •

edited

Loading

skganesan008 commented Mar 13, 2026 •

edited by emankov

Loading