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

Revisiting tox's requirement that build-system is defined in pyproject.toml #2429

Closed
di opened this issue May 31, 2022 · 4 comments
Closed
Labels
bug:normal affects many people or has quite an impact fixed-by-tox4 This has been fixed within tox 4, and we don't plant to backport it to version 3.

Comments

@di
Copy link

di commented May 31, 2022

Currently tox requires that a build-system table is defined in pyproject.toml:

$ echo -e '[tox]\nisolated_build = true' > tox.ini

$ touch pyproject.toml

$ python -m tox -e py
ERROR: build-system section missing inside /tmp/sampleproject/pyproject.toml

This was previously discussed in #1482 by @nuno-andre and eventually closed, but I think the intention there was generally correct: PEP 518 is explicit that the build-system table is not required:

Tools should not require the existence of the [build-system] table... If the file exists but is lacking the [build-system] table then the default values as specified above should be used.

Furthermore, most other tools do not have this requirement. For example, build supports omitting the build-system table -- when building with the above example:

$ python -m build
* Creating virtualenv isolated environment...
* Installing packages in isolated environment... (setuptools >= 40.8.0, wheel)
* Getting dependencies for sdist...
running egg_info
creating UNKNOWN.egg-info
writing UNKNOWN.egg-info/PKG-INFO
writing dependency_links to UNKNOWN.egg-info/dependency_links.txt
writing top-level names to UNKNOWN.egg-info/top_level.txt
writing manifest file 'UNKNOWN.egg-info/SOURCES.txt'
reading manifest file 'UNKNOWN.egg-info/SOURCES.txt'
writing manifest file 'UNKNOWN.egg-info/SOURCES.txt'
* Building sdist...
running sdist
running egg_info
writing UNKNOWN.egg-info/PKG-INFO
writing dependency_links to UNKNOWN.egg-info/dependency_links.txt
writing top-level names to UNKNOWN.egg-info/top_level.txt
reading manifest file 'UNKNOWN.egg-info/SOURCES.txt'
writing manifest file 'UNKNOWN.egg-info/SOURCES.txt'
warning: sdist: standard file not found: should have one of README, README.rst, README.txt, README.md

running check
warning: check: missing required meta-data: name

creating UNKNOWN-0.0.0
creating UNKNOWN-0.0.0/UNKNOWN.egg-info
copying files to UNKNOWN-0.0.0...
copying pyproject.toml -> UNKNOWN-0.0.0
copying UNKNOWN.egg-info/PKG-INFO -> UNKNOWN-0.0.0/UNKNOWN.egg-info
copying UNKNOWN.egg-info/SOURCES.txt -> UNKNOWN-0.0.0/UNKNOWN.egg-info
copying UNKNOWN.egg-info/dependency_links.txt -> UNKNOWN-0.0.0/UNKNOWN.egg-info
copying UNKNOWN.egg-info/top_level.txt -> UNKNOWN-0.0.0/UNKNOWN.egg-info
Writing UNKNOWN-0.0.0/setup.cfg
Creating tar archive
removing 'UNKNOWN-0.0.0' (and everything under it)
* Building wheel from sdist
* Creating virtualenv isolated environment...
* Installing packages in isolated environment... (setuptools >= 40.8.0, wheel)
* Getting dependencies for wheel...
running egg_info
writing UNKNOWN.egg-info/PKG-INFO
writing dependency_links to UNKNOWN.egg-info/dependency_links.txt
writing top-level names to UNKNOWN.egg-info/top_level.txt
reading manifest file 'UNKNOWN.egg-info/SOURCES.txt'
writing manifest file 'UNKNOWN.egg-info/SOURCES.txt'
* Installing packages in isolated environment... (wheel)
* Building wheel...
running bdist_wheel
running build
installing to build/bdist.linux-x86_64/wheel
running install
running install_egg_info
running egg_info
writing UNKNOWN.egg-info/PKG-INFO
writing dependency_links to UNKNOWN.egg-info/dependency_links.txt
writing top-level names to UNKNOWN.egg-info/top_level.txt
reading manifest file 'UNKNOWN.egg-info/SOURCES.txt'
writing manifest file 'UNKNOWN.egg-info/SOURCES.txt'
Copying UNKNOWN.egg-info to build/bdist.linux-x86_64/wheel/UNKNOWN-0.0.0-py3.9.egg-info
running install_scripts
creating build/bdist.linux-x86_64/wheel/UNKNOWN-0.0.0.dist-info/WHEEL
creating '/tmp/sampleproject/dist/tmpkm28yh9l/UNKNOWN-0.0.0-py3-none-any.whl' and adding 'build/bdist.linux-x86_64/wheel' to it
adding 'UNKNOWN-0.0.0.dist-info/METADATA'
adding 'UNKNOWN-0.0.0.dist-info/WHEEL'
adding 'UNKNOWN-0.0.0.dist-info/top_level.txt'
adding 'UNKNOWN-0.0.0.dist-info/RECORD'
removing build/bdist.linux-x86_64/wheel
Successfully built UNKNOWN-0.0.0.tar.gz and UNKNOWN-0.0.0-py3-none-any.whl

