pypa / readme_renderer

Safely render long_description/README files in Warehouse
Apache License 2.0
157 stars 88 forks source link

this tools should install first build-requires from pep-517 before executing #118

Closed gaborbernat closed 5 years ago

gaborbernat commented 6 years ago

Otherwise, build extensions (e.g. setuptools-scm) cannot be interpreted (unless also fored in setup_requires). This package tries to look into setup.py so should first ensure it has build-requires.

2018-09-12T17:48:42.0336128Z package-description runtests: commands[0] | python setup.py check -r -s
2018-09-12T17:48:42.3094762Z running check
2018-09-12T17:48:42.3112596Z /opt/hostedtoolcache/Python/3.7.0/x64/lib/python3.7/distutils/dist.py:274: UserWarning: Unknown distribution option: 'use_scm_version'
2018-09-12T17:48:42.3129581Z   warnings.warn(msg)
2018-09-12T17:48:42.3145311Z warning: Check: missing required meta-data: version
2018-09-12T17:48:42.3153240Z 
2018-09-12T17:48:42.9435964Z The project's long description is valid RST.
2018-09-12T17:48:42.9454270Z error: Please correct your package.
2018-09-12T17:48:43.0199465Z ERROR: InvocationError for command '/home/vsts/work/1/s/.tox/package-description/bin/python setup.py check -r -s' (exited with code 1)
theacodes commented 6 years ago

I'm not quite sure I understand what the bug/request is here?

gaborbernat commented 6 years ago

this tools need to ensure build dependencies are installed before doing a validity check on setup.py. eg the error above is a false positive reported because the lack of this.

di commented 6 years ago

I think what @gaborbernat is trying to say is that if your package has build dependencies (like using setuptools_scm in your setup.py that running python setup.py check -r -s won't install those dependencies automatically.

This package is just a distutils extension though, if this is an issue, it should be fixed in distutils, not in each individual package that extends it.

gaborbernat commented 6 years ago

@di not sure if distutils has plans to implement PEP 517/8 though :thinking: generally people rarely use nowadays distutils directly and instead there's a tendency for going down the setuptools/poetry etc path; not?

dstufft commented 6 years ago

This command happens too late in the process to do anything wrt to PEP517/518. One of the things that pyproject.toml enables is sticking import foo at the top of your setup.py, which would mean that the issues with setup_requires would come back, that if you wanted to use this command you'd have to delay all your imports and such so that this command had a chance to install dependencies first.

IOW, I don't think there is a way to do this, except by not integrating as a setup.py command and working as a top level command instead. Probably it would be saner to add a twine check command or something that could do the needful and invoke readme_renderer in a post PEP 517/518 world.

gaborbernat commented 6 years ago

agreed can we move warehouse check to twine then ?

dstufft commented 6 years ago

I'm not sure what you're asking there? Warehouse will continue to check the description itself, but it doesn't use twine itself at all.

gaborbernat commented 6 years ago

is not warehouse using this tool to do the check?

dstufft commented 6 years ago

Warehouse is using readme_renderer yes, my suggestion would be that twine takes a dependency on readme_renderer to do the check.

gaborbernat commented 6 years ago

I'm not sure I follow how would that fix the problem, would twine install build dependencies via PEP-517/8 specification?

di commented 5 years ago

@gaborbernat You would make the distribution with your PEP 517/518 build tool of choice, and twine check would extract the description from the distribution, rather than by running setup.py.

gaborbernat commented 5 years ago

I see. That would imply though for this tool to read the data from the distribution meta-data, and warehouse to switch over to use twine check to validate the readme not (by delegating to the content of this package)?

di commented 5 years ago

@gaborbernat I'm not really following you. Twine already reads from the distribution metadata, we would just instruct people to run twine check rather than python setup.py check. This wouldn't change anything about how Warehouse uses this project.

gaborbernat commented 5 years ago

I see. So what would this twine require to be a reality?

My goal would be to have a way to locally check that the readme file is correct. https://github.com/pypa/readme_renderer#restructuredtext this now instructs to use setup.py; however that does not play nicely with build dependencies.

di commented 5 years ago

I see. So what would this twine require to be a reality?

https://github.com/pypa/twine/pull/395 should do it.

I'm going to leave this issue open until that's merged/released and we've updated the readme_renderer documentation to tell people to use twine check instead.

gaborbernat commented 5 years ago

Thanks! thanks

di commented 5 years ago

This is done, thanks for raising the issue @gaborbernat!