fix(ssh): support macOS tar during file sync - #26323
Conversation
|
Thanks for linking these. Yes, #26323 is a cleaned-up resubmission of the earlier accidentally-routed #26154. I agree this overlaps with #13955 around the macOS bsdtar/root-cause area. The approach here is intentionally narrower: preserve the existing GNU tar Happy to close this in favor of #13955 if maintainers prefer that direction, or rework this PR into supplemental regression coverage for the bsdtar case. |
d4e1846 to
e4ee02d
Compare
|
Moving this back to Draft while maintainers decide the preferred direction. I re-checked the related PRs: #13955 and #10205 are both closed and not merged, so this is not currently superseded by an already-landed fix. That said, this PR still overlaps with #13955 around the macOS bsdtar/root-cause area. The intent here remains the narrower fallback fix: preserve the existing GNU tar behavior where supported, and only fall back when the remote tar rejects |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the focused compatibility fix. The current main path still unconditionally sends the GNU-only flag at tools/environments/ssh.py:253, so the reported bsdtar failure remains relevant.
Problems
- The new probe invoked from
tools/environments/ssh.py:258adds a secondsubprocess.runto first bulk upload. Existing tests assert exactly one mkdir call attests/tools/test_ssh_bulk_upload.py:79-85,:342-348, and:388-394; their initialized environments leave the new cache unset, so the probe executes and those assertions fail.
Suggested changes
- Stub
_remote_tar_supports_no_overwrite_dir()in the shared bulk-upload fixture, or make those tests assert the mkdir command specifically rather than the total run-call count. Preserve the new direct probe tests for cache and failure coverage.
Automated hermes-sweeper review.
| quoted_base = shlex.quote(base) | ||
| tar_extract_cmd = f"tar xf - --no-overwrite-dir -C {quoted_base}" | ||
| if not self._remote_tar_supports_no_overwrite_dir(): | ||
| # bsdtar on macOS also tries to restore mtimes on existing |
There was a problem hiding this comment.
This first-use probe adds a second subprocess.run after the mkdir call. Existing bulk-upload tests assert one run call for mkdir (tests/tools/test_ssh_bulk_upload.py:79-85, :342-348, :388-394), so stub this method in their shared fixture or revise those assertions to select the mkdir invocation.
e4ee02d to
2a4f7dd
Compare
|
M28.2 refresh evidence for PR #26323: Verdict: GO-for-review Verification run in clean worktree
Mergeability snapshot after push: |
2a4f7dd to
6e7eef3
Compare
|
M28.2 update after supervisor review: Verdict: GO-for-review Additional fix since the prior comment: the tar stream now archives only staged file entries, not the staging root Verification rerun in clean worktree
Mergeability snapshot after push: |
|
M28.2 independent ccreviewer evidence for PR #26323: Verdict: GO Reviewed:
Verified:
Findings:
Blockers/Risks: none found in the reviewed scope. Live SSH integration was not exercised because credentials/env were unavailable; local targeted coverage and merge-tree checks passed. |
|
Maintainer review requested when convenient. GitHub currently reports this focused macOS/bsdtar compatibility fix as mergeable; the branch remains at the independently reviewed head |
Summary
tarsupports GNU--no-overwrite-dirbefore SSH bulk upload extractiontar xmf - -C /when the GNU flag is unavailableTest Plan
python -m pytest tests/tools/test_ssh_environment.py::TestSSHBulkUploadTarCompatibility -q -o 'addopts='python -m pytest tests/tools/test_ssh_environment.py -q -o 'addopts='python -m py_compile tools/environments/ssh.py tests/tools/test_ssh_environment.pygit diff --check origin/main..HEADNote: the full SSH environment test file passes with existing pytest thread warnings from older mocked process tests; no new failures were observed.
Review Focus
tar xmf - -C /is the right portable fallback for bsdtar/macOS targets