🔙 Bring back Debug builds for Windows and more#30
Draft
burgholzer wants to merge 93 commits intomainfrom
Draft
Conversation
This reverts commit 867ad10
burgholzer
added
fix
📝 WalkthroughSummary by CodeRabbit
WalkthroughReplaces the Windows matrix job with two reusable-workflow invocations and a dedicated Windows runner; splits Windows build into modular scripts; adds platform test-installation scripts, shared logging helpers, Ninja-based builds, packaging, and a minimal MLIR integration test; updates changelog and license years. (50 words) Changes
Sequence Diagram(s)sequenceDiagram
actor Dev
participant GH as "GitHub Actions"
participant Entry as "build-mlir-windows.yml"
participant Runner as "build-mlir-windows-runner.yml"
participant ZSTD as "build-zstd.ps1"
participant LLD as "build-lld.ps1"
participant MLIR as "build-mlir.ps1"
participant Package as "package-mlir.ps1"
participant Test as "test-installation.ps1"
participant Artifacts as "GitHub Artifacts"
Dev->>GH: push / open PR
GH->>Entry: trigger windows workflow
Entry->>Runner: invoke reusable runner (llvm-project-ref, runs-on)
Runner->>ZSTD: run build-zstd (checkout, build, upload)
ZSTD->>Artifacts: upload zstd artifact
Runner->>LLD: run build-lld (download zstd, build lld, upload)
LLD->>Artifacts: upload lld artifact
Runner->>MLIR: run build-mlir (download zstd/lld, build Release/Debug)
MLIR->>Package: package archive with zstd
Package->>Artifacts: upload llvm-mlir_*.tar.zst
Runner->>Test: run test-release/debug (download artifacts, extract, test)
Test->>Artifacts: read artifacts, extract, build/run integration test
Test->>Runner: report success/failure
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/toolchain/windows/build.ps1`:
- Line 173: Replace the positional Join-Path calls with named parameters to
improve readability: update both Join-Path invocations used in the $env:PATH
assignment (the outer Join-Path that uses $repo_dir and the inner one that uses
$build_dir and $build_type) to use -Path and -ChildPath (or nested Join-Path
with -Path/-ChildPath) and keep the same resulting strings that include "bin"
and $build_type so the environment PATH concatenation remains identical.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/build-mlir-windows.yml.github/workflows/build-portable-mlir-toolchain.ymlCHANGELOG.mdscripts/toolchain/windows/build.ps1
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/toolchain/windows/build.ps1`:
- Around line 29-30: Update the usage/output comment that currently reads
"Creates zstd-<zstd_version>_windows_<arch>_<host_target>[_debug].zip" to match
actual behavior: either remove the "[_debug]" suffix from that zstd archive
description or explicitly state that zstd packaging is skipped for Debug builds;
then adjust the corresponding code comment near the Debug-branch that skips zstd
packaging (the block that omits zstd packaging for Debug builds) so both the
top-level output doc and the Debug-branch comments consistently reflect that
zstd archives are not produced for Debug builds.
- Around line 169-173: The CMake policy flag in the debug branch is using the
wrong cache variable name; update the argument added to $cmake_args from
'-DCMAKE_POLICY_CMP0141=NEW' to the correct cache variable
'-DCMAKE_POLICY_DEFAULT_CMP0141=NEW' so the policy default is actually applied
(change the entry in the block that appends to $cmake_args in the debug branch).
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/build-mlir-windows.ymlscripts/toolchain/linux/in-container.shscripts/toolchain/macos/build.shscripts/toolchain/windows/build.ps1
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
scripts/toolchain/windows/build.ps1 (1)
29-30:⚠️ Potential issue | 🟡 MinorOutput docs still claim a Debug zstd archive that is never produced.
Line 30 advertises
[_debug]for the zstd zip, but Lines 234-239 explicitly skip zstd packaging in Debug mode.📝 Proposed doc fix
-# - Creates zstd-<zstd_version>_windows_<arch>_<host_target>[_debug].zip in the current directory +# - Creates zstd-<zstd_version>_windows_<arch>_<host_target>.zip in the current directory (Release builds only)Also applies to: 234-239
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/toolchain/windows/build.ps1` around lines 29 - 30, The header output comment incorrectly advertises a debug zstd archive ("zstd-<zstd_version>_windows_<arch>_<host_target>[_debug].zip") while the script's Debug-mode conditional explicitly skips zstd packaging; fix by making them consistent: either remove the "[_debug]" suffix from the header comment that lists zstd output, or change the Debug-mode conditional that skips zstd packaging so it produces the debug zstd archive; specifically update the output docs line containing "zstd-<zstd_version>_windows_<arch>_<host_target>[_debug].zip" or adjust the zstd-packaging conditional that currently bypasses zstd creation in Debug mode.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/build-mlir-windows.yml:
- Around line 119-121: The removal of temp directories via Remove-IfExists
("C:\Windows\Temp", $env:TEMP, $env:LOCALAPPDATA\Temp) can break later steps;
after each Remove-IfExists call (or once after all removals) ensure the
directories are recreated and writable by calling the appropriate
create/ensure-directory routine (e.g., use New-Item -ItemType Directory or an
Ensure-Directory helper) for "C:\Windows\Temp", $env:TEMP and
$env:LOCALAPPDATA\Temp so subsequent actions have valid temp paths; keep the
recreate step immediately after the Remove-IfExists invocations and preserve
correct permissions/ownership.
---
Duplicate comments:
In `@scripts/toolchain/windows/build.ps1`:
- Around line 29-30: The header output comment incorrectly advertises a debug
zstd archive ("zstd-<zstd_version>_windows_<arch>_<host_target>[_debug].zip")
while the script's Debug-mode conditional explicitly skips zstd packaging; fix
by making them consistent: either remove the "[_debug]" suffix from the header
comment that lists zstd output, or change the Debug-mode conditional that skips
zstd packaging so it produces the debug zstd archive; specifically update the
output docs line containing
"zstd-<zstd_version>_windows_<arch>_<host_target>[_debug].zip" or adjust the
zstd-packaging conditional that currently bypasses zstd creation in Debug mode.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/build-mlir-windows.ymlscripts/toolchain/linux/in-container.shscripts/toolchain/macos/build.shscripts/toolchain/windows/build.ps1
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
scripts/toolchain/windows/build.ps1 (1)
29-30:⚠️ Potential issue | 🟡 MinorDocumentation inconsistency for zstd archive in Debug builds.
Line 30 states the zstd archive is created with an optional
_debugsuffix, but lines 234-239 skip zstd packaging entirely for Debug builds. The documentation should clarify that zstd archives are only produced for Release builds.📝 Proposed documentation fix
# Outputs: # - Installs into <install_prefix> # - Creates llvm-mlir_<llvm_project_ref>_windows_<arch>_<host_target>[_debug].tar.zst in the current directory -# - Creates zstd-<zstd_version>_windows_<arch>_<host_target>.zip in the current directory (for Release builds) +# - Creates zstd-<zstd_version>_windows_<arch>_<host_target>.zip in the current directory (Release builds only)This is a duplicate concern from a past review but the original issue was about the
[_debug]suffix in the zstd description which has now been partially addressed by adding "(for Release builds)" but remains slightly misleading.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/toolchain/windows/build.ps1` around lines 29 - 30, Update the header comment that describes the output artifacts so it no longer suggests a zstd archive may include an optional "_debug" suffix; explicitly state that zstd-<zstd_version>_windows_<arch>_<host_target>.zip is only produced for Release builds (matching the packaging logic that skips zstd for Debug), and remove or correct the "[_debug]" mention in the zstd line so the header accurately reflects the behavior implemented later in the script.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/build-mlir-windows.yml:
- Around line 152-155: The conditional in the upload step's path uses `${{
inputs.build-type == 'Release' && 'zstd-*.zip' || '' }}` which can produce an
empty string and makes `path` empty for non-Release builds; fix by either (A)
moving the entire upload step behind `if: ${{ inputs.build-type == 'Release' }}`
so the step only runs for Release builds (keep name and uses:
actions/upload-artifact@... and list both patterns `llvm-mlir_*.tar.zst` and
`zstd-*.zip` in path), or (B) keep the step always present but ensure `path` is
never empty by replacing the false branch with a non-existent placeholder (e.g.
`'__no_such_path__'`) and add `if-no-files-found: ignore` to the upload-artifact
step so it won’t fail when no files match; update the `path` and
`if-no-files-found` settings accordingly.
In `@scripts/toolchain/linux/in-container.sh`:
- Around line 193-199: The conditional copy that checks [[ -d
"$ZSTD_INSTALL_PREFIX/lib/cmake" ]] and then cp -r
"$ZSTD_INSTALL_PREFIX/lib/cmake/." "$INSTALL_PREFIX/lib/cmake/" is redundant
because the preceding cp -r "$ZSTD_INSTALL_PREFIX/lib/." "$INSTALL_PREFIX/lib/"
already copies lib/cmake; remove that conditional block to avoid duplicate work,
or if you intended to handle an alternate cmake location, replace it with a
clarifying comment explaining the special case and why an extra copy is required
(referencing the variables $ZSTD_INSTALL_PREFIX and $INSTALL_PREFIX and the cp
-r operations).
---
Duplicate comments:
In `@scripts/toolchain/windows/build.ps1`:
- Around line 29-30: Update the header comment that describes the output
artifacts so it no longer suggests a zstd archive may include an optional
"_debug" suffix; explicitly state that
zstd-<zstd_version>_windows_<arch>_<host_target>.zip is only produced for
Release builds (matching the packaging logic that skips zstd for Debug), and
remove or correct the "[_debug]" mention in the zstd line so the header
accurately reflects the behavior implemented later in the script.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/build-mlir-windows.ymlscripts/toolchain/linux/in-container.shscripts/toolchain/windows/build.ps1
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/build-mlir-linux.yml:
- Around line 75-78: Replace the fragile use of ls when setting archive with a
robust find command: change the archive assignment (the line setting
archive=$(ls llvm-mlir_*.tar.zst | head -1)) to use find to locate the first
matching file (e.g. use find . -maxdepth 1 -type f -name 'llvm-mlir_*.tar.zst'
-print -quit and assign its basename or path to archive) so the subsequent bash
call that passes "${{ github.workspace }}/$archive" uses a reliably discovered
filename; ensure you preserve the existing behavior of choosing the first match
and handle the case where find returns nothing (fail the job or error out) in
the same location where archive is used.
- Around line 64-67: Replace the loose tag actions/download-artifact@v4 with the
repository commit SHA used elsewhere in this workflow (matching the pinning
pattern), e.g. actions/download-artifact@<SHA> # v4; locate the step named
"Download archive" and update the uses value to the SHA-pinned ref so it follows
the same pinning style as actions/checkout and actions/upload-artifact.
In @.github/workflows/build-mlir-macos.yml:
- Around line 46-49: Update the "Download archive" step that currently uses
actions/download-artifact@v4 to pin it to a specific commit SHA for security and
reproducibility; replace the version tag in the uses field
(actions/download-artifact@v4) with the repository@<commit-sha> form (the exact
SHA from the Linux workflow) so the "Download archive" step uses
actions/download-artifact@<sha>.
- Around line 57-60: Replace the fragile ls-based archive discovery assigned to
the archive variable with a find-based lookup to handle special characters: use
find to search for files matching the pattern llvm-mlir_*.tar.zst (e.g.,
starting from the workspace directory or current directory), limit to a single
result (first match) and assign that path to the archive variable, then pass
"${{ github.workspace }}/$archive" to the test-installation.sh invocation
unchanged; update the line that sets archive and keep the subsequent bash call
to test-installation.sh using the -a and -z flags as before.
In @.github/workflows/build-mlir-windows.yml:
- Around line 168-171: The workflow step named "Download archive" currently uses
actions/download-artifact@v4; replace that tag with a specific commit SHA to
match the other pinned actions (i.e., change uses: actions/download-artifact@v4
to uses: actions/download-artifact@<commit-sha>) so the step is pinned for
supply-chain security; locate the appropriate commit SHA for the
actions/download-artifact repository (matching the v4 release you expect) and
update the uses line in that "Download archive" step.
In `@tests/integration/main.cpp`:
- Around line 24-38: ModuleOp created with mlir::ModuleOp::create is an owning
operation that should be explicitly erased before the mlir::MLIRContext is
destroyed; update main to destroy/erase the module operation (call the
appropriate erase/destroy method on the mlir::ModuleOp instance returned by
ModuleOp::create, e.g. module.erase() or module->destroy(), before returning) so
the module is cleaned up explicitly instead of relying on implicit context
teardown.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (13)
.github/workflows/build-mlir-linux.yml.github/workflows/build-mlir-macos.yml.github/workflows/build-mlir-windows.yml.license-tools-config.jsonscripts/toolchain/linux/build.shscripts/toolchain/linux/in-container.shscripts/toolchain/linux/test-installation.shscripts/toolchain/macos/build.shscripts/toolchain/macos/test-installation.shscripts/toolchain/windows/build.ps1scripts/toolchain/windows/test-installation.ps1tests/integration/CMakeLists.txttests/integration/main.cpp
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
1 participant
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.
This reverts commit 867ad10.
MQT Core Windows Debug tests won't compile when mixing Release MLIR builds with Debug MQT Core builds.
We have to make this work somehow.