jaraco / pytest-checkdocs

MIT License
6 stars 5 forks source link

drop troublesome pep517.meta usage #12

Closed FFY00 closed 2 years ago

FFY00 commented 3 years ago

pep517.meta.load will use pep517.meta.build, which tries to provision a environment with pip. This is causing lots of issues in downstreams, namely Arch Linux. This patch replaces the pep517.meta usage with pypa/build, which can do the same job without provisioning a isolated environment.

The drawback is that the PEP 517 dependencies will now have to be present, which I feel is very reasonable. Given a environment with all the required packages, we should not be adding a dependency on the network to be able to run this plugin.

Signed-off-by: Filipe Laíns lains@riseup.net

FFY00 commented 3 years ago

Seems like the CI is failing with a mypy issue.

jaraco commented 3 years ago

The drawback is that the PEP 517 dependencies will now have to be present, which I feel is very reasonable.

I don't feel that's reasonable at all. The whole point of PEP 517 is that build dependencies don't have to be present to run/test the project. I explicitly remove setuptools from my canonical environments to test that projects can install without it as an implicit dependency. Other projects may declare their own build dependencies and there's no way this project can know in advance what those are, and we definitely don't want to get into the habit of always installing all of the build dependencies as runtime dependencies in order for the tests to run.

In fact, it's plausible, even likely, that adding build dependencies to the runtime environment could alter the runtime environment - perhaps masking missing runtime dependencies or altering the presence of plugins. It's undesirable for build dependencies to be present in the installed environment.

Given a environment with all the required packages, we should not be adding a dependency on the network to be able to run this plugin.

I agree with this sentiment in theory, but the solution can't be to always implicitly require the build dependencies and leave it up to the environment orchestration to always make build dependencies present. Instead, we need to increase the sophistication of the metadata build tools (like pep517.meta or similar) to support building with existing dependencies when they're already present. Setuptools, for example, has had this functionality for a long time in setup_requires and tests_require - the project would declare the dependencies and if those dependencies were already present, it would re-use them, but otherwise it would load them into an environment.

PEP 517 does support building outside of an isolated build environment, but that approach requires that the caller take responsibility for pre-configuring the environment. Whatever solution is employed here needs to at least shoulder that responsibility if it doesn't rely on PEP 517 to do it and not require downstream consumers to pre-install build requirements.

FFY00 commented 3 years ago

I agree with this sentiment in theory, but the solution can't be to always implicitly require the build dependencies and leave it up to the environment orchestration to always make build dependencies present. Instead, we need to increase the sophistication of the metadata build tools (like pep517.meta or similar) to support building with existing dependencies when they're already present.

This is incredibly difficult to do reliably, at least efficiently.

PEP 517 does support building outside of an isolated build environment, but that approach requires that the caller take responsibility for pre-configuring the environment. Whatever solution is employed here needs to at least shoulder that responsibility if it doesn't rely on PEP 517 to do it and not require downstream consumers to pre-install build requirements.

Okay. That makes sense, I guess I have a different POV, but I understand where you are coming from here. As a middle ground, would it be possibly to add an option to this plugin to switch this behavior? This way people that are provisioning their own environment, such as Arch Linux, could choose to use that environment, while having the behavior you describe by default.

jaraco commented 3 years ago

As a middle ground, would it be possibly to add an option to this plugin to switch this behavior?

Perhaps. Could you use PIP_USE_ISOLATED_BUILD=no (or similar, please verify exact syntax) to override PIP's behavior? Note that you would need to set TOX_TESTENV_PASSENV=PIP_USE_ISOLATED_BUILD to inform tox to pass the value.

jaraco commented 2 years ago

As discussed above, the current implementation is the best available, and the proposal presented has some serious downsides I'd like to avoid (namely, requiring a lot of complexity and implicit requirements by the consumer). I'd really like to see a replacement for pep517.meta instead of pushing the responsibility on the consumer.

I suspect that existing PIP options may provide a path to achieve the desired ends. I'm happy to revisit if anyone wishes to revive the discussion.