remove the setup reader

[dimbleby]

fixes #9066, probably others

the more I think about it the more I reckon that the setup reader and mechanics around it are buggy beyond repair.

eg I realised today that even though the setup reader might correctly identify that some piece of metadata is not set in either setup.py or setup.cfg - it might still be completely wrong to declare that the value is missing, because this code knows nothing about PEP621-style pyproject.toml.

indeed, an approximately empty setup.py is recommended by the "how to modernize" guide as "perfectly fine".

as discussed in #9066, the python ecosystem is in a different place than it was when this was written: pretty much all of the popular packages are providing wheels these days and so the value in trying so hard to avoid building from source is much reduced.

(It might be worth someone making something along the lines of #7670 work at some point. That tries to read metadata from a PEP-621 pyproject.toml without performing a build. In the current ecosystem, if you can get all that you need that way then you likely should.)

meanwhile let's rip out the code that gives wrong answers, and just take the performance hit.

[abn]

While I agree with the sentiment and intent of this change, I'm not yet convinced yanking it outright is the best option. I'm not decided however.

The rationale for my thinking is that the mere fact these issues show up is an indication that dropping the feature might cause more issues than it solves.

I'm wondering if the best way forward is to swap the order of the 517 build and setup reader with a deprecation warning. Then drop in a later release.

[dimbleby]

I'm wondering if the best way forward is to swap the order of the 517 build and setup reader ...

• if the pep517 build succeeds then it must give all the answers and there's no point in trying the setup reader
• if the pep517 build fails then - per today's earlier pull request - poetry would later refuse to install the package anyway

so as far as I can see, once you've committed to trying the pep517 build there is never any point in doing anything else after that

[Secrus]

Could we remove only setup.py reading part and if setup.cfg doesn't provide enough info/is missing then fallback to build? I am reworking #7670 and the existing code from SetupReader could still be useful.

[dimbleby]

Not only the setup.py reader but also the code around checking whether all the available information is present is buggy. (Some things are initialized to empty lists and some to None, it's all kinds of confused about which of these things means what).

I prefer to rip off the bandage. If there is still useful code there - well it still is in version control so you can copy it easily enough. Likewise if we think that reading setup.cfg actually is valuable (I estimate that a complete setup.cfg is a rare thing and not worth worrying about), well someone can put back a working version of stuff later.

IMO getting to a cleaner sheet here is actually likely to be helpful in restoring #7670 to health.

[dimbleby]

noting that this makes the test scripts (therefore also pipelines) quite a bit slower: in combination with #9103 the effect is that the scripts are pretty much always doing pep517 metadata extraction for source packages in fixtures.

if that motivates someone to figure out how to make the pep517 metadata extraction faster, that could turn out to be a win...

alternatively most of the slowdown can be mitigated by reinstating the generated metadata files removed in #9103 (git checkout 173391b9c22 -- tests/fixtures). Then the scripts that use those fixtures don't need to do the pep517 metadata thing after all.

[abn]

For posterity. Here are the list of test cases that call SetupReader.from_directory which will, upon merging this change, start calling the isolated build logic. Identified when working on #9148. This also assumes #9148 is already merged.

This should also provide insights into what fixture is being used, in some of these test cases (the ones that do not test Poetry's setup file handling), we could simply swap out the package fixture we use as well. But in the long run, making the metadata extraction more efficient is the way to go.

test: test_fallback_can_read_setup_to_get_dependencies, args: (PosixPath('/tmp/tmps7zxucbr/SQLAlchemy-1.2.12'),), kwargs: {}
test: test_show_outdated_local_dependencies[project_with_local_dependencies-required_fixtures0], args: (PosixPath('/tmp/pytest-of-abn/pytest-68/popen-gw6/test_show_outdated_local_depen0/project_with_setup'),), kwargs: {}
test: test_run_installs_with_local_setuptools_directory, args: (PosixPath('/tmp/pytest-of-abn/pytest-68/popen-gw6/test_run_installs_with_local_s0/root/project'),), kwargs: {}
test: test_run_installs_with_local_setuptools_directory, args: (PosixPath('/tmp/pytest-of-abn/pytest-68/popen-gw6/test_run_installs_with_local_s0/root/project'),), kwargs: {}
test: test_search_for_directory_setup_with_base[non-canonical-name], args: (PosixPath('/tmp/pytest-of-abn/pytest-68/popen-gw5/test_search_for_directory_setu1/project'),), kwargs: {}
test: test_search_for_directory_setup_egg_info[non-canonical-name], args: (PosixPath('/tmp/pytest-of-abn/pytest-68/popen-gw4/test_search_for_directory_setu0/project'),), kwargs: {}
test: test_search_for_directory_setup_read_setup_with_no_dependencies, args: (PosixPath('/tmp/pytest-of-abn/pytest-68/popen-gw4/test_search_for_directory_setu2/project'),), kwargs: {}
test: test_package_partial_yank, args: (PosixPath('/tmp/tmp78r42g0t/futures-3.2.0'),), kwargs: {}
test: test_get_package_information_fallback_read_setup, args: (PosixPath('/tmp/tmp_6ix2b51/jupyter-1.0.0'),), kwargs: {}
test: test_packages_property_returns_empty_list, args: (PosixPath('/tmp/tmpd03rb__h/jupyter-1.0.0'),), kwargs: {}
test: test_get_package_retrieves_packages_with_no_hashes, args: (PosixPath('/tmp/tmpvzaj60oh/jupyter-1.0.0'),), kwargs: {}
test: test_info_setup_complex, args: (PosixPath('/tmp/pytest-of-abn/pytest-68/popen-gw0/test_info_setup_complex0/source'),), kwargs: {}
test: test_info_setup_simple, args: (PosixPath('/tmp/pytest-of-abn/pytest-68/popen-gw0/test_info_setup_simple0/source'),), kwargs: {}
test: test_info_setup_cfg, args: (PosixPath('/tmp/pytest-of-abn/pytest-68/popen-gw0/test_info_setup_cfg0/source'),), kwargs: {}
test: test_info_setup_complex_pep517_error, args: (PosixPath('/tmp/pytest-of-abn/pytest-68/popen-gw1/test_info_setup_complex_pep5170/source'),), kwargs: {}
test: test_info_from_setup_cfg, args: (PosixPath('/tmp/pytest-of-abn/pytest-68/popen-gw1/test_info_from_setup_cfg0/source'),), kwargs: {}
test: test_info_from_sdist_no_pkg_info, args: (PosixPath('/tmp/tmpork3iaul/demo-0.1.0'),), kwargs: {}
test: test_info_from_setup_py, args: (PosixPath('/tmp/pytest-of-abn/pytest-68/popen-gw1/test_info_from_setup_py0/source'),), kwargs: {}
test: test_setup_reader_read_setup_cfg, args: (PosixPath('/home/abn/workspace/python-poetry/poetry/tests/utils/fixtures/setups/with-setup-cfg'),), kwargs: {}
test: test_setup_reader_read_setup_kwargs, args: (PosixPath('/home/abn/workspace/python-poetry/poetry/tests/utils/fixtures/setups/pendulum'),), kwargs: {}
test: test_setup_reader_read_minimal_setup_cfg, args: (PosixPath('/home/abn/workspace/python-poetry/poetry/tests/utils/fixtures/setups/with-setup-cfg-minimal'),), kwargs: {}
test: test_setup_reader_read_sub_level_setup_call_with_direct_types, args: (PosixPath('/home/abn/workspace/python-poetry/poetry/tests/utils/fixtures/setups/sqlalchemy'),), kwargs: {}
test: test_setup_reader_read_first_level_setup_call_with_direct_types, args: (PosixPath('/home/abn/workspace/python-poetry/poetry/tests/utils/fixtures/setups/flask'),), kwargs: {}
test: test_setup_reader_read_minimal_setup_py, args: (PosixPath('/home/abn/workspace/python-poetry/poetry/tests/utils/fixtures/setups/minimal'),), kwargs: {}
test: test_setup_reader_read_setup_cfg_with_attr, args: (PosixPath('/home/abn/workspace/python-poetry/poetry/tests/utils/fixtures/setups/with-setup-cfg-attr'),), kwargs: {}
test: test_setup_reader_read_extras_require_with_variables, args: (PosixPath('/home/abn/workspace/python-poetry/poetry/tests/utils/fixtures/setups/extras_require_with_vars'),), kwargs: {}
test: test_setup_reader_setuptools, args: (PosixPath('/home/abn/workspace/python-poetry/poetry/tests/utils/fixtures/setups/setuptools_setup'),), kwargs: {}
test: test_setup_reader_read_setup_call_in_main, args: (PosixPath('/home/abn/workspace/python-poetry/poetry/tests/utils/fixtures/setups/pyyaml'),), kwargs: {}
test: test_setup_reader_read_first_level_setup_call_with_variables, args: (PosixPath('/home/abn/workspace/python-poetry/poetry/tests/utils/fixtures/setups/requests'),), kwargs: {}
test: test_solver_does_not_choose_from_secondary_repository_by_default, args: (PosixPath('/tmp/tmpl0xkloqs/pastel-0.1.0'),), kwargs: {}
test: test_solver_chooses_from_secondary_if_explicit, args: (PosixPath('/tmp/tmpby8q612i/pastel-0.1.0'),), kwargs: {}
test: test_solver_ignores_explicit_repo_for_transient_dependencies, args: (PosixPath('/tmp/tmpf1st_bbr/pastel-0.1.0'),), kwargs: {}
test: test_info_setup_missing_mandatory_should_trigger_pep517[version], args: (PosixPath('/tmp/pytest-of-abn/pytest-68/popen-gw0/test_info_setup_missing_mandat0/source'),), kwargs: {}
test: test_info_setup_complex_disable_build, args: (PosixPath('/tmp/pytest-of-abn/pytest-68/popen-gw0/test_info_setup_complex_disabl0/source'),), kwargs: {}
test: test_info_setup_complex_calls_script, args: (PosixPath('/tmp/pytest-of-abn/pytest-68/popen-gw1/test_info_setup_complex_calls_0/source'),), kwargs: {}
test: test_info_setup_missing_mandatory_should_trigger_pep517[name], args: (PosixPath('/tmp/pytest-of-abn/pytest-68/popen-gw1/test_info_setup_missing_mandat0/source'),), kwargs: {}
test: test_info_setup_missing_install_requires_is_fine, args: (PosixPath('/tmp/pytest-of-abn/pytest-68/popen-gw1/test_info_setup_missing_instal0/source'),), kwargs: {}
test: test_info_setup_complex_pep517_legacy, args: (PosixPath('/tmp/pytest-of-abn/pytest-68/popen-gw1/test_info_setup_complex_pep5171/source'),), kwargs: {}

[abn]

The isolated builds are improved further in #9168. We should eventually also deal with some level of caching for build environments.

[abn]

In general, I agree with the change. Couple of questions regarding the removed tests. Otherwise, lgtm.

Will need another reviewer from @python-poetry/core to also approve as this is a major change in established process for extracting package metadata.

[abn]

Approving form my side.

Will need an additional reviewer from @python-poetry/core to also approve as this is a major change in established process for extracting package metadata.

[dimbleby]

#9168 (comment) - something from that pull request does not fit well with this one

[abn]

@dimbleby #9180 should resolve the issue too.

[abn]

Approving again.

Merge pending on secondary approval from @python-poetry/core.

[github-actions]

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