-
Notifications
You must be signed in to change notification settings - Fork 21
Don't raise an error if there are spaces in file paths supplied to PIP_CONSTRAINT
#210
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
base: main
Are you sure you want to change the base?
Conversation
a1be3d6
to
80b6791
Compare
Would it be alright to release 0.30.5 with #209 and this PR? |
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.
Thanks! I left a minor comment.
Suggested-by: Gyeongjae Choi <def6488@gmail.com>
I'm a little confused. If somebody outside pyodide_build is setting the environment variable PIP_CONSTRAINT to It looks to me that pyodide-build will take the above and transform it into I'm not sure why pyodide-build needs to do any transformation to the value of PIP_CONSTRAINT. Is there a version of this where pyodide-build just leaves the value alone? |
Thanks for your thoughts, @joerick – this was also my source of confusion; I thought that we were just wanting to accept spaces with file paths to accommodate I cannot think of a neat way to allow multiple constraints files with So, would I be right here if I say that the implementation should be:
with the assumption that our intention with this PR is to undo https://github.com/pypa/cibuildwheel/blob/e92147f52a3f32818588e968f4be9315c8c35635/test/test_environment.py#L103-L108? |
PIP_CONSTRAINT
PIP_CONSTRAINT
With the above (reverted) commits and changes, we no longer raise an error when there are spaces in the file path, and rather add a message about this |
The CI failure looks unrelated; I'll debug it. Edit: it is unrelated – I can see it locally after a refresh of my dependencies 😕 Edit 2: something must have changed and Rich/Click/Typer now sends |
"message if you are using multiple constraints files." | ||
) | ||
|
||
constraints_file = Path(constraints) |
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.
I suppose you'd have to split and take the first entry if you're keeping the following code? constraints
might contain two files, so Path(constraints)
wouldn't refer to a file.
Huh, I didn't know PIP_CONSTRAINT accepts multiple constraint files. Then I am leaning towards following joerick's suggestion: allow spaces in the PIP_CONSTRAINT variable, but consider them as separate files. Actually, in that case, we can remove some logic in pyodide-build. We have a logic to merge the user-provided PIP_CONSTRAINT with the ones provided by pyodide-build. But instead, we can just append the pyodide-build-provided constraints into PIP_CONSTRAINT. |
Description
In the file
pyodide-build/pyodide_build/build_env.py
Line 339 in 7126c5f
RecipeBuilder
class, we were checking if thePIP_CONSTRAINT
supplied contains spaces and we were raising aValueError
, and this came up in pypa/cibuildwheel#2002 (comment). The issue exists withpip
: pypa/pip#10114; however, there is a reasonable workaround – we can use a URI in order to not let a space in the file path be interpreted as a separate file.cc: @joerick @hoodmane