-
Notifications
You must be signed in to change notification settings - Fork 12
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
feat: Support multiple constraints files #540
base: main
Are you sure you want to change the base?
Conversation
Fromager now supports multiple constraints files. The option `fromager --constraints-file` can be supplied multiple times. The option no longer accepts URLs. Remote constraints files are now specified with `--constraints-url`. The split was necessary to correctly parse environment variables with multiple entries and white spaces in local file names. Paths are split at `:`, URLs are split at ` ` (white space). There can only be one constraint per package. Internally, remote constraints from `https://` URLs are no longer retrieved twice. Instead all constraints are merged and the final set is dumped into a single constraints file. Fixes: python-wheel-build#539 Signed-off-by: Christian Heimes <[email protected]>
481ab95
to
c295a82
Compare
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.
The logic all looks good. I had a couple of comments, but nothing that has to hold this up.
logger.info(f"network isolation: {network_isolation}") | ||
overrides.log_overrides() | ||
|
||
if network_isolation and not SUPPORTS_NETWORK_ISOLATION: | ||
ctx.fail(f"network isolation is not available: {NETWORK_ISOLATION_ERROR}") | ||
|
||
constraints: list[str] = list(constraints_url) | ||
constraints.extend(os.fspath(p) for p in constraints_file) |
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.
This implies that constraints at URLs take precedence over local files. I don't think that ordering is wrong, but I can definitely imagine wanting a local file to have a value that replaces a value in some global file on a web server.
"""Dump all constraints into a file""" | ||
with constraints_file.open("w", encoding="utf-8") as f: | ||
for req in self._data.values(): | ||
f.write(f"{req}\n") |
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.
Do we log the constraints as we parse the input? I know we log matching constraints as we process individual packages. It may also be useful to log the contents of this file to help with debugging.
Fromager now supports multiple constraints files. The option
(white space).
fromager --constraints-file
can be supplied multiple times. The option no longer accepts URLs. Remote constraints files are now specified with--constraints-url
. The split was necessary to correctly parse environment variables with multiple entries and white spaces in local file names. Paths are split at:
, URLs are split atThere can only be one constraint per package.
Internally, remote constraints from
https://
URLs are no longer retrieved twice. Instead all constraints are merged and the final set is dumped into a single constraints file.Fixes: #539