-
Notifications
You must be signed in to change notification settings - Fork 9
fix: add BSD tar fallback support for macOS #102
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Modify tar extraction to detect if gtar is available and fall back to system tar on macOS. GNU-specific flags (--no-anchored, --wildcards) are only used when gtar is present, enabling builds on macOS without requiring GNU tar installation. This resolves "command not found: gtar" errors on macOS systems where GNU tar is not installed, allowing the build system to use the native BSD tar instead.
WalkthroughDetects available archiver, builds a portable TAR_CMD array at runtime with conditional GNU-only flags, and appends dynamic --exclude/--include patterns before executing the assembled tar command, replacing the previous inline tar extraction invocation. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
Break long TAR_CMD array initialization into multiple lines to comply with yamllint max line length of 100 characters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@exports/taskfiles/utils/remote.yaml`:
- Line 116: The TAR_CMD array declaration exceeds the 100-character limit; break
the array into multiple lines so each element is on its own line to satisfy
linting. Modify the TAR_CMD assignment (the variable name TAR_CMD and use of
ARCHIVER, --extract, --strip-components="{{.NUM_COMPONENTS_TO_STRIP}}",
--directory "{{.OUTPUT_DIR}}", and --file "{{.TAR_FILE}}") so the opening
parenthesis and ARCHIVER are on the first line, each flag/argument is on its own
subsequent line, and the closing parenthesis is on its own line to keep line
lengths under 100 characters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@exports/taskfiles/utils/remote.yaml`:
- Around line 139-140: The tar invocation currently silences all stderr via the
redirection on the "${TAR_CMD[@]}" invocation, which can hide real errors;
update the call to either remove the "2> /dev/null" so tar errors surface or
redirect stderr to a log file/variable instead, and if suppression is
intentionally desired add a clarifying comment explaining why warnings are
ignored; reference the TAR_CMD invocation ("${TAR_CMD[@]}") and ensure the
change preserves the intended behavior for noisy but non-fatal warnings or
surfaces fatal errors for debugging.
| # Execute tar command | ||
| "${TAR_CMD[@]}" 2> /dev/null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick | 🔵 Trivial
Consider whether suppressing all stderr is desirable.
Redirecting 2> /dev/null hides all tar errors, which could mask legitimate issues (e.g., corrupted archives, permission problems). If this is intentional to suppress noisy warnings, a comment explaining the rationale would help future maintainers.
🤖 Prompt for AI Agents
In `@exports/taskfiles/utils/remote.yaml` around lines 139 - 140, The tar
invocation currently silences all stderr via the redirection on the
"${TAR_CMD[@]}" invocation, which can hide real errors; update the call to
either remove the "2> /dev/null" so tar errors surface or redirect stderr to a
log file/variable instead, and if suppression is intentionally desired add a
clarifying comment explaining why warnings are ignored; reference the TAR_CMD
invocation ("${TAR_CMD[@]}") and ensure the change preserves the intended
behavior for noisy but non-fatal warnings or surfaces fatal errors for
debugging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
Does the bsdtar have the same behaviour as
--no-anchored --wildcards? If it doesn't then we're creating different behaviour on linux and mac that can lead to errors depending on the user's input to the task. -
If 1 is not an issue, then we should try to build the archiver command up as a variable like it was before since it helps make the script more readable/modular/etc.
-
Ideally resolve any/all coderabbit comments before the review (unless you want the reviewers help/opinion) and request a review on github for tracking (e.g. so Bill and I don't overlap reviewing this).
-
For the coderabbit comment, I'm a bit indifferent. I'd be fine with moving away from the original behaviour and printing the tar output. The extra output could help in debugging missing files and task's output is already quite verbose so I wouldn't be that worried about making it more messy (we should consult with Kirk and Bill if we want to switch this though).
Description
This PR adds automatic fallback support for macOS BSD tar when GNU tar (
gtar) is not available.The build system currently assumes GNU tar is available on macOS, which causes build failures with "command not found: gtar" errors. This change:
gtaris available at runtimetar(BSD tar on macOS) ifgtaris not found--no-anchored,--wildcards) only when usinggtarThis enables developers to build on macOS using only Homebrew-installed CMake and the system-provided BSD tar, without needing to install GNU tar separately.
Checklist
Validation performed
gtarinstalled)task deps:install-allSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.