Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix: Support environment markers in PEP 518 workaround #398

Closed
wants to merge 5 commits into from
Closed

fix: Support environment markers in PEP 518 workaround #398

wants to merge 5 commits into from

Conversation

henryiii
Copy link
Contributor

@henryiii henryiii commented Jul 6, 2020 •
edited
Loading

The original version didn't handle environment markers, making the common idiom for supporting all NumPy versions:

requires = [
    "numpy==1.13.3; python_version<='3.6'",
    "numpy==1.14.5; python_version=='3.7'",
    "numpy==1.17.3; python_version>='3.8'",
    ...

fail. Will fix #392.

@henryiii
Copy link
Contributor Author

henryiii commented Jul 6, 2020

Assuming the tests pass, how hard would it be to push it into a 1.5.2 release?

@henryiii henryiii mentioned this pull request Jul 6, 2020
Merged
@henryiii henryiii marked this pull request as draft July 6, 2020 05:30
@YannickJadoul
Copy link
Member

What about just [f'"r"' for r in requirements] (as suggested by #392 (comment)) ?

@henryiii
Copy link
Contributor Author

henryiii commented Jul 6, 2020 •
edited
Loading

I'll try that to see if it works, but then I'll take a stab at removing the shell + join. Really this should have worked in the original form, it just is tripping up because of the join hack, IIUC, so it's worth testing to see if that's easy to remove.

@YannickJadoul
Copy link
Member

Great, that would be even better! Thanks for taking up this annoying job! :-)

@Czaki
Copy link
Contributor

Czaki commented Jul 6, 2020

I try some time ago to remove shell=True bui it is forced by bug in python https://bugs.python.org/issue8557
If I good understand it can be changed after drop of python 3.6 support by cibuildwheel

@henryiii henryiii mentioned this pull request Jul 6, 2020
Merged
@henryiii
Copy link
Contributor Author

henryiii commented Jul 6, 2020

I think that says it's still affecting at least Python 3.7, and it's marked as open and languishing so I bet it is true in 3.8 and 3.9. I'm playing around with it, and will see if I can come up with a solution. Using the correct environment is important, so if that's really completely missing when shell=False, then the solution above is probably the only reasonable one.

YannickJadoul
YannickJadoul reviewed Jul 6, 2020
View reviewed changes
Copy link
Member

@YannickJadoul YannickJadoul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, things are green! :-)

cibuildwheel/windows.py
@@ -172,7 +172,8 @@ def pep_518_cp35_workaround(package_dir: Path, env: Dict[str, str]) -> None:
else []
)
if requirements:
shell(['pip', 'install'] + requirements, env=env)
escaped_requirements = [f'"{s}"' for s in requirements]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking out loud: is this something we should do here, or more broadly inside of shell?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't have any protection for quotes inside the string, so I was a little hesitant to add it higher up. If someone wrote "requests==2.22.0; python_version<\"3.6\"",, in fact, this might not work.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I suppose we also control most of the other things to not contain spaces?
Though .. what about spaces in CIBW_TEST_EXTRAS or so?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That can be changed by the cibw user, if something odd happens. You can't change pyproject.toml (at least you shouldn't in CI).

@henryiii
Copy link
Contributor Author

henryiii commented Jul 6, 2020

PermissionError: [WinError 5] Access is denied: 'C:\Users\travis\AppData\Local\Temp\tmpusgvk8k0\Scripts\nosetests.exe'

Don't think this is related, only on Travis.

@henryiii henryiii marked this pull request as ready for review July 6, 2020 20:13
YannickJadoul
YannickJadoul approved these changes Jul 6, 2020
View reviewed changes
Copy link
Member

@YannickJadoul YannickJadoul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Restarted the one Travis CI failure.

Would be great if #399 would work, but meanwhile, I think this can go in, @joerick?
Or do we need to perform a more extensive search of where the same/a similar issue can cause problems?

@henryiii
Copy link
Contributor Author

henryiii commented Jul 7, 2020

I think this would be a good candidate for 1.5.2, but #399 would be more of a candidate for 1.6...

@henryiii henryiii closed this Jul 8, 2020
@ajfriend ajfriend mentioned this pull request Jul 15, 2020
Merged
@henryiii henryiii deleted the fix/identifiers branch June 6, 2024 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@YannickJadoul YannickJadoul YannickJadoul approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Windows pyproject.toml cannot handle versioned dependencies
3 participants
@henryiii @YannickJadoul @Czaki