Additionally, pip does support omitting it as well:

$ pip install .
Defaulting to user installation because normal site-packages is not writeable
Processing /tmp/sampleproject
  Installing build dependencies ... done
  Getting requirements to build wheel ... done
  Preparing metadata (pyproject.toml) ... done
Building wheels for collected packages: UNKNOWN
  Building wheel for UNKNOWN (pyproject.toml) ... done
  Created wheel for UNKNOWN: filename=UNKNOWN-0.0.0-py3-none-any.whl size=920 sha256=df0aa573f73a6d411f548e7064cf1566e4121560cca4e4d4475bd4705aad4da1
  Stored in directory: /tmp/pip-ephem-wheel-cache-4yph8vy7/wheels/43/b3/41/c546307af7091a98924f94fc7774884cc49fc15f0211c49065
Successfully built UNKNOWN
Installing collected packages: UNKNOWN
Successfully installed UNKNOWN-0.0.0

I think tox should do what these tools do and provide defaults when the build-system table is omitted, rather than erroring.

@di di added the bug:normal affects many people or has quite an impact label May 31, 2022
@gaborbernat
Copy link
Member

This has been fixed in tox v4. If anyone wants to fix it in v3 they're invited to put in a PR for it.

@gaborbernat
Copy link
Member

Furthermore, most other tools do not have this requirement. For example, build supports omitting the build-system table -- when building with the above example:

Additionally, pip does support omitting it as well:

Arguably those tools support it, however, it's still not considered best practice to use the default. You know, explicit better than implicit? Considering tox is encouraging users to follow best practices one can make the argument that tox shouldn't support defaulting...

@di
Copy link
Author

di commented May 31, 2022

This has been fixed in tox v4.

Thanks, I forgot to check against pre-releases! Feel free to close this as that's sufficient for me. (As an aside, is there an issue I can track for the tox4 release? Or is https://github.com/tox-dev/tox/milestone/7 what I should use?)

Arguably those tools support it, however, it's still not considered best practice to use the default. You know, explicit better than implicit? Considering tox is encouraging users to follow best practices one can make the argument that tox shouldn't support defaulting...

I think it's fine for tox to make additional recommendations on declaring default fields, but my original issue was just with the error when it's not present, which is explicitly prohibited by the PEP 518 spec 😉

@jugmac00 jugmac00 added the fixed-by-tox4 This has been fixed within tox 4, and we don't plant to backport it to version 3. label Jun 4, 2022
@gaborbernat
Copy link
Member

Version 4 is now the main one, so closing this.

sjdemartini added a commit to sjdemartini/graphene-django-permissions that referenced this issue May 3, 2023
sjdemartini added a commit to sjdemartini/graphene-django-permissions that referenced this issue May 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug:normal affects many people or has quite an impact fixed-by-tox4 This has been fixed within tox 4, and we don't plant to backport it to version 3.
Projects
None yet
Development

No branches or pull requests

3 participants