bug: crash when a requirement is skipped by an environment marker#375
bug: crash when a requirement is skipped by an environment marker#375vbelouso wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces support for filtering out requirements with inactive environment markers during parsing, while still validating all requirements for cross-platform reproducibility. It also updates the compilation script to handle cases where the resolver is null (e.g., when all dependencies are filtered out), and adds corresponding unit and integration tests. Feedback on the tests highlights two main issues: first, using 'sys_platform == 'win32'' as an inactive marker makes tests platform-dependent and prone to failure on Windows, so a guaranteed inactive marker should be used instead; second, using 'traceback.print_tb(result.exc_info[2])' in test assertions can raise a TypeError if 'exc_info' is null and will result in an empty assertion message, so asserting on 'result.exit_code' with 'result.output' is recommended.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| requirements_path: Path = tmp_path / "requirements.txt" | ||
| requirements_path.write_text( | ||
| "cryptography==40.0.0 ; python_version >= '3.0'\n" | ||
| "pywin32==311 ; sys_platform == 'win32'\n" |
There was a problem hiding this comment.
Using sys_platform == 'win32' as an inactive marker makes this test platform-dependent. On Windows, this marker will evaluate to True, causing the assertion assert 'pywin32' not in names and the length assertion to fail. Use a guaranteed inactive marker like sys_platform == 'impossible_platform' to make the test robust across all platforms.
| "pywin32==311 ; sys_platform == 'win32'\n" | |
| "pywin32==311 ; sys_platform == 'impossible_platform'\n" |
| """Test compile with --generate-hashes and inactive markers.""" | ||
| output = tmp_path / "requirements-build.txt" | ||
| requirements_path: Path = tmp_path / "requirements.txt" | ||
| requirements_path.write_text("pywin32==311 ; sys_platform == 'win32'") |
There was a problem hiding this comment.
Using sys_platform == 'win32' as an inactive marker makes this test platform-dependent. On Windows, this marker evaluates to True, so the requirement will be active and the test will fail. Use a guaranteed inactive marker like sys_platform == 'impossible_platform' to ensure the test passes on all operating systems.
| requirements_path.write_text("pywin32==311 ; sys_platform == 'win32'") | |
| requirements_path.write_text("pywin32==311 ; sys_platform == 'impossible_platform'") |
| result = runner.invoke( | ||
| main.cli, args=["compile", str(requirements_path), "-o", str(output)] | ||
| ) | ||
| assert result.exit_code == 0, traceback.print_tb(result.exc_info[2]) |
There was a problem hiding this comment.
Using traceback.print_tb(result.exc_info[2]) in the assertion message has two issues:\n1. If result.exc_info is None (e.g., when the command exits with a non-zero code without raising an unhandled exception), accessing result.exc_info[2] will raise a TypeError, masking the actual assertion failure.\n2. traceback.print_tb prints to stdout and returns None, so the assertion message itself will be empty.\n\nInstead, assert on result.exit_code and provide result.output as the assertion message, which contains the captured stdout/stderr (including any traceback if an exception occurred).
| assert result.exit_code == 0, traceback.print_tb(result.exc_info[2]) | |
| assert result.exit_code == 0, result.output |
Signed-off-by: Vladimir Belousov <vbelouso@redhat.com>
ddd8cee to
883604b
Compare
|
@eskultety can you please take a look? |
Hi,
I ran into a crash in
pybuild-depswhen a requirements file contains a dependency that should be ignored on the current platform because of an environment marker.Environment:
pybuild-deps: 0.5.0I think this affects non-Windows platforms in general, not just my Linux env.
Minimal repro:
Expected:
On Linux,
sys_platform == 'win32'is false, so this requirement should be ignored. My expectation was that the command exits cleanly and produces an empty output file, basically the same behavior aspip/pip-compile.Actual:
Instead it crashes with:
With
--generate-hashes, it crashes in a similar way:Analysis:
From looking at the code, it seems like inactive marker requirements are still passed further into the resolver. In this case there are no active dependencies to resolve, so the resolver loop does not run and
compiler.resolverstaysNone. Latercompile.pyassumes the resolver exists and tries to accessunsafe_packagesorresolve_hashes.Fix:
The fix I used is:
parsers/requirements.pyscripts/compile.pyfor the empty dependency casecompiler.resolveris always setDesign note:
One small design point: validation still happens before marker filtering. So this still fails, even on Linux:
because `foo>1` is not pinned exactly. I think this behavior is reasonable for this tool, since it catches invalid requirements even if they are only active on another platform.
Refs: