Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 19 additions & 33 deletions .github/workflows/build_portable_linux_artifacts.yml
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,8 @@ on:
type: boolean
extra_cmake_options:
type: string
test_type:
type: string

# See the details regarding permissions from the link:
# https://github.com/aws-actions/configure-aws-credentials?tab=readme-ov-file#oidc
Expand Down Expand Up @@ -82,8 +84,9 @@ jobs:
with:
repository: "ROCm/TheRock"
ref: 'compiler/amd-staging'

- name: Update Submodule Pointer to the PR
if: ${{ github.event_name == 'pull_request' }}
run: |
git config --global --add safe.directory $PWD
# Fetch the latest commit SHA from the PR branch
Expand All @@ -96,7 +99,6 @@ jobs:
# Verify the pointer update
git submodule status
git submodule


- name: Install python deps
run: |
Expand All @@ -121,10 +123,6 @@ jobs:
run: |
./build_tools/health_status.py

- name: Test build_tools
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

run: |
python -m pytest build_tools/tests build_tools/github_actions/tests

- name: Remove ununsed existing patchces
run: |
rm -fv patches/amd-mainline/llvm-project/0001-Ensure-to-use-libamdhip64-with-major-version.patch
Expand All @@ -134,32 +132,8 @@ jobs:
run: |
./build_tools/fetch_sources.py --jobs 12

- name: "Checking out repository for llvm-project"
uses: actions/checkout@08c6903cd8c0fde910a37f88322edcfb5dd907a8 # v5.0.0
with:
repository: "ROCm/llvm-project"
path: compiler/amd-llvm
ref: amd-staging

- name: "Checking out repository for spirv-llvm-translator"
uses: actions/checkout@08c6903cd8c0fde910a37f88322edcfb5dd907a8 # v5.0.0
with:
repository: "ROCm/spirv-llvm-translator"
path: compiler/spirv-llvm-translator
ref: amd-staging

