Emit warning for invalid [tools.setuptools] table

[SnoopJ]

Summary of changes

This changeset adds a warning when the invalid [tools.setuptools] table is detected in pyproject.toml metadata, as a hint to users that any data they've declared there is being ignored by setuptools.

Closes #4150.

Pull Request Checklist

• Changes have tests
  • I have not added a test because the _ExperimentalConfiguration warning in the affected module is also not tested. I'd be happy to write one if the maintainers would like one.
• News fragment added in newsfragments/.
  (See documentation for details)

[abravalheri]

Hi @SnoopJ, thank you very much for the contribution.

I agree that this makes setuptools more friendly towards the end user.
But I am a bit on the fence about excessive defensive programming, because it does impose a cost in terms of maintenance (for example, ideally we want a test for this change, so all the code introduced is properly covered -- but then we also have a cost in terms of increase in testing time/resources) and code complexity.

For this change I guess it is fine, since it is small and trivial. But I think we do want to have that line covered in the tests¹.

Alternatively, users can also rely on linter's scrutiny for catching problems like this.
Have you tried using something like validate-pyproject' pre-commit hook?
Would that be a practical solution for this problem?

Footnotes

1. In general is a good thing that all code paths run at least once in the test suite, even if it is trivial, because it works as a regression test in the case something we depend on changes in the future. ↩

[SnoopJ]

Have you tried using something like validate-pyproject' pre-commit hook? Would that be a practical solution for this problem?

To be clear, I have personally not had this problem in quite a while, but filed the originating issue and this contribution on behalf of a user who did have this problem and who I consider to be pretty sophisticated. A linter could certainly catch the problem, but I think the users who are most likely to have this problem are probably not even going to be aware that linters are an option.

But I think we do want to have that line covered in the tests1.

Sure, I can look at getting it under coverage by the tests.

I agree that this makes setuptools more friendly towards the end user. But I am a bit on the fence about excessive defensive programming, because it does impose a cost in terms of maintenance

I agree with the general point that there's a limit to what setuptools ought to do, although where exactly the line for "excessive" lies is subjective and I don't know where setuptools falls philosophically on that spectrum. Personally, in a case like this with a fairly likely UX stumbling point, I'm usually willing to engage in the tradeoff. I can't think of any other issues with metadata user error that I think would be worthwhile for setuptools to bother with, but acknowledge that adding this change would create a bit of a precedent.

Since the only action taken here is issuing a warning, the main maintenance burden I see (aside from test coverage) is to the legibility of the surrounding code, especially if setuptools got deeper into the business of "sanity checking" the project metadata (requiring more warnings and probably a dedicated method for performing the checks), but as an outsider I may be underestimating the complexity/burden that would be added to setuptools by this changeset.

[abravalheri]

Yeah, I agree that the change here is very minimal and straight forward so I think it is fine to add it to the code base (if we manage to get it covered in the tests).

[SnoopJ]

Having another look at this, resolving the test failures it introduces before I write a new test. Turns out that some of them are caused by this typo being present in the test suite, which does make me feel better about it as a feature.

Not sure yet about the other failures.

[SnoopJ]

Okay I've re-synchronized with main and I now understand the failures, but I think I will need the assistance of a setuptools maintainer to resolve them. After applying the fix for the bad metadata in the test suite, there are 5 failing tests. Two of those failures (measure_startup_perf and test_editable_with_prefix) are also present against main on my system and I will therefore ignore them. Maybe I have a local configuration problem and they will come back okay in CI.

The remaining failures are tests for inclusion of typing files when using the metadata that previously contained the typo:

click to see failures

[gw0] FAILED setuptools/tests/test_build_py.py::TestTypeInfoFiles::test_type_files_included_by_default[simple_namespace-dont_include_package_data]                      
[gw4] FAILED setuptools/tests/test_build_py.py::TestTypeInfoFiles::test_type_files_included_by_default[namespace_nested_inside_regular-dont_include_package_data]       
[gw3] FAILED setuptools/tests/test_build_py.py::TestTypeInfoFiles::test_type_files_included_by_default[nested_inside_namespace-dont_include_package_data]               

__________________________________ TestTypeInfoFiles.test_type_files_included_by_default[simple_namespace-dont_include_package_data] ___________________________________
[gw1] linux -- Python 3.9.14 /home/jgerity/repos/setuptools.git/.direnv/python-3.9.14/bin/python3                                                                       
                                                                                    
self = <setuptools.tests.test_build_py.TestTypeInfoFiles object at 0x72a9790e0fa0>, tmpdir_cwd = local('/home/jgerity/repos/setuptools.git')
pyproject = 'dont_include_package_data', example = 'simple_namespace'                                                                                                   
                                                                                                                                                                        
    @pytest.mark.parametrize(                                                                                                                                           
        "pyproject", ["default_pyproject", "dont_include_package_data"]                                                                                                 
    )                                                                               
    @pytest.mark.parametrize("example", EXAMPLES.keys())                                                                                                                
    def test_type_files_included_by_default(self, tmpdir_cwd, pyproject, example):  
        structure = {                                                                                                                                                   
            **self.EXAMPLES[example]["directory_structure"],                                                                                                            
            "pyproject.toml": self.PYPROJECTS[pyproject],                                                                                                               
        }                                                                           
        expected_type_files = self.EXAMPLES[example]["expected_type_files"]                                                                                             
        jaraco.path.build(structure)                                                                                                                                    
                                                                                    
        build_py = get_finalized_build_py()                                                                                                                             
        outputs = get_outputs(build_py)                                                                                                                                 
>       assert expected_type_files <= outputs                                                                                                                           
E       AssertionError: assert {'foo/bar.pyi...foo/py.typed'} <= set()                                                                                                  
E         
E         Extra items in the left set:
E         'foo/py.typed'
E         'foo/bar.pyi'

/home/jgerity/repos/setuptools.git/setuptools/tests/test_build_py.py:412: AssertionError
_______________________________ TestTypeInfoFiles.test_type_files_included_by_default[nested_inside_namespace-dont_include_package_data] _______________________________
[gw1] linux -- Python 3.9.14 /home/jgerity/repos/setuptools.git/.direnv/python-3.9.14/bin/python3

self = <setuptools.tests.test_build_py.TestTypeInfoFiles object at 0x72a9790e6340>, tmpdir_cwd = local('/home/jgerity/repos/setuptools.git')
pyproject = 'dont_include_package_data', example = 'nested_inside_namespace'

    @pytest.mark.parametrize(
        "pyproject", ["default_pyproject", "dont_include_package_data"]
    )
    @pytest.mark.parametrize("example", EXAMPLES.keys())
    def test_type_files_included_by_default(self, tmpdir_cwd, pyproject, example):
        structure = {
            **self.EXAMPLES[example]["directory_structure"],
            "pyproject.toml": self.PYPROJECTS[pyproject],
        }
        expected_type_files = self.EXAMPLES[example]["expected_type_files"]
        jaraco.path.build(structure)
     
        build_py = get_finalized_build_py()
        outputs = get_outputs(build_py)
>       assert expected_type_files <= outputs
E       AssertionError: assert {'foo/bar/mod...bar/py.typed'} <= set()
E         
E         Extra items in the left set:
E         'foo/bar/py.typed'
E         'foo/bar/mod.pyi'

/home/jgerity/repos/setuptools.git/setuptools/tests/test_build_py.py:412: AssertionError
___________________________ TestTypeInfoFiles.test_type_files_included_by_default[namespace_nested_inside_regular-dont_include_package_data] ___________________________
[gw1] linux -- Python 3.9.14 /home/jgerity/repos/setuptools.git/.direnv/python-3.9.14/bin/python3

self = <setuptools.tests.test_build_py.TestTypeInfoFiles object at 0x72a9790e64c0>, tmpdir_cwd = local('/home/jgerity/repos/setuptools.git')
pyproject = 'dont_include_package_data', example = 'namespace_nested_inside_regular' 

    @pytest.mark.parametrize(
        "pyproject", ["default_pyproject", "dont_include_package_data"]
    )
    @pytest.mark.parametrize("example", EXAMPLES.keys())
    def test_type_files_included_by_default(self, tmpdir_cwd, pyproject, example):
        structure = {
            **self.EXAMPLES[example]["directory_structure"],
            "pyproject.toml": self.PYPROJECTS[pyproject],
        }
        expected_type_files = self.EXAMPLES[example]["expected_type_files"]
        jaraco.path.build(structure)
     
        build_py = get_finalized_build_py()
        outputs = get_outputs(build_py)
>       assert expected_type_files <= outputs
E       AssertionError: assert {'foo/__init_...foo/py.typed'} <= set()
E         
E         Extra items in the left set:
E         'foo/__init__.pyi'
E         'foo/py.typed'
E         'foo/namespace/foo.pyi'

/home/jgerity/repos/setuptools.git/setuptools/tests/test_build_py.py:412: AssertionError

test_type_files_included_by_default previously passed when parametrized by dont_include_package_data, because the associated metadata had the tools.setuptools typo and include-package-data was not being disabled as a result. Fixing the metadata causes the test to fail with this parameter, presumably because the typing files are considered package data.

I don't know if this means that:

1. the test is broken (in which case, the pyproject parameter should be dropped), or
2. setuptools is supposed to include typing files even when including package data has been explicitly disabled (in which case, I think this has unearthed a core bug?)

I'd like a maintainer to tell me which of those is correct, or perhaps tell me some obvious thing I've missed that means the typo in the metadata should not be corrected.

[SnoopJ]

This PR now also includes the test that was the main blocker to date. Disabling the emit() call for the new warning fails the test as expected.

[SnoopJ]

4 test failures in CI, 3 of which were the expected problem with test_type_files_included_by_default. Ruff error fixed in f443652

[SnoopJ]

Looking further into setuptools support for typing files, I can't really figure out if the typing files are supposed to be ignored when ignore-package-date = true. The documentation says:

 ``setuptools`` will attempt to include type information files
 by default in the distribution
 (``.pyi`` and ``py.typed``, as specified in :pep:`561`).

...

If you have .pyi and py.typed files in your project, but do not
wish to distribute them, you can opt out by setting
:doc:exclude-package-data </userguide/datafiles> to remove them.

But it isn't clear to me from this and reviewing the discussion in #4021 if this means that functionality is provided by enabling include-package-data when it is absent, or if it means that typing files should always be included except when explicitly excluded using exclude-package-data. The current

However, having been through this history, I see that @abravalheri was closely involved in the integration of this feature and the corresponding test.

What do you think? Are the .pyi, py.typed files are supposed to be included even when include-package-data = false?

[abravalheri]

Thank you very much @SnoopJ for having a look on this.

it means that typing files should always be included except when explicitly excluded using exclude-package-data
What do you think? Are the .pyi, py.typed files are supposed to be included even when include-package-data = false?

That would make sense to me...

@Danie-1, it seems that we oversaw a spelling mistake in #4021. Could you help us to assess the impact here?

[SnoopJ]

It sounds like setuptools should include the typing files in this case (i.e. except when the user specifically asks for them to be excluded), but from my understanding of how the typing files are implicitly added to the package data, this will be a somewhat-involved fix that doesn't really overlap well with this changeset.

Maybe the best thing to do here is to file a new bug and mark the tests parameterized by dont_include_package_data as xfail (with an explicit reference to the issue) until that issue is resolved? I have a patch for this already if you agree with that approach.

[Danie-1]

I agree that fixing the bug about type information files might take quite a bit of work. The behavior that I intended was that the type information files should be added even when include-package-data is false. I will have a look at it. That was an embarrassing typo!

[SnoopJ]

Interestingly, I am having difficulty reproducing the problem I thought was here in order to file a separate issue. In other words, in an attempted reproduction, if I set include-package-data = false, I am still getting the typing files in my resulting wheel.

It is possible that the breakage is exclusive to the test suite after all. Continuing to investigate.

[SnoopJ]

I am at this point confident that the problem is exclusive to the test suite. It is kinda tricky to figure out exactly how the tests should be tweaked to properly test this feature, but at least this isn't a new bug in setuptools

I believe the crux of the problem is that the build_py used in the test has an unpopulated packages attribute. If I manually step into the failing test and call build_py._get_pkg_data_files(appropriate_package_name) I can see the typing files being collected.

But I'm having difficulty understanding the difference in what the tests do and what a pip install does, where the two differ, and how to resolve that so that the tests are closer to the actual functionality.

[Danie-1]

I have also found that the feature seems to work in practice, but doesn't seem to work in the tests. The tests pass if I change

        build_py = get_finalized_build_py()
        outputs = get_outputs(build_py)

to

        build_py = get_finalized_build_py()
        build_py.run_command("build")
        outputs = get_outputs(build_py)

I don't really understand what all the different possible arguments to run_command do, so I am not very sure why this works, but it does seem to work.

[SnoopJ]

I have also found that the feature seems to work in practice, but doesn't seem to work in the tests. The tests pass if I change

        build_py = get_finalized_build_py()
        outputs = get_outputs(build_py)

to

        build_py = get_finalized_build_py()
        build_py.run_command("build")
        outputs = get_outputs(build_py)

Well spotted, thanks! I can confirm that this results in a passing test, as does invoking the build_py, sdist, bdist commands. Probably "build" is the best choice here unless there's a compelling reason we shouldn't be invoking commands here.

I don't really understand what all the different possible arguments to run_command do, so I am not very sure why this works, but it does seem to work.

As I understand it, it's "just" an interface to run the official distutils commands or custom ones built on top of that foundation (e.g. bdist_wheel defined by the wheel package). It's probably debatable whether a full build is necessary here, but it does have the virtue of more closely following what's done in 'real' usage, and the package isn't complex.

[SnoopJ]

Remaining failure in CI seems to be failure when provisioning the Cygwin environment, I see nothing that is actionable for me as the author of the PR.