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

Add support for setup.cfg format? #132

Closed
greschd opened this issue Apr 23, 2020 · 21 comments · Fixed by #190
Closed

Add support for setup.cfg format? #132

greschd opened this issue Apr 23, 2020 · 21 comments · Fixed by #190

Comments

@greschd
Copy link
Member

greschd commented Apr 23, 2020

Setuptools provides the option use a setup.cfg configuration file instead of specifying everything in setup.py (documentation).

I think adding support for that would be pretty straightforward, using setuptools.config.read_configuration to parse the config.

Is there any interest in supporting this format? Remember, "PR welcome" is always a valid response 😉

@ltalirz
Copy link
Member

ltalirz commented Apr 23, 2020

My impression was that the ecosystem seems to be moving to the pyproject.toml.
Would be great if we could avoid the detour via setup.cfg (which also some tools started supporting...)

@greschd
Copy link
Member Author

greschd commented Apr 23, 2020

My understanding is that those are two separate things that are intended to work together. The pyproject.toml specifies which tool should build the package, while setup.cfg is a setuptools-specific configuration replacing setup.py (almost, you still need a shim setup.py for editable installs).

@greschd
Copy link
Member Author

greschd commented Apr 23, 2020

The fact that different tools allow putting their configuration in pyproject.toml or setup.cfg is a bit unfortunate, IMHO.

@greschd
Copy link
Member Author

greschd commented Apr 23, 2020

In any case, when building with setuptools, the metadata / options / etc. can AFAIK not go into pyproject.toml, that file contains only

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

and whatever other sections other tools might need.

@ltalirz
Copy link
Member

ltalirz commented Apr 23, 2020

Ok. On a related note, you may notice that we already have support in the registry for parsing info from the pyproject.toml for flit/poetry, this was added by @dev-zero

"""Fetch metadata from plugins and dump it to temporary JSON file.
This fetches metadata from plugins defined using different build systems
* setuptools/pip: setup.json
* poetry: pyproject.toml
* flit: pyproject.toml
"""

Once the direction of the overall ecosystem has converged (or has it already?), it would be great to define a strategy for where we want to go and suggest people to follow it.
Until then, I'm a bit reluctant to change things given that the status quo is "working".

@greschd
Copy link
Member Author

greschd commented Apr 23, 2020

Once the direction of the overall ecosystem has converged (or has it already?)

With regards to build systems, I think it's converging to having a top-level standard (pyproject.toml) that allows using different ones. I doubt it will converge on a single build system.. ever.

Is there a particular reason we're fetching the metadata from the repositories, instead of the built distributions that are already on PyPI?

The format of metadata in {sdist, wheel} is AFAIK standardized in PEP 566, so it wouldn't matter how exactly it gets there -- developers can choose whichever build system / configuration format they like.

@ltalirz
Copy link
Member

ltalirz commented Apr 23, 2020

Is there a particular reason we're fetching the metadata from the repositories, instead of the built distributions that are already on PyPI?

There is certainly a historical one, namely that not all packages were/are on pypi.
I'd happily accept a PR that fetches the data from pypi, where the pip_url is simply a python package name (in that case, the plugin_info key can be omitted).

Fully agree this would be a nice improvement.

@greschd
Copy link
Member Author

greschd commented Apr 23, 2020

What's your opinion on using e.g. pip to build the package from source if the package is not on PyPI? It would be a heavier process - which I'm not sure is justified to only fetch the metadata.

On the other hand, adding support for different build systems ourselves feels like re-implementing things that pip already has.

For example:

(aiida) $ time pip wheel --no-deps git+https://github.com/greschd/aiida-bands-inspect.git
Collecting git+https://github.com/greschd/aiida-bands-inspect.git
  Cloning https://github.com/greschd/aiida-bands-inspect.git to /tmp/pip-req-build-uz4i3nqc
  Running command git clone -q https://github.com/greschd/aiida-bands-inspect.git /tmp/pip-req-build-uz4i3nqc
Building wheels for collected packages: aiida-bands-inspect
  Building wheel for aiida-bands-inspect (setup.py) ... done
  Created wheel for aiida-bands-inspect: filename=aiida_bands_inspect-0.4.0-py3-none-any.whl size=16406 sha256=139d571db9fad274df36c9482f2f2e14557bda9b8ce8b9f5bed8e3234dd1d309
  Stored in directory: /tmp/pip-ephem-wheel-cache-2koazn61/wheels/48/12/31/cf62798efecee1f93dca582734107983f38d814871bdeed8a7
Successfully built aiida-bands-inspect

real    0m3.694s
user    0m1.109s
sys     0m1.500s

(aiida) $ time wheel unpack aiida_bands_inspect-0.4.0-py3-none-any.whl
Unpacking to: ./aiida_bands_inspect-0.4.0...OK

real    0m0.163s
user    0m0.031s
sys     0m0.094s

(aiida) $ tree aiida_bands_inspect-0.4.0/aiida_bands_inspect-0.4.0.dist-info/
aiida_bands_inspect-0.4.0/aiida_bands_inspect-0.4.0.dist-info/
├── LICENSE.txt
├── METADATA
├── RECORD
├── WHEEL
├── entry_points.txt
└── top_level.txt

0 directories, 6 files

@greschd
Copy link
Member Author

greschd commented Apr 23, 2020

Tested also with aiida-graphql (which uses poetry), it works but takes ~35s -- spent almost entirely "installing build dependencies".

@dev-zero
Copy link
Contributor

dev-zero commented Apr 23, 2020

