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

Move setup_requires to pyproject.yaml #14

Closed
jonathanunderwood opened this issue Apr 4, 2019 · 16 comments · Fixed by #17
Closed

Move setup_requires to pyproject.yaml #14

jonathanunderwood opened this issue Apr 4, 2019 · 16 comments · Fixed by #17

Comments

@jonathanunderwood
Copy link

When build system (setuptools) requirements are specified in setup.py, they end up being installed by distutils, even when pip installing. Because distutils is bit-rotting, it doesn't work with system installed openssl.

Locally, for me, that means distutils doesn't know about some SSL CA certs, and as such, a pip install markdown-full-yaml-metadata will fail when trying to install setuptools_markdown due to being unable to validate the SSL cert (I am behind a coorporate proxy which MITMs all traffic and resigns with a local cert).

Moving the setup_requires deps to pyproject.toml fixes this - I suggest something like this:

[build-system]
# Minimum requirements for the build system to execute.
requires = ["setuptools>=36.6", "setuptools_markdown", "wheel",]
build-backend = "setuptools.build_meta"
@sivakov512
Copy link
Owner

Looks like setuptools_markdown is not required for installation - some time ago I started using long_description_content_type="text/markdown", but forgot to remove setup_requires.
Will your problem be fixed if I remove this dep?

@jonathanunderwood
Copy link
Author

That'd also fix it, yes :)

@sivakov512
Copy link
Owner

I think you can create a PR for #13 and fix this problem too. Are you ok with this?

@jonathanunderwood
Copy link
Author

Unfortunately, this doesn't quite fix things, as setup.py still has a setup_requires entry, for pytest-runner now. To fix this, I suggest adding the following to pyproject.toml:

[build-system]
# Minimum requirements for the build system to execute.
requires = ["setuptools>=36.6", "pytest-runner", "wheel",]
build-backend = "setuptools.build_meta"

and removing setup_requires from setup.py.

@sivakov512
Copy link
Owner

After this changes python setup.py test will fail. Can you provide more info about suggested changes?

@jonathanunderwood
Copy link
Author

Hm That's true. You'd need to pip install pytest and pytest directly. I understand that running of subcommands via setup.py is something that'll go away moving forward anyway. There's a way to fix this which satisfies the desire to keep python setup.py test still working, though, along the lines of:

import sys

...
needs_pytest = {'pytest', 'test', 'ptr'}.intersection(sys.argv)
pytest_runner = ['pytest-runner'] if needs_pytest else []

setup(
...
setup_requires=pytest_runner,
...
)

@sivakov512
Copy link
Owner

It looks like a bad solution.
How do you install it? I try to install this lib via pip and as "install requires" in setup.py and does not see installation of pytest-runner.
I can't reproduce it.

@jonathanunderwood
Copy link
Author

jonathanunderwood commented Apr 4, 2019

Not sure I understand the question.

pip install python-markdown-full-yaml-metadata should install the plugin, PyYAML and Markdown, but not pytest-runner. Why would you expect pytest-runner to be installed - it's only needed for running the tests, which aren't available.

python setup.py test should download pytest_runner and run the tests.

Which doesn't work?

@sivakov512
Copy link
Owner

Yes, that's true. I mean that I don't understand your problem with "setup_requires". What are the steps to reproduce your problem?

@jonathanunderwood
Copy link
Author

jonathanunderwood commented Apr 4, 2019

See the description at the top. Unfortunately, to reproduce it you'd need to be behind a corporate proxy that requires you add an extra CA cert to your trust store. The problem is that distutils doesn't support that, and it's distutils that is invoked to install the setup_requires deps. Hence, behind a corp proxy, or if using a local mirror of pypi, a pip install python-markdown-full-yaml-metadata fails at the point distutils is invoked to install the setup_requires dependencies.

setup_requires and invoking setup.py to ask "what dependencies does setuptools need to run?" is considered a broken pattern anyway, and tools are moving away from that (see: poetry, flit, and the fact that pip doesn't support it).

@ol-lo
Copy link

ol-lo commented Apr 4, 2019

@jonathanunderwood

Could you say please? How do you exactly do install the package? What command do you run?:

  • pip install
  • setup.py install
  • maybe something other?

@jonathanunderwood
Copy link
Author

pip install python-markdown-full-yaml-metadata

@sivakov512
Copy link
Owner

Oh, now I understand how the problem happens. I'll think about it and come back with a solution a bit later.

For now you can use version 1.1.0 - it should work correctly for you, but depending on the version of PyYAML ​​installed, it may produce warnings.

@sivakov512 sivakov512 reopened this Apr 4, 2019
@jonathanunderwood
Copy link
Author

Oh, now I understand how the problem happens. I'll think about it and come back with a solution a bit later.

What's wrong with the fix to setup.py I posted above?

@sivakov512
Copy link
Owner

I'm not sure this is a good solution. I would like the setup to always work the same.

Also it still seems wrong to me to require pytest-runner if the user just wants to install it

@jonathanunderwood
Copy link
Author

jonathanunderwood commented Apr 4, 2019

I'm not sure this is a good solution. I would like the setup to always work the same.

Also it still seems wrong to me to require pytest-runner if the user just wants to install it

Both of those are achieved with the change to the setup.py I outlined above

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.

3 participants