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.

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

Windows pyproject.toml cannot handle versioned dependencies #392

Closed
jbarlow83 opened this issue Jul 4, 2020 · 6 comments
Closed

Windows pyproject.toml cannot handle versioned dependencies #392

jbarlow83 opened this issue Jul 4, 2020 · 6 comments

Comments

@jbarlow83
Copy link
Contributor

If pyproject.toml specifies a versioned dependency, such as...

[build-system]
requires = [
  "setuptools >= 30.3.0",
]
build-backend = "setuptools.build_meta"

then in windows.py, the code current parses the requirements and calls...

return subprocess.check_call(' '.join(args), env=env, cwd=cwd, shell=True)

which means that the result of ' '.join(args) is...

pip install setuptools >= 30.3.0

which breaks the shell parser since that needs to be quoted to be not be a redirect.

shell=True is usually bad mojo. It should work to do subprocess.check_call(args, env=env, cwd=cwd).

@Czaki
Copy link
Contributor

Czaki commented Jul 4, 2020

shell=True is not good, but if I good remember without this environment is not passed on windows (something is not working, we try to remove this). So I do not see option to workaround this.

This problem which you report is only connected with python 3.5

https://github.com/joerick/cibuildwheel/blob/db9d658b51fe08529a6d5cbef65fc42deb8ca0aa/cibuildwheel/windows.py#L153-L175

@joerick @YannickJadoul
What did you think. Possible workaround is to add following line:

requirements = [x.replace(" ", "") for x in requirements]

@joerick
Copy link
Contributor

joerick commented Jul 4, 2020

shell=True is usually bad mojo. It should work to do subprocess.check_call(args, env=env, cwd=cwd).

hm. actually I think the reason for the ' '.join() is historical - we used to be calling the Windows commands using run_with_env.cmd, which was some magic from multibuild that set up MSVC environment variables for different Python versions. But it ran all subcommands inside a shell, necessitating collapsing args into a string (in hindsight, we should have used list2cmdline for this).

We don't use that any more, so moving to a call() based approach would match the other platforms and would fix bugs like the above. That would be my preferred route, I think!

I've got my hands full with #386 right now, so anyone else is interesting in writing a PR, that would be cool!

@Czaki
Copy link
Contributor

Czaki commented Jul 4, 2020

I;m pretty sure that I try remove shell=True in some PR and it fail.

@YannickJadoul
Copy link
Member

What did you think. Possible workaround is to add following line:

requirements = [x.replace(" ", "") for x in requirements]

If we're going for a workaround, would surrounding each of the requirements not be better?

@jbarlow83
Copy link
Contributor Author

I think you need to quote each requirement to get Windows to cooperate, not substitute spaces: ['"%s"' for x in requirements].

@joerick
Copy link
Contributor

joerick commented Jul 8, 2020

Fixed in the latest release. Thanks all!

@joerick joerick closed this as completed Jul 8, 2020
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 a pull request may close this issue.

4 participants