Add Windows runner to bsdtar wf#2813
Merged
Merged
Conversation
Contributor
|
Note Gemini is unable to generate a summary for this pull request due to the file types involved not being currently supported. |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe GitHub Actions workflow Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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}
Contributor
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 @.github/workflows/bsdtar-compat.yml:
- Line 69: The workflow uses step-level "shell: ${{ matrix.shell }}" and similar
uses of "matrix.shell" (and "defaults.run.shell") which GitHub Actions disallows
because "shell" cannot contain expressions; fix by removing expression
interpolation from all "shell:" keys (instances referencing matrix.shell at
lines reported) and instead implement per-shell behavior by either (a) splitting
into separate jobs per shell value in the strategy matrix and hardcoding a
literal shell in each job, or (b) keep a single concrete shell (e.g., bash) and
drive platform-specific commands via environment variables or conditional logic
inside the "run:" script; update all affected "shell:" occurrences and any
defaults.run.shell usage to one of these patterns and ensure any conditional
gating uses "if:" or separate jobs rather than interpolated "shell:" values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9f393895-137e-4e5a-aeaa-74fb693157d1
📒 Files selected for processing (1)
.github/workflows/bsdtar-compat.yml
Contributor
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 @.github/workflows/bsdtar-compat.yml:
- Around line 70-83: The Windows dependency installation step named "Install
build dependencies (Windows)" is missing autotools; update the pacman install
list in that step to include autoconf, automake, and libtool so the subsequent
libarchive build commands (./build/autogen.sh and ./configure) can run
successfully.
- Around line 160-175: The Windows dependency install step "Install dependencies
(Windows)" (runs when matrix.windows is true) is missing autotools needed to
build libarchive; update the pacman package list in that step to include
autoconf, automake, and libtool so the libarchive build can run successfully.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5171243e-4f4c-42f7-b39c-0e16e0929678
📒 Files selected for processing (1)
.github/workflows/bsdtar-compat.yml
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
.github/workflows/bsdtar-compat.yml (1)
56-58:⚠️ Potential issue | 🔴 CriticalThe
defaults.run.shelldoes not support expression interpolation.GitHub Actions docs explicitly state: "You cannot use contexts or expressions in this keyword." This pattern will fail workflow validation.
The same issue applies to the
pna-stdiojob at lines 148-150.Consider either:
- Using separate jobs per platform with hardcoded shell values
- Using a fixed shell (e.g.,
bash) and handling platform differences via environment variables or conditional logic within run scripts- Using step-level
ifconditions with separate steps per platform that have hardcoded shells,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/bsdtar-compat.yml around lines 56 - 58, The workflow uses expression interpolation in defaults.run.shell (and similarly in the pna-stdio job) which is invalid; remove the expression usage and replace it by either (a) hardcoding the shell per job (create separate jobs per platform and set run.shell to the concrete value), (b) set a fixed cross-platform shell (e.g., bash) in defaults.run.shell and handle platform differences inside run scripts, or (c) move the conditional logic down to step-level and create separate steps with hardcoded shell values; update references to defaults.run.shell and the pna-stdio job accordingly so no expression interpolation remains.
🧹 Nitpick comments (1)
.github/workflows/bsdtar-compat.yml (1)
337-348: Considergrep -oPavailability on Windows MSYS2.The
grep -oP(Perl regex) is used throughout the summary generation. While MSYS2's default grep package typically supports PCRE, this could fail if thepcrepackage is not installed.Consider adding
pcreto the Windows dependencies, or use POSIX-compatible alternatives likesedorawkfor extraction.Alternative using awk for better portability
- LAST_TEST=$(grep -oP '^\s+\d+: \K\S+' bsdtar_test_output.txt | tail -1 || echo "unknown") + LAST_TEST=$(awk '/^[[:space:]]+[0-9]+:/ {sub(/^[[:space:]]+[0-9]+: /, ""); print $1}' bsdtar_test_output.txt | tail -1) + LAST_TEST=${LAST_TEST:-unknown}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/bsdtar-compat.yml around lines 337 - 348, The summary generation uses grep -oP (Perl regex), which can fail on Windows/MSYS2 if PCRE support is missing; update the aggregation loops that read from bsdtar_test_output.txt (the blocks populating TOTAL, FAILED, ASSERTIONS, ASSERT_FAIL, SKIPS) to avoid grep -oP. Replace each grep -oP invocation with a POSIX-friendly extraction (e.g., use awk or sed to match the label like "Tests run:", "Tests failed:", "Assertions checked:", "Assertions failed:", "Skips reported:" and print the numeric field) or alternatively ensure the Windows job installs the pcre-enabled grep by adding the pcre/pcregrep dependency to the Windows setup; pick one approach and apply it consistently to the loops that feed TOTAL, FAILED, ASSERTIONS, ASSERT_FAIL, and SKIPS.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.github/workflows/bsdtar-compat.yml:
- Around line 56-58: The workflow uses expression interpolation in
defaults.run.shell (and similarly in the pna-stdio job) which is invalid; remove
the expression usage and replace it by either (a) hardcoding the shell per job
(create separate jobs per platform and set run.shell to the concrete value), (b)
set a fixed cross-platform shell (e.g., bash) in defaults.run.shell and handle
platform differences inside run scripts, or (c) move the conditional logic down
to step-level and create separate steps with hardcoded shell values; update
references to defaults.run.shell and the pna-stdio job accordingly so no
expression interpolation remains.
---
Nitpick comments:
In @.github/workflows/bsdtar-compat.yml:
- Around line 337-348: The summary generation uses grep -oP (Perl regex), which
can fail on Windows/MSYS2 if PCRE support is missing; update the aggregation
loops that read from bsdtar_test_output.txt (the blocks populating TOTAL,
FAILED, ASSERTIONS, ASSERT_FAIL, SKIPS) to avoid grep -oP. Replace each grep -oP
invocation with a POSIX-friendly extraction (e.g., use awk or sed to match the
label like "Tests run:", "Tests failed:", "Assertions checked:", "Assertions
failed:", "Skips reported:" and print the numeric field) or alternatively ensure
the Windows job installs the pcre-enabled grep by adding the pcre/pcregrep
dependency to the Windows setup; pick one approach and apply it consistently to
the loops that feed TOTAL, FAILED, ASSERTIONS, ASSERT_FAIL, and SKIPS.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b465a91f-a678-4afd-9aa7-3e3b4e9cdc66
📒 Files selected for processing (5)
.github/workflows/bsdtar-compat.ymlcli/src/command/core.rscli/src/command/diff.rscli/src/utils/fs/file_id.rscli/src/utils/fs/owner.rs
💤 Files with no reviewable changes (1)
- cli/src/utils/fs/file_id.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
.github/workflows/bsdtar-compat.yml (2)
250-252: Inconsistent shell handling compared to "Build pna" step.The "Build pna" step (lines 214-219) explicitly uses
shell: pwshfor Windows, but this step relies on the default shell. For consistency and reliability, consider adding a Windows-specific step withshell: pwshoverride.♻️ Proposed fix for consistent Windows handling
- name: Build tar2pna + if: ${{ !matrix.windows }} run: cargo build --locked --release -p xtask working-directory: pna + + - name: Build tar2pna (Windows) + if: ${{ matrix.windows }} + shell: pwsh + run: cargo build --locked --release -p xtask + working-directory: pna🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/bsdtar-compat.yml around lines 250 - 252, The "Build tar2pna" job step named "Build tar2pna" is missing an explicit Windows shell override and is inconsistent with the "Build pna" step; update the workflow to add a Windows-specific variant or conditional for this step that sets shell: pwsh (matching the "Build pna" behavior) when running on windows-latest, or include a matrix/if conditional that uses shell: pwsh for runner.os == 'Windows' so the cargo build --locked --release -p xtask command in the pna working-directory runs under PowerShell on Windows.
267-271: Consider creating platform-specific wrappers for robust cross-platform execution.The
pnatarwrapper is a Unix shell script with#!/bin/shshebang. While the workflow runs within an MSYS2 bash environment on Windows (which provides POSIX compatibility), relying on this context is fragile. Whenbsdtar_test(a MinGW64-compiled binary) invokes the script via its-pflag, Windows may not properly interpret the shebang depending on how the executable spawns processes.Creating platform-appropriate wrappers ensures the solution works consistently regardless of MSYS2 implementation details:
Suggested approach: Platform-specific wrappers
- name: Create pnatar wrapper + if: ${{ !matrix.windows }} run: | WORKSPACE="$(pwd)" printf '#!/bin/sh\nexec "%s" --log-level error experimental stdio --unstable "$@"\n' "${PNA_BIN}" > "${WORKSPACE}/pnatar" chmod +x "${WORKSPACE}/pnatar" + + - name: Create pnatar wrapper (Windows) + if: ${{ matrix.windows }} + shell: pwsh + run: | + $pnaBin = "$env:PNA_BIN" -replace '\\', '/' + @" + `@echo` off + "$pnaBin" --log-level error experimental stdio --unstable %* + "@ | Set-Content -Path "pnatar.cmd" -Encoding ASCII🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/bsdtar-compat.yml around lines 267 - 271, The current pnatar wrapper created by the workflow is a Unix shell script (pnatar) which can be fragile on Windows; update the workflow step that creates pnatar to emit platform-specific wrappers: when on Unix produce the existing shell script using PNA_BIN and chmod +x it, and when on Windows produce a .bat wrapper (e.g., pnatar.bat) that forwards arguments to "%PNA_BIN%" /p or the equivalent invocation so MinGW/bsdtar_test's use of the -p flag will work; ensure the workflow uses the appropriate wrapper name (pnatar on Unix, pnatar.bat on Windows) where bsdtar_test invokes the helper and set the file names/permissions accordingly.
🤖 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/bsdtar-compat.yml:
- Line 346: The current artifact copy step only targets /tmp (cp -rf
/tmp/bsdtar* artifact-staging/) and will miss Windows temp files; update the
workflow to also copy from the Windows temp location by adding a platform-aware
copy: keep the existing cp line for Linux/macOS and add a Windows-specific
command that copies from the %TEMP% (or $env:TEMP in PowerShell) path (e.g.,
copy %TEMP%\bsdtar* to artifact-staging or use PowerShell Copy-Item -Recurse),
gated by the runner/os or conditional step so both bsdtar_test.exe artifacts are
picked up on Windows and the existing behavior remains for Unix runners.
---
Nitpick comments:
In @.github/workflows/bsdtar-compat.yml:
- Around line 250-252: The "Build tar2pna" job step named "Build tar2pna" is
missing an explicit Windows shell override and is inconsistent with the "Build
pna" step; update the workflow to add a Windows-specific variant or conditional
for this step that sets shell: pwsh (matching the "Build pna" behavior) when
running on windows-latest, or include a matrix/if conditional that uses shell:
pwsh for runner.os == 'Windows' so the cargo build --locked --release -p xtask
command in the pna working-directory runs under PowerShell on Windows.
- Around line 267-271: The current pnatar wrapper created by the workflow is a
Unix shell script (pnatar) which can be fragile on Windows; update the workflow
step that creates pnatar to emit platform-specific wrappers: when on Unix
produce the existing shell script using PNA_BIN and chmod +x it, and when on
Windows produce a .bat wrapper (e.g., pnatar.bat) that forwards arguments to
"%PNA_BIN%" /p or the equivalent invocation so MinGW/bsdtar_test's use of the -p
flag will work; ensure the workflow uses the appropriate wrapper name (pnatar on
Unix, pnatar.bat on Windows) where bsdtar_test invokes the helper and set the
file names/permissions accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c7125fc6-099f-4c36-82d6-61c688976582
📒 Files selected for processing (2)
.github/workflows/bsdtar-compat.ymlcli/src/command/core.rs
ChanTsune
force-pushed
the
ci/bsdtar-compat/add-windows
branch
2 times, most recently
from
7099d86 to
09eba70
Compare
Extend .github/workflows/bsdtar-compat.yml to run the bsdtar reference and pna stdio compatibility jobs on Windows in addition to Ubuntu. Use the libarchive Windows build flow for bsdtar and bsdtar_test, add a native pnatar.cmd wrapper for bsdtar_test.exe, keep pna stdio summaries, and skip summary output for the reference-only jobs while still failing on abnormal termination.
ChanTsune
force-pushed
the
ci/bsdtar-compat/add-windows
branch
from
af5d0be to
9afeba4
Compare
This was referenced Mar 15, 2026
This was referenced Mar 26, 2026
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.
Summary by CodeRabbit