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

Packaging cleanup, migration to full setuptools without distutils #4294

Merged

Conversation

Pierre-Sassoulas
Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas commented Apr 4, 2021

Description

Move from distutil to setuptools. (Second step toward pyproject.toml after #4295 ).

We might also want to remove doc/ and tests/ directory from the packaging ? Are you using the tests when packaging for debian @sandrotosi ?

Type of Changes

Type
🔨 Refactoring

Related Issue

Relates to #4193

@Pierre-Sassoulas Pierre-Sassoulas added Proposal 📨 Needs review 🔍 Needs to be reviewed by one or multiple more persons Documentation 📗 Maintenance Discussion or action around maintaining pylint or the dev workflow labels Apr 4, 2021
@coveralls
Copy link

coveralls commented Apr 4, 2021

Coverage Status

Coverage decreased (-0.02%) to 91.593% when pulling 95994e5 on Pierre-Sassoulas:packaging-cleanup into f46a1fc on PyCQA:master.

@Pierre-Sassoulas Pierre-Sassoulas force-pushed the packaging-cleanup branch 2 times, most recently from acce8ad to 7920451 Compare April 4, 2021 15:54
@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.8.0 milestone Apr 4, 2021
Copy link
Member

@cdce8p cdce8p left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer if the metadata where to be defined in setup.cfg instead. See here, for reference: https://setuptools.readthedocs.io/en/latest/userguide/declarative_config.html
It might also make sense to separate the build system changes from the removal of bin, debian, and man and put that in another MR.

setup.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
@Pierre-Sassoulas
Copy link
Member Author

I opened another MR to separate removal of legacy build file and setuptools migration. Regarding the move to setup.cfg I think it's going to be difficult because we have the version defined inside the package and also the test_requires and extra_requires so we need to get the value with code.

@Pierre-Sassoulas
Copy link
Member Author

Nevermind that, most of it is doable with setup.cfg. For version we have pbr and setuptools_scm that could work. I don't know if we could access the version inside pylint for pylint --version though. But the setup.py became very reasonable already.

@Pierre-Sassoulas Pierre-Sassoulas force-pushed the packaging-cleanup branch 2 times, most recently from 956743b to d86f090 Compare April 4, 2021 20:49
Copy link
Member

@cdce8p cdce8p left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some comments. Even if you add an extras_require, please don't remove the requirements_test.txt files. They are need to pin exact versions for CI testing.

setup.cfg Outdated Show resolved Hide resolved
setup.cfg Outdated Show resolved Hide resolved
setup.cfg Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
tox.ini Outdated Show resolved Hide resolved
tox.ini Outdated Show resolved Hide resolved
MANIFEST.in Outdated Show resolved Hide resolved
setup.cfg Outdated Show resolved Hide resolved
setup.cfg Show resolved Hide resolved
@Pierre-Sassoulas
Copy link
Member Author

Please don't remove the requirements_test.txt files. They are need to pin exact versions for CI testing.

I thought the the setup.cfg could also work for that goal ? Are we talking about the hash part ?

@cdce8p
Copy link
Member

cdce8p commented Apr 4, 2021

Please don't remove the requirements_test.txt files. They are need to pin exact versions for CI testing.

I thought the the setup.cfg could also work for that goal ? Are we talking about the hash part ?

Unfortunately extras_require doesn't work and shouldn't be used for that purpose. Tbh I have only seen install -e .[test] as part of legacy configs that haven't been updated yet.
The reason is, that users would expect to be able to install the extra seamlessly in their environment. That is only possible if the requirements aren't fixed as it otherwise might create dependency resolution conflicts. The idea behind the requirements file on the other hand is exactly to provide a fixed environment that can be used for testing and will always yield the same result.

@sandrotosi
Copy link
Contributor

We might also want to remove doc/ and tests/ directory from the packaging ? Are you using the tests when packaging for debian @sandrotosi ?

we do run tests and build the documentation during debian package build, but dont worry about that: i've been gradually migrating all my packages from fetching new upstream releases from PyPI to GitHub releases: it's just easier downstream (we get all the files that we need) and you can keep the PyPI release small (without extra files end users dont really need when pip installing pylint) -- thanks for asking tho!

setup.cfg Outdated Show resolved Hide resolved
setup.cfg Outdated Show resolved Hide resolved
@Pierre-Sassoulas
Copy link
Member Author

i've been gradually migrating all my packages from fetching new upstream releases from PyPI to GitHub releases: it's just easier downstream (we get all the files that we need) and you can keep the PyPI release small (without extra files end users dont really need when pip installing pylint)

Amazing, we're going to be able to make the package a lot smaller !

@cdce8p
Copy link
Member

cdce8p commented Apr 4, 2021

@Pierre-Sassoulas Regarding the failing benchmark test. The reason is that the test tries to import all modules in the root dir to check for a register function. A patch fixes that:

from unittest.mock import patch

# ....

    def test_baseline_benchmark_j1_all_checks_lots_of_files(self, benchmark):
        # ...
        with patch("os.listdir", return_value=["pylint", "tests"]):
            register_plugins(
                linter, os.path.abspath(os.path.join(os.path.dirname(__file__), "..", ".."))
            )
        # ...

@cdce8p
Copy link
Member

cdce8p commented Apr 4, 2021

You should also be able to include .isort.cfg and pytest.ini in setup.cfg.

@Pierre-Sassoulas Pierre-Sassoulas merged commit a47cb27 into pylint-dev:master Apr 5, 2021
Pierre-Sassoulas added a commit that referenced this pull request Apr 5, 2021
As it's already a dependencie but with less constraint.
See #4294
Pierre-Sassoulas added a commit that referenced this pull request Apr 5, 2021
@Pierre-Sassoulas Pierre-Sassoulas deleted the packaging-cleanup branch April 5, 2021 13:08
@Pierre-Sassoulas Pierre-Sassoulas self-assigned this Apr 5, 2021
@Pierre-Sassoulas Pierre-Sassoulas modified the milestones: 2.9.0, 2.8.3 May 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation 📗 Maintenance Discussion or action around maintaining pylint or the dev workflow Needs review 🔍 Needs to be reviewed by one or multiple more persons Proposal 📨
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants