pypa / pip

The Python package installer
https://pip.pypa.io/
MIT License
9.45k stars 3k forks source link

Automatically enable PEP 517 when `--config-settings` are used for legacy projects #11915

Closed sbidoul closed 6 months ago

sbidoul commented 1 year ago

When using --config-settings with a project that does not have a pyproject.toml, they are silently ignored.

I see two possibilities:

pradyunsg commented 1 year ago

Nice catch! My vote is toward auto-enabling pep517 for them.

sbidoul commented 1 year ago

In #11917, PEP 517 is automatically enabled when config settings are present. I decided not to warn about it because PEP 517 will become the default at some point.

sbidoul commented 1 year ago

Hmmm... actually, now that I think more about it, auto-enabling PEP 517 might be a breaking change to some extent.

Indeed CLI config settings are applied to all builds and setuptools fails when passed unknown --build-option settings . When building a project that depends on a legacy sdist, the config settings were ignored for the legacy sdist dependency. With #11917 they will be applied to both modern and legacy dependencies.

... Which makes me question the pertinence of propagating CLI config settings to dependencies. Not sure, maybe I'm overthinking it.

sbidoul commented 1 year ago

Let's make this a deprecation warning in 23.1 (#11919).

pfmoore commented 1 year ago

Agreed, that seems safer on reflection.

Regarding propagating config settings, I think this is something we need user input on, and probably discussion with backends. My instinct is that the safest option is to not propagate. Config settings only affect the directly requested packages - if you want to apply config settings to a dependency you should build a wheel using those config settings explicitly. It's not very friendly, but it has the value of being explicit and avoids the problem that if we propagate, there's no way to turn the config settings off for dependencies. I'm also not sure how this impacts the idea of config settings in requirement files. It probably doesn't (IIRC, the current state of that PR is that they don't propagate) but it's worth checking. In case it's not obvious, I'd prefer not to rush any decision on propagation, it should wait till after 23.1.

sbidoul commented 1 year ago

I'm also not sure how this impacts the idea of config settings in requirement files.

Actually I now realize that the current per-requirements PR (#11634) introduces a breaking change too by propagating CLI config settings to requirement files (my bad).

Indeed, in 23.0 and current main, CLI config settings are passed to CLI requirements and their dependencies, but not to requirements provided via a requirements file. I should have noticed sooner because I removed the unused argument in https://github.com/pypa/pip/pull/11876, but it is sinking only now...

My instinct is that the safest option is to not propagate.

It is also my intuition. Let me sleep on it...

@q0w sorry for the confusion, it looks like I may have misled you :/

q0w commented 1 year ago

So now cli and requirements config settings should not be merged?

sbidoul commented 1 year ago

So, I investigated this again. Since config settings were introduced in 22.1, there is a discrepancy in their propagation. CLI config settings are propagated to dependencies of CLI requirements, but not propagated to dependencies found in requirement files: https://github.com/pypa/pip/blob/5f12c59f69656cf682cbd20cc1eee880578bce88/src/pip/_internal/cli/req_command.py#L421-L426

Since I knew they were propagated to dependencies I wrongly assumed it was the case for requirement files too, which caused me to propose merging CLI config settings and per-requirements config settings in #11634.

To resolve this discrepancy, I now believe we should remove the propagation to dependencies, and let CLI config settings apply only to CLI requirements, while lettings per-requirement config settings in requirement files apply to individual lines only, independently of CLI requirements.

pfmoore commented 1 year ago

Thanks for looking into this. When I implemented the CLI support, this wasn't something I thought particularly about (which is probably why it's inconsistent!)

I agree with your suggestion.

One thought - does this mean that if the user sets config_settings in their pip config file, or via an environment variable, that would only affect requirements directly specified on the command line? That would be logical, but might be surprising to people who expect such a setting to be "global" (in some sense). It might be worth a documentation note.

While this isn't perfect (the workaround needed to build a dependency with specific config settings is pretty clumsy) I don't think we can do much better without feedback from actual use cases where there is a practical problem to address. There's only so far we can go based on theory.

sbidoul commented 1 year ago

I created #11941 to stop the propagation of CLI config settings to dependencies, including a documentation note.

the workaround needed to build a dependency with specific config settings is pretty clumsy

Not necessarily? If A depends on B, pip install --config-settings FOO=hello A B will be sufficient to pass config settings to B too.

pfmoore commented 1 year ago

Not necessarily? If A depends on B, pip install --config-settings FOO=hello A B will be sufficient to pass config settings to B too.

Doh. I need to send my brain in for a service. I was thinking you'd need to manually build a B wheel with the right settings. Ignore me.