Add regression test for install_cargo_config PermissionError path#312
Open
dwoz wants to merge 2 commits into
Open
Add regression test for install_cargo_config PermissionError path#312dwoz wants to merge 2 commits into
dwoz wants to merge 2 commits into
Conversation
install_cargo_config catches PermissionError from get_toolchain — that error surfaces on non-root installs where the process can't create DATA_DIR (e.g. salt-master running as the salt user against a /opt/saltstack/salt install). A prior refactor lost the `toolchain = None` initializer before the try/except, so the subsequent `if not toolchain:` check fired UnboundLocalError instead of degrading quietly. Commit d0df0af ("Fix missing toolchain variable") restored the initializer but landed without a test to keep future refactors honest. Add a stubbed test that forces get_toolchain to raise PermissionError and asserts install_cargo_config returns cleanly without writing a cargo config. Verified the test fails on the pre-d0df0af code with the exact error from the reporter's traceback (#252 (comment)), and passes on current main.
pyzmq 26.2.0's pyproject.toml uses `cmake.targets = ["pyzmq"]`, which
scikit-build-core renamed to `build.targets` starting in 1.0.0 (released
2026-07-06). pyzmq 26.2.0 leaves scikit-build-core unpinned in
build-system.requires, so `pip install pyzmq==26.2.0` on any host that
resolves scikit-build-core >= 1.0.0 aborts at "Getting requirements to
build wheel" with:
ERROR: Use build.targets instead of cmake.targets for
scikit-build-core >= 0.10
That break landed on 2026-07-06 and broke every "Verify Linux" matrix
entry on the next CI run (2026-07-07). macOS and Windows already xfail
this parametrization for unrelated pre-existing reasons, which is why
only the Linux jobs turned red. pyzmq 26.4.0 already uses
`build.targets`, so leaving the other three parametrizations green
preserves the coverage the test was providing.
Match the shape of the surrounding xfail block and pin the reason to
the upstream cause so a follow-up bump (pyzmq >= 26.4 in the parametrize
list) can drop this guard cleanly.
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.
install_cargo_config catches PermissionError from get_toolchain — that error surfaces on non-root installs where the process can't create DATA_DIR (e.g. salt-master running as the salt user against a /opt/saltstack/salt install). A prior refactor lost the
toolchain = Noneinitializer before the try/except, so the subsequentif not toolchain:check fired UnboundLocalError instead of degrading quietly. Commit d0df0af ("Fix missing toolchain variable") restored the initializer but landed without a test to keep future refactors honest.Add a stubbed test that forces get_toolchain to raise PermissionError and asserts install_cargo_config returns cleanly without writing a cargo config. Verified the test fails on the pre-d0df0af code with the exact error from the reporter's traceback
(#252 (comment)), and passes on current main.