Skip to content

Conversation

agriyakhetarpal
Copy link
Member

@agriyakhetarpal agriyakhetarpal commented May 23, 2025

Description

In the file

"PIP_CONSTRAINT contains spaces so pip will misinterpret it. Make sure the path to pyodide has no spaces.\n"
and in the RecipeBuilder class, we were checking if the PIP_CONSTRAINT supplied contains spaces and we were raising a ValueError, and this came up in pypa/cibuildwheel#2002 (comment). The issue exists with pip: 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

@agriyakhetarpal agriyakhetarpal force-pushed the allow-multiple-pip-constraint-files branch from a1be3d6 to 80b6791 Compare May 23, 2025 03:03
@agriyakhetarpal
Copy link
Member Author

Would it be alright to release 0.30.5 with #209 and this PR?

@agriyakhetarpal agriyakhetarpal requested a review from hoodmane May 23, 2025 03:05
Copy link
Member

@ryanking13 ryanking13 left a 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.

@joerick
Copy link

joerick commented May 23, 2025

I'm a little confused. If somebody outside pyodide_build is setting the environment variable PIP_CONSTRAINT to /path to file/with spaces.txt, then that's a misconfiguration - pip would fail to understand this.

It looks to me that pyodide-build will take the above and transform it into file:///path%20to%20file/with%20spaces.txt. That's potentially helpful(?), but also that means that users can't pass multiple files like file1.txt file2.txt, because pyodide-build would transform it into file://file1.txt%20file2.txt.

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?

@agriyakhetarpal
Copy link
Member Author

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 cibuildwheel, but on re-reading combine_constraints(), it looks like it is combining cibuildwheel's constraints with the user's constraints, and the fact that pyodide-build is not allowing constraints with spaces is part of the problem.

I cannot think of a neat way to allow multiple constraints files with PIP_CONSTRAINT and still allow a file with spaces to be interpreted correctly, so I'll go ahead and remove the workaround from here.

So, would I be right here if I say that the implementation should be:

  1. If a user supplies a file via PIP_CONSTRAINT to pyodide-build and there are no spaces, we proceed in the normal fashion, and cibuildwheel can combine/extend it with its constraints if it wants to
  2. If a user wants to supply multiple constraints files to pyodide-build, they use PIP_CONSTRAINT="my_constraints1.txt my_constraints2.txt" as usual and pip interprets them as multiple files
  3. If a user wants to supply a constraint file to pyodide-build whose name/path contains spaces, they are the ones to implement and use the URI workaround themselves, and we shouldn't operate on the PIP_CONSTRAINT file ourselves. cibuildwheel can then extend the PIP_CONSTRAINT it receives from pyodide-build correctly as it will have a file:// in the string, and combine its own constraints without issues.

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?

@agriyakhetarpal agriyakhetarpal changed the title Work around spaces in file paths supplied to PIP_CONSTRAINT Don't raise an error if there are spaces in file paths supplied to PIP_CONSTRAINT May 27, 2025
@agriyakhetarpal
Copy link
Member Author

agriyakhetarpal commented May 27, 2025

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 PIP_CONSTRAINTS problem to keep them informed. I still don't think we as downstream users should be responsible for the pip bug, but then anything we can do to help our users is always helpful.

@agriyakhetarpal
Copy link
Member Author

agriyakhetarpal commented May 27, 2025

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 logger.error() messages to stderr instead of stdout – I've fixed the failing test.

"message if you are using multiple constraints files."
)

constraints_file = Path(constraints)
Copy link

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.

@ryanking13
Copy link
Member

ryanking13 commented May 28, 2025

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants