On Mon, Dec 14, 2020 at 09:54, Lee Yarwood <lyarwood@redhat.com> wrote:
On 13-12-20 16:33:39, Jeremy Stanley wrote:
On Saturday, 12 December 2020 00:12:36 CET Jeremy Stanley wrote:
On 2020-12-11 20:38:30 +0000 (+0000), Sorin Sbarnea wrote: [...]
Regarding decoupling linting from test-requirements: yes! This was already done by some when conflicts appeared. For old branches I personally do not care much even if maintainers decide to disable linting, their main benefit is on main branches. [...]
To be honest, if I had my way, test-requirements.txt files would die in a fire. Sure it's a little more work to be specific about the individual requirements for each of your testenvs in tox.ini, but the payoff is that people aren't needlessly installing bandit when they run flake8 (for example). The thing we got into the PTI about using a separate doc/requirements.txt is a nice compromise in
On 2020-12-13 14:39:58 +0100 (+0100), Luigi Toscano wrote: that
direction, at least.
Wouldn't this mean tracking requirements into two different kind of places:the main requirements.txt file, which is still going to be needed even for tests, and the tox environment definitions?
Technically we already do. The requirements.txt file contains actual runtime Python dependencies of the software (technically setup_requires in Setuptools parlance). Then we have this vague test-requirements.txt file which installs everything under the sun a test might want, including the kitchen sink. Tox doesn't reuse one virtualenv for multiple testenv definitions, it creates a separate one for each, so for example...
That isn't technically true within Nova, multiple tox envs use the {toxworkdir}/shared envdir for the virtualenv.
mypy, pep8, fast8, genconfig, genpolicy, cover, debug and bandit.
In the nova repo, if you `tox -e bandit` or `tox -e pep8` it's going to install coverage, psycopg2, PyMySQL, requests, python-barbicanclient, python-ironicclient, and a whole host of other stuff, including the entire transitive dependency set for everything in there, rather than just the one tool it needs to run.
Yup that's pointless.
I can't even run the pep8 testenv locally because to do that I apparently need a Python package named zVMCloudConnector which wants root access to create files like /lib/systemd/system/sdkserver.service and /etc/sudoers.d/sudoers-zvmsdk and /var/lib/zvmsdk/* and /etc/zvmsdk/* in my system. WHAT?!? Do nova's developers actually ever run any of this themselves?
...
Which version of that package is the pep8 env pulling in for you?
I don't see any such issues with zVMCloudConnector==1.4.1 locally on Fedora 33, tox 3.19.0, pip 20.2.2 etc.
Would you mind writing up a launchpad bug for this?
Okay, so that one's actually in requirements.txt (might be a good candidate for a separate extras in the setup.cfg instead), but seriously, it's trying to install 182 packages (present count on master) just to do a "quick" style check, and the resulting .tox created from that is 319MB in size. How is that in any way sane? If I tweak the testenv:pep8 definition in tox.ini to set deps=flake8,hacking,mypy and and usedevelop=False, and set skipsdist=True in the general tox section, it installs a total of 9 packages for a 36MB .tox directory. It's an extreme example, sure, but remember this is also happening in CI for each patch uploaded, and this setup cost is incurred every time in that context.
EWww yeah this is awful.
This is already solved in a few places in the nova repo, in different ways. One is the docs testenv, which installs doc/requirements.txt (currently 10 mostly Sphinx-related entries) instead of combining all that into test-requirements.txt too. Another is the osprofiler extra in setup.cfg allowing you to `pip install nova[osprofiler]` to get that specific dependency. Yet still another is the bindep testenv, which explicitly declares deps=bindep and so installs absolutely nothing else (save bindep's own dependencies)... or, well, it would except skipsdist got set to False by https://review.openstack.org/622972 making that testenv effectively pointless because now `tox -e bindep` has to install nova before it can tell you what packages you're missing to be able to install nova. *sigh*
So anyway, there's a lot of opportunity for improvement, and that's just in nova, I'm sure there are similar situations throughout many of our projects. Using a test-requirements.txt file as a dumping ground for every last package any tox testenv could want may be convenient for tracking things, but it's far from convenient to actually use. The main thing we risk losing is that the requirements-check job currently reports whether entries in test-requirements.txt are compatible with the global upper-constraints.txt in openstack/requirements, so extending that to check dependencies declared in tox.ini or in package extras or additional external requirements lists would be needed if we wanted to preserve that capability.
Gibi, should we track all of this in a few launchpad bugs for Nova?
Sure, we can open couple of low prio low-hanging-fruit bugs for these. Cheers, gibi
Cheers,
-- Lee Yarwood A5D1 9385 88CB 7E5F BE64 6618 BCA6 6E33 F672 2D76