- name: Apply patches
run: |
cp -v patches/amd-mainline/llvm-project/*.patch compiler/amd-llvm
cd compiler/amd-llvm
git log -10
git config --global --add safe.directory $PWD
find . -type f -name '*.patch' -exec git apply --check {} \;
find . -type f -name '*.patch' -exec git apply {} \;
git log -15
cd -

- name: TheRock and llvm SHA
if: ${{ github.event_name == 'pull_request' || github.event_name == 'workflow_dispatch' }}
run: |
git config --global --add safe.directory $PWD
git log --oneline -1
Expand All @@ -170,6 +144,7 @@ jobs:
cd -

- name: Configure PR Projects
if: ${{ github.event_name == 'pull_request' || (github.event_name == 'workflow_dispatch' && inputs.test_type != 'full') }}
env:
cmake_preset: ${{ inputs.build_variant_cmake_preset }}
amdgpu_families: ${{ inputs.amdgpu_families }}
Expand All @@ -179,6 +154,17 @@ jobs:
run: |
python3 build_tools/github_actions/build_configure.py --manylinux

- name: Configure All Projects
if: ${{ github.event_name != 'pull_request' && inputs.test_type == 'full' }}
env:
cmake_preset: ${{ inputs.build_variant_cmake_preset }}
amdgpu_families: ${{ inputs.amdgpu_families }}
package_version: ${{ inputs.package_version }}
extra_cmake_options: ${{ inputs.extra_cmake_options }}
BUILD_DIR: build
run: |
python3 build_tools/github_actions/build_configure.py --manylinux

- name: Build therock-archives and therock-dist
run: |
cmake --build build --target therock-archives therock-dist -- -j32
Expand Down Expand Up @@ -218,8 +204,8 @@ jobs:
run: |
python3 build_tools/analyze_build_times.py --build-dir build

- name: Configure AWS Credentials for non-forked repos
if: ${{ always() && !github.event.pull_request.head.repo.fork }}
- name: Configure AWS Credentials
if: ${{ github.repository == 'ROCm/TheRock' }}
uses: aws-actions/configure-aws-credentials@61815dcd50bd041e203e49132bacad1fd04d2708 # v5.1.1
with:
aws-region: us-east-2
Expand Down
93 changes: 60 additions & 33 deletions .github/workflows/build_windows_artifacts.yml
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,8 @@ on:
type: boolean
extra_cmake_options:
type: string
test_type:
type: string

permissions:
contents: read
Expand Down Expand Up @@ -82,6 +84,7 @@ jobs:


- name: Update Submodule Pointer to the PR
if: ${{ github.event_name == 'pull_request' }}
run: |
git config --global --add safe.directory $PWD
# Fetch the latest commit SHA from the PR branch
Expand All @@ -95,7 +98,35 @@ jobs:
git submodule status
git submodule


- name: "Map Current Directory to a Drive"
id: subst
shell: cmd
run: |
setlocal EnableDelayedExpansion
set "currentDir=%cd%"
set "driveLetter="
REM Default: try L: first #L: is default to check
if not exist L:\ (
set "driveLetter=L:"
goto :found
)
REM L: in use — find another available drive letter
for %%L in (Z Y X W V U T S R Q P O N M K) do (
if not exist %%L:\ (
set "driveLetter=%%L:"
goto :found
)
)
echo "No available drive letter found. So exiting" >&2
exit /b 1
:found
subst !driveLetter! "!currentDir!"
cd /d !driveLetter!
dir !driveLetter!
wmic logicaldisk get name
echo drive=!driveLetter!>>"%GITHUB_OUTPUT%"
endlocal

- uses: actions/setup-python@83679a892e2d95755f2dac6acb0bfd1e9ac5d548 # v6.1.0
with:
python-version: 3.12
Expand Down Expand Up @@ -132,10 +163,6 @@ jobs:
ccache --zero-stats
python ./build_tools/health_status.py

- name: Test build_tools
run: |
python -m pytest build_tools/tests build_tools/github_actions/tests

- name: Remove ununsed existing patchces
run: |
rm -fv patches/amd-mainline/llvm-project/0003-HACK-Handle-ROCM-installation-layout-of-lib-llvm-bin.patch
Expand All @@ -145,37 +172,14 @@ jobs:
- name: Fetch sources
timeout-minutes: 30
run: |
cd "${{ steps.subst.outputs.drive }}/"
git config fetch.parallel 10
git config --global core.symlinks true
git config --global core.longpaths true
python ./build_tools/fetch_sources.py --jobs 12

- name: "Checking out repository for llvm-project"
uses: actions/checkout@08c6903cd8c0fde910a37f88322edcfb5dd907a8 # v5.0.0
with:
repository: "ROCm/llvm-project"
path: compiler/amd-llvm
ref: amd-staging

- name: "Checking out repository for spirv-llvm-translator"
uses: actions/checkout@08c6903cd8c0fde910a37f88322edcfb5dd907a8 # v5.0.0
with:
repository: "ROCm/spirv-llvm-translator"
path: compiler/spirv-llvm-translator
ref: amd-staging

- name: Apply patches
run: |
cp -v patches/amd-mainline/llvm-project/*.patch compiler/amd-llvm
cd compiler/amd-llvm
git log -10
git config --global --add safe.directory $PWD
find . -type f -name '*.patch' -exec git apply --check {} \;
find . -type f -name '*.patch' -exec git apply {} \;
git log -15
cd -

- name: TheRock and llvm SHA
if: ${{ github.event_name == 'pull_request' || github.event_name == 'workflow_dispatch' }}
run: |
git config --global --add safe.directory $PWD
git log --oneline -1
Expand All @@ -186,19 +190,36 @@ jobs:
cd -

- name: Configure PR Projects
if: ${{ github.event_name == 'pull_request' || (github.event_name == 'workflow_dispatch' && inputs.test_type != 'full') }}
env:
cmake_preset: ${{ inputs.build_variant_cmake_preset }}
amdgpu_families: ${{ inputs.amdgpu_families }}
package_version: ${{ inputs.package_version }}
#extra_cmake_options: ${{ inputs.extra_cmake_options }}
extra_cmake_options: "-DTHEROCK_ENABLE_ALL=OFF -DTHEROCK_ENABLE_COMPILER=ON -DTHEROCK_ENABLE_HIP_RUNTIME=ON -DTHEROCK_ENABLE_PRIM=ON -DTHEROCK_ENABLE_BLAS=ON -DTHEROCK_ENABLE_RAND=ON -DTHEROCK_ENABLE_SOLVER=ON -DTHEROCK_ENABLE_SPARSE=ON -DTHEROCK_ENABLE_SYSDEPS=OFF -DTHEROCK_ENABLE_COMPOSABLE_KERNEL=OFF -DTHEROCK_ENABLE_OCL_RUNTIME=ON -DTHEROCK_ENABLE_MATH_LIBS=ON -DLLVM_SMREV_REPO='' -DLLVM_SMREV_REVISION=''"
run: |
cd "${{ steps.subst.outputs.drive }}/"
# clear cache before build and after download
ccache -z
python3 build_tools/github_actions/build_configure.py

- name: Configure All Projects
if: ${{ github.event_name != 'pull_request' && inputs.test_type == 'full' }}
env:
cmake_preset: ${{ inputs.build_variant_cmake_preset }}
amdgpu_families: ${{ inputs.amdgpu_families }}
package_version: ${{ inputs.package_version }}
extra_cmake_options: ${{ inputs.extra_cmake_options }}
run: |
# 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
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. You should always use "" for paths instead of '', as in Windows CMD, single quotes are not string delimiters.
  2. Targetting a very specific MSVS version, like 14.44.35207, is a bad solution. You should use vcvars64.bat beforehand.
  3. Could you explain why and where the drive letter B has appeared?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  2. 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.

  3. 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)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


- name: Build therock-archives and therock-dist
run: cmake --build "${{ env.BUILD_DIR }}" --target therock-archives therock-dist -- -j32
run: |
cd "${{ steps.subst.outputs.drive }}/"
cmake --build "${{ env.BUILD_DIR }}" --target therock-archives therock-dist -- -j32

- name: Report
if: ${{ !cancelled() }}
Expand Down Expand Up @@ -232,8 +253,8 @@ jobs:
$fsout | % {$_.Used/=1GB;$_.Free/=1GB;$_} | Write-Host
get-disk | Select-object @{Name="Size(GB)";Expression={$_.Size/1GB}} | Write-Host

- name: Configure AWS Credentials for non-forked repos
if: ${{ always() && !github.event.pull_request.head.repo.fork }}
- name: Configure AWS Credentials
if: ${{ github.repository == 'ROCm/TheRock' }}
uses: aws-actions/configure-aws-credentials@61815dcd50bd041e203e49132bacad1fd04d2708 # v5.1.1
with:
aws-region: us-east-2
Expand All @@ -249,3 +270,9 @@ jobs:
--build-dir ${{ env.BUILD_DIR }} \
--upload

- name: Remove Drive Substitution
if: always() && steps.subst.outcome == 'success'
shell: cmd
run: |
REM Remove the drive mapping
subst ${{ steps.subst.outputs.drive }} /D
6 changes: 4 additions & 2 deletions .github/workflows/setup.yml
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ jobs:
setup:
runs-on: ubuntu-24.04
env:
BASE_REF: '5a23b542e1d03fbb515e014a7d9635c8884b12e6'
BASE_REF: '0efc7532e43c16b532fdd2887e00aec8eb69e10f'
outputs:
enable_build_jobs: true
linux_variants: >
Expand All @@ -56,7 +56,9 @@ jobs:
uses: actions/checkout@8e8c483db84b4bee98b60c0593521ed34d9990e8 # v6.0.1
with:
repository: "ROCm/TheRock"
ref: '5a23b542e1d03fbb515e014a7d9635c8884b12e6'
ref: 'compiler/amd-staging'
fetch-depth: 0

- name: SHA of TheRock
run: |
git rev-parse HEAD
Expand Down
Loading
Loading