[ci] adding windows configuration for configure project script by geomin12 · Pull Request #857 · ROCm/TheRock

geomin12 · 2025-06-17T22:40:55Z

Initially, project configurations were specifically linux. now, this script handles both linux and windows

Working here for rocm-libraries (a note: build is failing for external reasons, but the configure step works)

once this goes through, will update in here: ROCm/rocm-libraries#156

ScottTodd

The structure of this LGTM, but I've tagged a few things to fix.

ScottTodd · 2025-06-18T15:13:43Z

+          package_version: ${{ inputs.package_version }}
+          extra_cmake_options: ${{ inputs.extra_cmake_options }}
+          build_dir: ${{ env.BUILD_DIR_BASH }}
+          vctools_install_dir: ${{ env.VCToolsInstallDir }}


Forwarding one environment variable to another doesn't look necessary here.

i just liked the uniformity of the snake case, but i agree! will update

ScottTodd · 2025-06-18T15:15:26Z

+vctools_install_dir = os.getenv("vctools_install_dir")
+github_workspace = os.getenv("GITHUB_WORKSPACE")

+platform_options = {
+    "linux": ["-DTHEROCK_VERBOSE=ON", "-DBUILD_TESTING=ON"],
+    "windows": [
+        f"-DCMAKE_C_COMPILER={vctools_install_dir}/bin/Hostx64/x64/cl.exe",
+        f"-DCMAKE_CXX_COMPILER={vctools_install_dir}/bin/Hostx64/x64/cl.exe",
+        f"-DCMAKE_LINKER={vctools_install_dir}/bin/Hostx64/x64/link.exe",


This script should check that the environment variables are set and provide useful error messages when they are not. Right now this will try to use /bin/Hostx64/x64/cl.exe if vctools_install_dir is unset. That path looks like a Linux path so it would be easy to get confused by in the logs.

ScottTodd · 2025-06-18T15:17:28Z

+        f"-DCMAKE_LINKER={vctools_install_dir}/bin/Hostx64/x64/link.exe",
+        "-DTHEROCK_BACKGROUND_BUILD_JOBS=4",
+        "-DCMAKE_MSVC_DEBUG_INFORMATION_FORMAT=Embedded",
+        f"-DTHEROCK_AMDGPU_WINDOWS_INTEROP_DIR={github_workspace}/amdgpu-windows-interop",


For this script to be testable outside of GitHub Actions, this could fallback to omitting the argument if GITHUB_WORKSPACE is undefined or no directory exists at that path. That would use the default CMake behavior:

TheRock/CMakeLists.txt

Line 76 in 34df5ba

set(THEROCK_AMDGPU_WINDOWS_INTEROP_DIR "${CMAKE_CURRENT_SOURCE_DIR}/../amdgpu-windows-interop" CACHE PATH "Directory containing the Windows AMDGPU/ROCm driver interop support files")

Question: I understand that vctools_install_dir is required (as it needs C, C++ to compile), but is amdgpu-windows-interop optional? I made the update where it is optional but not sure

It's optional if the directory exists in the default location. I tried to get the checkout action to use that path but found it easier to line up the paths with an explicit argument instead.

That being said... we should just convert from a separate install to something automatically handled by the build system: #693

So given that we want to simplify anyway, you could keep the existing github_workspace behavior in this script. That will make it easier to just remove a single line when we have the automatic repository fetch.

ScottTodd · 2025-06-18T15:18:30Z

+github_workspace = os.getenv("GITHUB_WORKSPACE")

+platform_options = {
+    "linux": ["-DTHEROCK_VERBOSE=ON", "-DBUILD_TESTING=ON"],


These options look useful across both operating systems. Why are they Linux specific?

Good question:

do we still find value in the -DBUILD_TESTING=ON as default in build_windows_packages CI? I know this was an initial choice as we were still testing windows, but maybe it's in a solid spot and can be default?

I have no idea why windows doesn't have -DTHEROCK_VERBOSE=ON flag. for some reason (without proof) in my head, i thought windows was incredibly verbose and we decided to ignore? if my assumption is wrong, i can add it :)

That -DBUILD_TESTING=ON was leftover from making the workflow build with and without tests in #552. Yes, it can be the default now. Separate from what the default is for that option, extra_cmake_options benefits from having documentation to make using it via workflow_dispatch easy - that can be either via a default value or a description.

The verbose setting is fine to enable. I build locally with it, and we usually want as much information as we can get in CI logs.

ScottTodd · 2025-06-18T17:08:40Z

          package_version: ${{ inputs.package_version }}
          extra_cmake_options: ${{ inputs.extra_cmake_options }}
-          build_dir: build
+          BUILD_DIR_BASH: build


I'd prefer to just call this build_dir (or BUILD_DIR). We still need to remove the non-bash path handling around here:

TheRock/.github/workflows/build_windows_packages.yml

Lines 42 to 62 in a2dd40a

BASE_BUILD_DIR_POWERSHELL: B:\tmpbuild

CACHE_DIR: "${{github.workspace}}/.cache"

CCACHE_DIR: "${{github.workspace}}/.cache/ccache"

CCACHE_MAXSIZE: "4000M"

TEATIME_FORCE_INTERACTIVE: 0

AMDGPU_FAMILIES: ${{ inputs.amdgpu_families }}

steps:

- name: "Create build dir"

shell: powershell

run: |

$buildDir = "$env:BASE_BUILD_DIR_POWERSHELL\"

echo "BUILD_DIR_POWERSHELL=$buildDir" >> $env:GITHUB_ENV

mkdir "$buildDir"

Write-Host "Generated Build Directory: $buildDir"

$bashBuildDir = $buildDir -replace '\\', '/' -replace '^B:', '/b'

echo "BUILD_DIR_BASH=$bashBuildDir" >> $env:GITHUB_ENV

Write-Host "Converted Build Directory For Bash: $bashBuildDir"

$fs = Get-PSDrive -PSProvider "FileSystem"

$fsout = $fs | Select-Object -Property Name,Used,Free,Root

$fsout | % {$_.Used/=1GB;$_.Free/=1GB;$_} | Write-Host

get-disk | Select-object @{Name="Size(GB)";Expression={$_.Size/1GB}} | Write-Host

Could clean that up together with changes to the other windows workflows

Yes, can we just remove all of that? I'm not sure what is needed or not. Will cmake handle creating that directory properly like linux?

i'll try ! one way to find out :)

I think the only load bearing detail there is using the B:/ drive instead of C:/, since the OS partition is too small to fit the build outputs. The powershell/bash split was only needed when we had powershell commands in the workflows, but all of those have been moved to Python scripts that run under bash.

That code is also copied to

TheRock/.github/workflows/release_windows_packages.yml

Lines 99 to 125 in a2dd40a

BASE_BUILD_DIR_POWERSHELL: B:\tmpbuild

CACHE_DIR: "${{github.workspace}}/.cache"

CCACHE_DIR: "${{github.workspace}}/.cache/ccache"

CCACHE_MAXSIZE: "4000M"

TEATIME_LABEL_GH_GROUP: 1

FILE_NAME: "therock-dist-windows-${{ matrix.target_bundle.amdgpu_family }}${{ inputs.package_suffix }}-${{ needs.setup_metadata.outputs.version }}.tar.gz"

DIST_ARCHIVE: "B:/tmpbuild/artifacts/therock-dist-windows-${{ matrix.target_bundle.amdgpu_family }}${{ inputs.package_suffix }}-${{ needs.setup_metadata.outputs.version }}.tar.gz"

RELEASE_TYPE: "${{ needs.setup_metadata.outputs.release_type }}"

S3_TARBALL: "therock-${{ needs.setup_metadata.outputs.release_type }}-tarball"

steps:

# TODO(amd-justchen): share with build_windows_packages.yml. Use pre-job hook?

# Maybe we can remove _POWERSHELL entirely now that commands are all bash?

- name: "Create build dir"

shell: powershell

run: |

$buildDir = "$env:BASE_BUILD_DIR_POWERSHELL\"

echo "BUILD_DIR_POWERSHELL=$buildDir" >> $env:GITHUB_ENV

mkdir "$buildDir"

Write-Host "Generated Build Directory: $buildDir"

$bashBuildDir = $buildDir -replace '\\', '/' -replace '^B:', '/b'

echo "BUILD_DIR_BASH=$bashBuildDir" >> $env:GITHUB_ENV

Write-Host "Converted Build Directory For Bash: $bashBuildDir"

$fs = Get-PSDrive -PSProvider "FileSystem"

$fsout = $fs | Select-Object -Property Name,Used,Free,Root

$fsout | % {$_.Used/=1GB;$_.Free/=1GB;$_} | Write-Host

get-disk | Select-object @{Name="Size(GB)";Expression={$_.Size/1GB}} | Write-Host

I'd prefer to clean that up in a separate PR across both files.

ScottTodd · 2025-06-18T17:11:47Z

+            raise Exception(
+                "Environment variable VCToolsInstallDir is not set. Exiting."
+            )


This could suggest configuring MSVC: https://github.com/ROCm/TheRock/blob/main/docs/development/windows_support.md#important-tool-settings

(fine to leave as is though)

ScottTodd · 2025-06-18T20:53:07Z

+"""
+This script runs the Linux build configuration
+
+Required environment variables:
+  - amdgpu_families
+  - package_version
+  - extra_cmake_options
+"""


Please update this docstring (works on both Windows and Linux, then document which env vars are required on each platform)

unfortunate. let me update this in the next PR

geomin12 added 3 commits June 17, 2025 10:53

Adding uniform build_configure

3d4f02a

lint

5e876f1

missing comma

3728163

github-project-automation Bot added this to TheRock Triage Jun 17, 2025

github-project-automation Bot moved this to TODO in TheRock Triage Jun 17, 2025

geomin12 changed the title ~~[ci] adding windows configuration for cmake~~ [ci] adding windows configuration for configure project script Jun 18, 2025

geomin12 marked this pull request as ready for review June 18, 2025 00:29

geomin12 requested review from ScottTodd and marbre June 18, 2025 00:31

marbre approved these changes Jun 18, 2025

View reviewed changes

ScottTodd requested changes Jun 18, 2025

View reviewed changes

PR comments

9987f08

geomin12 requested a review from ScottTodd June 18, 2025 16:13

geomin12 added 2 commits June 18, 2025 09:14

Removing Path

62d89e0

Pr comments

d8b5f37

ScottTodd approved these changes Jun 18, 2025

View reviewed changes

geomin12 added 2 commits June 18, 2025 10:27

Adding cleanup

8ee3ed3

Adding back

e135e04

ScottTodd approved these changes Jun 18, 2025

View reviewed changes

geomin12 merged commit 646786e into main Jun 18, 2025
23 checks passed

geomin12 deleted the users/geomin12/windows-monorepo-cleanup branch June 18, 2025 20:53

github-project-automation Bot moved this from TODO to Done in TheRock Triage Jun 18, 2025

	BASE_BUILD_DIR_POWERSHELL: B:\tmpbuild
	CACHE_DIR: "${{github.workspace}}/.cache"
	CCACHE_DIR: "${{github.workspace}}/.cache/ccache"
	CCACHE_MAXSIZE: "4000M"
	TEATIME_FORCE_INTERACTIVE: 0
	AMDGPU_FAMILIES: ${{ inputs.amdgpu_families }}
	steps:
	- name: "Create build dir"
	shell: powershell
	run: \|
	$buildDir = "$env:BASE_BUILD_DIR_POWERSHELL\"
	echo "BUILD_DIR_POWERSHELL=$buildDir" >> $env:GITHUB_ENV
	mkdir "$buildDir"
	Write-Host "Generated Build Directory: $buildDir"
	$bashBuildDir = $buildDir -replace '\\', '/' -replace '^B:', '/b'
	echo "BUILD_DIR_BASH=$bashBuildDir" >> $env:GITHUB_ENV
	Write-Host "Converted Build Directory For Bash: $bashBuildDir"
	$fs = Get-PSDrive -PSProvider "FileSystem"
	$fsout = $fs \| Select-Object -Property Name,Used,Free,Root
	$fsout \| % {$_.Used/=1GB;$_.Free/=1GB;$_} \| Write-Host
	get-disk \| Select-object @{Name="Size(GB)";Expression={$_.Size/1GB}} \| Write-Host

Conversation

geomin12 commented Jun 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ScottTodd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

geomin12 commented Jun 17, 2025 •

edited

Loading