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

Solicit isolation setting from the environment. #766

Closed
wants to merge 2 commits into from

Conversation

jaraco
Copy link
Member

@jaraco jaraco commented Mar 31, 2024

Closes #556.

…e environment, allowing builders to affect the isolation behavior. Closes pypa#556.
@jaraco
Copy link
Member Author

jaraco commented Mar 31, 2024

I considered adding tests for this behavior, but decided against it, because it would require adding more dynamism to the behavior (processing the environment variable late) or otherwise refreshing the import of util to solicit a different value. I'm considering a test that merely asserts that when inspected in a subprocess with the environment variable set, the default is False.

I'm a little worried about the choice of "BUILD_ENVIRONMENT" being a fairly general phrase and colliding with some other purpose. I'm aiming for {application}_{setting}, but because the application is "build", that's pretty generic. Perhaps better would be PYPROJECT_BUILD_ENVIRONMENT (leveraging the name of the CLI app)?

@layday
Copy link
Member

layday commented Mar 31, 2024

This has the appearance of 'spooky action at a distance' - I'm not in favour of modifying library functions through environment variables, which take effect globally. An env var-controlled function can have multiple consumers stepping on each other's toes.

In the original ticket, you say:

I could imagine that each consumer of project_wheel_metadata could expose an environment variable or other configuration mechanism to alter the behavior, but probably build should do that instead.

I think this is something that should in fact be done by the consumer and not by build.

@jaraco
Copy link
Member Author

jaraco commented Mar 31, 2024

I think this is something that should in fact be done by the consumer and not by build.

Since there is more than one consumer (at least pytest-checkdocs and jaraco.packaging), requiring the setting to be solicited by the consumer requires all of the consumers to coordinate or provide locally-unique settings. Do we really want JARACO_PACKAGING_BUILD_ENVIRONMENT and PYTEST_CHECKDOCS_BUILD_ENVIRONMENT (and one more for each consumer) and then ask downstream builders to configure each of these settings based on which tools that happen to be in use? Alternatively, each of these consumers could potentially solicit BUILD_ENVIRONMENT or PYPROJECT_BUILD_ENVIRONMENT and then honor than when calling project_build_metadata, which would have the same effect as this change, except that other consumers would not get that behavior and there would be no good authority or forum for coordinating the use of this common setting (maybe jaraco.packaging implements one value but pytest-checkdocs likes the other, or maybe an issue is reported and one package changes the name but the other doesn't, or maybe both manage to coordinate and make the change at the same time but downstream they get different versions with alternate behaviors). The whole point of filing the issue was to point out that there is a common concern that's bound to the signature of this function.

This behavior doesn't feel that much more spooky than other environmental build flags like PIP_NO_INDEX or PIP_FIND_LINKS, which drastically change the behavior at a distance.

Even the consumers of this API don't have a great way to solicit this behavior, so they solicit it through environment variables. One is a sphinx plugin, the other a pytest plugin, so it's not obvious what direct method they could have to influence the behavior, and there's almost certainly no common solution across those two CLI surfaces (i.e. no argument you could pass to sphinx build and pytest that would signal to use non-isolated builds whenever metadata is needed).

To my knowledge, these are just two consumers of this API, and these are possibly the only consumers of this API, and I do maintain both, so maybe I shouldn't be concerned about the coordination concerns, but my hope was to solicit the behavior where it's needed but make it reachable by the builders without having to plumb it through every consumer.

At the very least, I'd like to consider adding a helper function that consumer could use thus:

from build.util import project_wheel_metadata, infer_isolation

project_wheel_metadata(..., isolated=infer_isolation())

So that the logic behind the environment variable and inference could be consolidated. It still would require each consumer to opt into the inference, and it still would be spooky action at a distance, but I don't see a good alternative.

In light of those concerns, does that shift your opinion at all?

@jaraco
Copy link
Member Author

jaraco commented Mar 31, 2024

I'm pretty sure the test failures are unrelated to my changes.

@layday
Copy link
Member

layday commented Mar 31, 2024

This behavior doesn't feel that much more spooky than other environmental build flags like PIP_NO_INDEX or PIP_FIND_LINKS, which drastically change the behavior at a distance.

These env vars (are only advertised to) work with the pip CLI and not with its programmatic interface. I've no expectation that a hypothetical pip API would be parameterisable via env vars. It sounds like you're planning on invoking the build API in a subprocess (and are comparing it with pip as a result), but there's no reason why that should always be the case. I don't think project_wheel_metadata is very useful in its current form, but opening it up to manipulation 'at a distance' probably isn't the solution.

@jaraco
Copy link
Member Author

jaraco commented Mar 31, 2024

It sounds like maybe there's no path forward with build. My next best solution is to provide this behavior through another library. As much trouble as it's been to contribute this functionality to pep517 and now to build, I'm conceding defeat. If build is refusing to address the concerns of downstream builders, we'll need yet another layer to address concerns. I guess I'll tackle the problem in jaraco.packaging.

@jaraco jaraco closed this Mar 31, 2024
@jaraco jaraco deleted the feature/build-isolation-from-env branch March 31, 2024 22:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Downstream packager unable to indicate "no isolation" for offline builds
2 participants