If there would be a large number of build systems I'd agree that this needs fixing. But given that their number is limited and the information reachable without having to execute code I fail to see the need for action here.

fyi, for Poetry:

@greschd
Copy link
Member Author

greschd commented Apr 23, 2020

@dev-zero what's your opinion on adding setup.cfg support?

@ltalirz
Copy link
Member

ltalirz commented Apr 23, 2020

What's your opinion on using e.g. pip to build the package from source if the package is not on PyPI? It would be a heavier process - which I'm not sure is justified to only fetch the metadata.

Right... it's a possibility and 10s per package won't kill us but I don't think it's really necessary.

If there would be a large number of build systems I'd agree that this needs fixing. But given that their number is limited and the information reachable without having to execute code I fail to see the need for action here.

Just to clarify: for the packages that are already on PyPI, fetching the information from there is an improvement -- there have already been cases where the setup.json in the respective master branch and the pypi release were not in sync, so we are displaying different information to what users get when they follow the installation instructions.

@greschd
Copy link
Member Author

greschd commented Apr 23, 2020

For poetry, isn't the issue fixed with pip==19.0 as commented here?

AFAICT the remainder of the discussion is about editable installs and other package installers.

@greschd
Copy link
Member Author

greschd commented Apr 23, 2020

Just to clarify: for the packages that are already on PyPI, fetching the information from there is an improvement -- there have already been cases where the setup.json in the respective master branch and the pypi release were not in sync, so we are displaying different information to what users get when they follow the installation instructions.

Yeah, if we implement fetching from PyPI (and thus parsing the sdist / wheel formats), is there a point to keep the parsing from setup.json / pyproject.toml code around? Or would it be a simpler fallback to build the wheel from source?

@greschd greschd changed the title Add support for setup.cfg format? Fetch metadata from PyPI Apr 23, 2020
@dev-zero
Copy link
Contributor

@dev-zero what's your opinion on adding setup.cfg support?

Sure, since this will probably just be a couple of lines.

Yeah, if we implement fetching from PyPI (and thus parsing the sdist / wheel formats), is there a point to keep the parsing from setup.json / pyproject.toml code around? Or would it be a simpler fallback to build the wheel from source?

Since we already have it I'd keep it as a fallback for now for packages not (yet) released on pypi.

For poetry, isn't the issue fixed with pip==19.0 as commented here?

Which issue are you referring to now? For being able to install a package without a setup.py with pip? Sure, pip install --use-pep517 . works on aiida-graphql. On the other hand is the parsing already implemented, so why waste cpu cycles there? ;-)

@greschd
Copy link
Member Author

greschd commented Apr 23, 2020

Which issue are you referring to now? For being able to install a package without a setup.py with pip? Sure, pip install --use-pep517 . works on aiida-graphql. On the other hand is the parsing already implemented, so why waste cpu cycles there? ;-)

Yes, I though the link to python-poetry/poetry#761 was meant to say this doesn't work with poetry.

Anyway, I agree with you here - we can add setup.cfg support pretty easily, and I'm not sure we need to add the code for wheel parsing.

Surprisingly I haven't found an easy tool to load the data in egg-info and dist-info -- although I also didn't look too hard.

@greschd greschd changed the title Fetch metadata from PyPI Add support for setup.cfg format? Apr 23, 2020
@ltalirz
Copy link
Member

ltalirz commented Apr 24, 2020

Feel free to proceed in either direction - a PR to add support for parsing the setup.cfg would be welcome as well :-)
My point here was just this: so far, we have a uniform approach with the setup.json file that may not be perfect, but works.
I.e. before suggesting to all plugin developers to switch to the setup.cfg by default, we should be convinced that this

  • is our "final answer" (at least for a few years to come)
  • will bring some benefits (I guess there will be...)

Of course, the first step is anyhow for someone to give it a try, and so there is no issue in adding support for it in the registry.

Cheers,
Leo

@greschd
Copy link
Member Author

greschd commented Apr 24, 2020

before suggesting to all plugin developers to switch to the setup.cfg by default

I'm not suggesting advertising it in any way 😉

In terms of benefits, the one I did find is that it allows a somewhat neat solution to the "duplicated version source" problem: The attr: directive can read from the Python source (provided the __version__ is defined in a file that doesn't itself import things that are not present at build time).

@chrisjsewell
Copy link
Member

In terms of benefits, the one I did find is that it allows a somewhat neat solution to the "duplicated version source" problem

ha I was just coming here to see if an issue was open for this, and mention the version ting as I've just added it to aiidalab: https://github.com/aiidalab/aiidalab/pull/162/files

definitely +1

My impression was that the ecosystem seems to be moving to the pyproject.toml.

Yeh hopefully with https://www.python.org/dev/peps/pep-0621/ now accepted, setup.cfg will eventually merge into pyproject.toml

@chrisjsewell
Copy link
Member

Additional FYI, apparently this is probably what a dynamic version will look like in pyproject.toml: https://discuss.python.org/t/why-isnt-there-a-version-read-from/7660/3

@chrisjsewell
Copy link
Member

Ok, I'm closing this with #190 for adding setup.cfg parsing (which I would most definitely advise over the bespoke setup.json). This also includes providing a version_file key to use for setup.cfg that use the attr: directive (__version__ is read statically)

We can open a separate issue for sourcing from PyPI at some point if you want. As already discussed, it's probably more consistent, but equally it seems that no one can actually be bothered to do it lol

If we do, then this is the format of PKG-INFO: https://www.python.org/dev/peps/pep-0314/

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

Successfully merging a pull request may close this issue.

4 participants