feat: implement multiple constraint files#1129
feat: implement multiple constraint files#1129tiran wants to merge 1 commit intopython-wheel-build:mainfrom
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughThis PR refactors the constraints system to support multiple constraint files with automatic merging. The Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/how-tos/bootstrap-constraints.rst`:
- Around line 47-49: The phrase "For examples" in the sentence starting with
"For examples ``egg>=1.0`` and ``egg!=1.1.2``..." is a typo; replace "For
examples" with "For example" so the sentence reads "For example ``egg>=1.0`` and
``egg!=1.1.2`` are combined into ``egg>=1.0,!=1.1.2``." Locate and edit that
exact string in the document.
In `@src/fromager/constraints.py`:
- Around line 83-88: Add a proper docstring to the public method
dump_constraints describing its behavior and parameters (e.g., that it writes
sorted constraints without markers to the given TextIO), promoting the existing
inline comment into the docstring; update the function signature for
dump_constraints(self, output: typing.TextIO) to include the docstring at the
top of the function body, mentioning that constraints are sorted by normalized
name and that markers were evaluated in add_constraint(), and leave the
implementation (iterating over self._data and writing
f"{req.name}{req.specifier}\n") unchanged.
In `@tests/test_cli.py`:
- Around line 99-121: The test leaks state because it doesn't pass
-O/--output-dir to the CLI and relies on log output; modify
test_multiple_constraints_files to add "-O", str(tmp_path) (or "--output-dir",
str(tmp_path)) to the cli_runner.invoke args so WorkContext.setup() uses the
temp dir, then after invoking fromager read the merged constraints file that the
CLI writes into that output dir (the merged constraints artifact created by the
command) and assert its contents contain "foo==1.0" and the merged constraint
"bar!=2.1.1,>=2.0" instead of checking result.output; reference
WorkContext.setup(), the CLI option -O/--output-dir, and
test_multiple_constraints_files to locate where to change the test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 502cbff3-998c-4443-83a5-48685ffcd411
📒 Files selected for processing (10)
docs/how-tos/bootstrap-constraints.rstdocs/spelling_wordlist.txtsrc/fromager/__main__.pysrc/fromager/constraints.pysrc/fromager/context.pytests/conftest.pytests/test_cli.pytests/test_constraints.pytests/test_context.pytests/test_cooldown.py
💤 Files with no reviewable changes (2)
- tests/conftest.py
- tests/test_cooldown.py
| occurrences of the same package are merged and validated. For examples | ||
| ``egg>=1.0`` and ``egg!=1.1.2`` are combined into ``egg>=1.0,!=1.1.2``. An | ||
| unsatisfiable combination like ``egg<1.0`` and ``egg>2.0`` is an error. |
There was a problem hiding this comment.
Typo: "For examples" → "For example".
✏️ Proposed fix
-occurrences of the same package are merged and validated. For examples
+occurrences of the same package are merged and validated. For example,
``egg>=1.0`` and ``egg!=1.1.2`` are combined into ``egg>=1.0,!=1.1.2``. An📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| occurrences of the same package are merged and validated. For examples | |
| ``egg>=1.0`` and ``egg!=1.1.2`` are combined into ``egg>=1.0,!=1.1.2``. An | |
| unsatisfiable combination like ``egg<1.0`` and ``egg>2.0`` is an error. | |
| occurrences of the same package are merged and validated. For example, | |
| ``egg>=1.0`` and ``egg!=1.1.2`` are combined into ``egg>=1.0,!=1.1.2``. An | |
| unsatisfiable combination like ``egg<1.0`` and ``egg>2.0`` is an error. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/how-tos/bootstrap-constraints.rst` around lines 47 - 49, The phrase "For
examples" in the sentence starting with "For examples ``egg>=1.0`` and
``egg!=1.1.2``..." is a typo; replace "For examples" with "For example" so the
sentence reads "For example ``egg>=1.0`` and ``egg!=1.1.2`` are combined into
``egg>=1.0,!=1.1.2``." Locate and edit that exact string in the document.
| def test_multiple_constraints_files( | ||
| tmp_path: pathlib.Path, cli_runner: CliRunner | ||
| ) -> None: | ||
| constraints1 = tmp_path / "constraints1.txt" | ||
| constraints1.write_text("foo==1.0\nbar!=2.1.1\n") | ||
| constraints2 = tmp_path / "constraints2.txt" | ||
| constraints2.write_text("bar>=2.0\n") | ||
|
|
||
| result = cli_runner.invoke( | ||
| fromager, | ||
| [ | ||
| "--verbose", | ||
| "-c", | ||
| str(constraints1), | ||
| "--constraints-file", | ||
| str(constraints2), | ||
| "lint", | ||
| ], | ||
| ) | ||
| assert result.exit_code == 0, result.output | ||
| assert "foo==1.0" in result.output | ||
| assert "bar!=2.1.1,>=2.0" in result.output | ||
|
|
There was a problem hiding this comment.
Two issues: missing --output-dir and log-coupled assertions.
-
Test isolation: Without
-O/--output-dir,WorkContext.setup()createswork-dir/in the current working directory, leaking filesystem state across test runs. -
Brittle output assertions:
"foo==1.0" in result.outputand"bar!=2.1.1,>=2.0" in result.outputwork only because--verbosesurfaces thelogger.debug(...)that embeds the merged file content. Per project learnings, prefer asserting functional artifacts over log strings.
Fix both by passing -O str(tmp_path) and reading the merged constraints file directly:
🛠️ Proposed fix
def test_multiple_constraints_files(
tmp_path: pathlib.Path, cli_runner: CliRunner
) -> None:
constraints1 = tmp_path / "constraints1.txt"
constraints1.write_text("foo==1.0\nbar!=2.1.1\n")
constraints2 = tmp_path / "constraints2.txt"
constraints2.write_text("bar>=2.0\n")
result = cli_runner.invoke(
fromager,
[
- "--verbose",
+ "-O",
+ str(tmp_path / "output"),
"-c",
str(constraints1),
"--constraints-file",
str(constraints2),
"lint",
],
)
assert result.exit_code == 0, result.output
- assert "foo==1.0" in result.output
- assert "bar!=2.1.1,>=2.0" in result.output
+ merged = tmp_path / "output" / "work-dir" / "merged-constraints.txt"
+ content = merged.read_text()
+ assert "foo==1.0" in content
+ assert "bar!=2.1.1,>=2.0" in content📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_multiple_constraints_files( | |
| tmp_path: pathlib.Path, cli_runner: CliRunner | |
| ) -> None: | |
| constraints1 = tmp_path / "constraints1.txt" | |
| constraints1.write_text("foo==1.0\nbar!=2.1.1\n") | |
| constraints2 = tmp_path / "constraints2.txt" | |
| constraints2.write_text("bar>=2.0\n") | |
| result = cli_runner.invoke( | |
| fromager, | |
| [ | |
| "--verbose", | |
| "-c", | |
| str(constraints1), | |
| "--constraints-file", | |
| str(constraints2), | |
| "lint", | |
| ], | |
| ) | |
| assert result.exit_code == 0, result.output | |
| assert "foo==1.0" in result.output | |
| assert "bar!=2.1.1,>=2.0" in result.output | |
| def test_multiple_constraints_files( | |
| tmp_path: pathlib.Path, cli_runner: CliRunner | |
| ) -> None: | |
| constraints1 = tmp_path / "constraints1.txt" | |
| constraints1.write_text("foo==1.0\nbar!=2.1.1\n") | |
| constraints2 = tmp_path / "constraints2.txt" | |
| constraints2.write_text("bar>=2.0\n") | |
| result = cli_runner.invoke( | |
| fromager, | |
| [ | |
| "-O", | |
| str(tmp_path / "output"), | |
| "-c", | |
| str(constraints1), | |
| "--constraints-file", | |
| str(constraints2), | |
| "lint", | |
| ], | |
| ) | |
| assert result.exit_code == 0, result.output | |
| merged = tmp_path / "output" / "work-dir" / "merged-constraints.txt" | |
| content = merged.read_text() | |
| assert "foo==1.0" in content | |
| assert "bar!=2.1.1,>=2.0" in content |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_cli.py` around lines 99 - 121, The test leaks state because it
doesn't pass -O/--output-dir to the CLI and relies on log output; modify
test_multiple_constraints_files to add "-O", str(tmp_path) (or "--output-dir",
str(tmp_path)) to the cli_runner.invoke args so WorkContext.setup() uses the
temp dir, then after invoking fromager read the merged constraints file that the
CLI writes into that output dir (the merged constraints artifact created by the
command) and assert its contents contain "foo==1.0" and the merged constraint
"bar!=2.1.1,>=2.0" instead of checking result.output; reference
WorkContext.setup(), the CLI option -O/--output-dir, and
test_multiple_constraints_files to locate where to change the test.
Fromager now supports multiple constraints files with `-c` /
``--constraints-file`` argument. Multiple constraints are merged and
validated.
Example:
```console
$ fromager \
-c constraints.txt \
-c local-constraints.txt \
-c https://company.example/security-constraints.txt \
bootstrap ...
```
Local and remote constraints are loaded in `WorkContext.setup()` and
dumped into a new file `merged-constraints.txt` in `work-dir`. Some
internals of `WorkContext` have changed in an API-incompatible way:
- `constraints_file` argument is now
`constraints_files: tuple[str, ...] = ()`
- `WorkContext` now only accepts keyword arguments
- `input_constraints_uri` is replaced by `input_constraints_files`
Fixes: python-wheel-build#1096
Signed-off-by: Christian Heimes <cheimes@redhat.com>
tiran
force-pushed
the
multiple-constraint-files
branch
from
f1e461d to
6c779c5
Compare
| text="remote>=1.0\n", | ||
| ) | ||
| c.load_constraints_file(url) | ||
| assert c.get_constraint("remote") == Requirement("remote>1.0") |
There was a problem hiding this comment.
Typo: assertion uses remote>1.0 but the mocked response is "remote>=1.0\n". Should be Requirement("remote>=1.0").
3 participants
Pull Request Description
What
Fromager now supports multiple constraints files with
-c/--constraints-fileargument. Multiple constraints are merged and validated.Example:
Local and remote constraints are loaded in
WorkContext.setup()anddumped into a new file
merged-constraints.txtinwork-dir. Someinternals of
WorkContexthave changed in an API-incompatible way:constraints_fileargument is nowconstraints_files: tuple[str, ...] = ()WorkContextnow only accepts keyword argumentsinput_constraints_uriis replaced byinput_constraints_filesWhy
Fixes: #1096