Start warning on non compliant pyproject.toml files and rejecting incorrect ones #5512

pradyunsg · 2018-06-18T18:19:38Z

Related to #5416

PEP 518 states:

There will be a [build-system] table in the configuration file to store build-related data. Initially only one key of the table will be valid and mandatory: requires. That key will have a value of a list of strings...

This PR makes pip warn when installing packages that they do not define build-system.requires in their pyproject.toml files and error out when it's not a list of strings.

benoit-pierre · 2018-06-18T18:23:19Z

src/pip/_internal/req/req_install.py

+            build_system = pp_toml.get('build-system', {})
+            if "requires" not in build_system:
+                raise InstallationError(
+                    "{} does not comply with PEP 518 as it since it's "


as it since it's?

I'd use the same format for both messages:

raise InstallationError( "{} does not comply with PEP 518 as pyproject.toml does" "not contain a valid [build-system].requires entry: {}" .format(self, reason) # with reason: 'missing' or 'not a list of strings' ) ```

Also, the function could be simplified to return the list of build requirements or None if PEP 518 support is not used, no need for the isolate return value anymore (and the corresponding code in prep_for_dist simplified too).

Indeed. Let's simplify the error message logic and the function.

benoit-pierre · 2018-06-18T18:27:22Z

tests/data/src/pep518_missing_requires/setup.py

+#!/usr/bin/env python
+from setuptools import setup
+
+import simplewheel  # ensure dependency is installed


That's not necessary, no?

benoit-pierre · 2018-06-18T18:28:25Z

tests/functional/test_install.py

+    result = script.pip(
+        'install',
+        '-f', common_wheels,
+        '-f', data.find_links,


Those 2 -f arguments are unnecessary, no?

benoit-pierre · 2018-06-18T18:28:35Z

tests/functional/test_install.py

+    result = script.pip(
+        'install',
+        '-f', common_wheels,
+        '-f', data.find_links,


Same as above.

I'd factorize the 2 tests with @pytest.mark.parametrize.

Nice idea about factorizing the tests.

benoit-pierre · 2018-06-18T18:36:25Z

src/pip/_internal/req/req_install.py

+
+            requires = build_system["requires"]
+            is_list_of_str = isinstance(requires, list) and all(
+                isinstance(req, str) for req in requires


- isinstance(req, str) for req in requires + isinstance(req, six.string_types) for req in requires

benoit-pierre · 2018-06-18T18:38:05Z

src/pip/_internal/req/req_install.py

@@ -571,8 +571,30 @@ def get_pep_518_info(self):
        if os.path.isfile(self.pyproject_toml):


How about taking the opportunity to simplify the function by getting the non PEP 518 case out of the way first:

if not os.path.isfile(self.pyproject_toml): return (['setuptools', 'wheel'], False)

Yeps. Good idea.

pradyunsg · 2018-06-18T19:39:54Z

Thanks for the review @benoit-pierre! Much appreciated. :)

asottile · 2018-06-18T23:11:41Z

I really think the PEP should be amended. As it is right now:

someone innocently adds black / towncrier / $othertool information in newly created pyproject.toml and suddenly their package isn't installable

I think now that PEP518 has settled on toml + pyproject.toml a bunch of tools are going to switch from setup.cfg / tox.ini (which has various problems due to interpolation, etc.).

RonnyPfannschmidt · 2018-06-19T06:10:04Z

this should be a big red warning for a while,
for example pytest ever since using towncrier is "invalid"

(this is on us as well for not properly researching the pep either)

but in the end just rejecting the projects will "break" what was "working"

i also disagree with @asottile wrt amending the pep - but pip will have to be graceful since it didnt reject before

pradyunsg · 2018-06-19T06:31:47Z

@RonnyPfannschmidt I stated in #5402 (comment) why I think it's better to reject "working"; until the PEP can be updated.

pradyunsg · 2018-06-19T06:38:38Z

@asottile The next step for amending the PEP would be to start a new thread on distutils-sig, making a case for why the amendment should be made.

RonnyPfannschmidt · 2018-06-19T07:32:00Z

@pradyunsg this will break various versions of pytest at least as well as plenty of other packages then ^^

pradyunsg · 2018-06-19T07:37:32Z

We're having a discussion about this in 3 places. Can we please move the discussion about dealing with non-compliant pyproject.toml files to #5416?

this makes the file valid and prepares for pypa/pip#5416 and pypa/pip#5512

blueyed · 2018-06-20T05:58:29Z

From my understanding PEP 518 only states that requires is required in the build-system section, but not that the build-system section is required.
And using the provided JSON schema allows for an empty pyproject.toml (i.e. "{}" as JSON).

pradyunsg · 2018-06-20T06:09:42Z

@blueyed I read it differently; I think we're in distutils-sig territory now on this -- amending PEP 518 to remove this ambiguity.

blueyed · 2018-06-20T06:13:04Z

@pradyunsg
Great ~~(do you have link?)~~ (misunderstood that there was discussion already somewhere happening).
Apart from that you can easily test it using the provided JSON schema already.

pradyunsg · 2018-06-20T07:17:52Z

@pypa/pip-committers Are there any reservations/blockers to merging this?

The behavior change here is an improvement over status quo
Transitioning to either of the positions the PEP might take is straightforward:
- optional: stop warning when it's missing
- mandatory: error out when it's missing

pfmoore · 2018-06-20T07:21:39Z

@pradyunsg No reservations from me.

lock · 2019-06-02T10:04:41Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

pradyunsg added T: bugfix PEP implementation Involves some PEP labels Jun 18, 2018

benoit-pierre reviewed Jun 18, 2018

View reviewed changes

pradyunsg force-pushed the feature/refuse-non-compliant-pyproject branch from 44841d3 to 7460551 Compare June 18, 2018 19:40

pfmoore mentioned this pull request Jun 18, 2018

Load build backend in isolated build environment with --no-index given #5402

Closed

pradyunsg mentioned this pull request Jun 19, 2018

PEP 518: Behavior when pyproject.toml doesn't have build-system.requires #5416

Closed

RonnyPfannschmidt added a commit to RonnyPfannschmidt/pytest that referenced this pull request Jun 19, 2018

add build-system section to pyproject.toml

af5d41f

this makes the file valid and prepares for pypa/pip#5416 and pypa/pip#5512

RonnyPfannschmidt mentioned this pull request Jun 19, 2018

add build-system section to pyproject.toml pytest-dev/pytest#3600

Merged

pradyunsg changed the title ~~Start refusing non compliant pyproject.toml files~~ Start warning on non compliant pyproject.toml files and rejecting incorrect ones Jun 19, 2018

pradyunsg added 6 commits June 19, 2018 16:27

Start refusing non PEP 518 pyproject.toml files

0534aeb

Add tests to verify refusal of non PEP 518 compliant pyproject.toml

a25cb53

Rework PEP 518 requirement logic

0eb566f

Cleanup Tests

fdd5101

📰

8f1cd5f

Only disable isolation when build-system.requires is skipped

2b68c79

pradyunsg force-pushed the feature/refuse-non-compliant-pyproject branch from ed154a1 to 2b68c79 Compare June 19, 2018 10:58

pradyunsg merged commit e8a5668 into pypa:master Jun 20, 2018

pradyunsg deleted the feature/refuse-non-compliant-pyproject branch June 20, 2018 09:55

pradyunsg mentioned this pull request Jul 9, 2018

Isolate when pyproject.toml does not have build-system.requires #5585

Merged

pradyunsg mentioned this pull request Jul 21, 2018

Don't warn if pyproject.toml does not contain build-system.requires #5626

Merged

lock bot added the auto-locked Outdated issues that have been locked by automation label Jun 2, 2019

lock bot locked as resolved and limited conversation to collaborators Jun 2, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Start warning on non compliant pyproject.toml files and rejecting incorrect ones #5512

Start warning on non compliant pyproject.toml files and rejecting incorrect ones #5512

pradyunsg commented Jun 18, 2018 •

edited

Loading

benoit-pierre Jun 18, 2018

benoit-pierre Jun 18, 2018

benoit-pierre Jun 18, 2018

pradyunsg Jun 18, 2018 •

edited

Loading

benoit-pierre Jun 18, 2018

benoit-pierre Jun 18, 2018

pradyunsg Jun 18, 2018

benoit-pierre Jun 18, 2018

benoit-pierre Jun 18, 2018

pradyunsg Jun 18, 2018

benoit-pierre Jun 18, 2018

benoit-pierre Jun 18, 2018

pradyunsg Jun 18, 2018

pradyunsg commented Jun 18, 2018

asottile commented Jun 18, 2018

RonnyPfannschmidt commented Jun 19, 2018

pradyunsg commented Jun 19, 2018

pradyunsg commented Jun 19, 2018

RonnyPfannschmidt commented Jun 19, 2018

pradyunsg commented Jun 19, 2018 •

edited

Loading

blueyed commented Jun 20, 2018 •

edited

Loading

pradyunsg commented Jun 20, 2018

blueyed commented Jun 20, 2018 •

edited

Loading

pradyunsg commented Jun 20, 2018

pfmoore commented Jun 20, 2018

lock bot commented Jun 2, 2019

		@@ -571,8 +571,30 @@ def get_pep_518_info(self):
		if os.path.isfile(self.pyproject_toml):

Start warning on non compliant pyproject.toml files and rejecting incorrect ones #5512

Start warning on non compliant pyproject.toml files and rejecting incorrect ones #5512

Conversation

pradyunsg commented Jun 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pradyunsg Jun 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pradyunsg commented Jun 18, 2018

asottile commented Jun 18, 2018

RonnyPfannschmidt commented Jun 19, 2018

pradyunsg commented Jun 19, 2018

pradyunsg commented Jun 19, 2018

RonnyPfannschmidt commented Jun 19, 2018

pradyunsg commented Jun 19, 2018 • edited Loading

blueyed commented Jun 20, 2018 • edited Loading

pradyunsg commented Jun 20, 2018

blueyed commented Jun 20, 2018 • edited Loading

pradyunsg commented Jun 20, 2018

pfmoore commented Jun 20, 2018

lock bot commented Jun 2, 2019

pradyunsg commented Jun 18, 2018 •

edited

Loading

pradyunsg Jun 18, 2018 •

edited

Loading

pradyunsg commented Jun 19, 2018 •

edited

Loading

blueyed commented Jun 20, 2018 •

edited

Loading

blueyed commented Jun 20, 2018 •

edited

Loading