Fix invalid config values soft-locking pip

[sepehr-rs]

Fixes #8671. Warn and skip invalid use-feature config values instead of crashing.
I also had to keep track of the invalid variables because check_default was called a lot, creating many duplicate logging messages.
Happy to adjust the approach if maintainers have a different preference.

[ichard26]

I'm confused, the PR claims to only prevent invalid use-feature values from stopping pip to function, but this seems to ignore all invalid configuration keys?

[sepehr-rs]

You're right. While the issue was reported for use-feature, the same underlying bug can be reproduced with use-deprecated, keyring-provider, exists-action, and other configuration options.

The PR title and news entry are too narrowly scoped given what the change actually does. I'll update them.

[ichard26]

I'm not sure if ignoring invalid values for all configuration options is a good idea. For example, if someone uses use-deprecated and then a future version of pip removes support for a deprecated feature, pip should fail loudly informing the user that this deprecated feature is gone. This has two benefits:

• It doesn't lead to unpredictable failures when the invocation probably crashes, possibly after tens or hundreds of lines of output
• If they don't actually need the deprecated feature anymore, they ought to remove the flag.

There are similar concerns with options like index-url, only-binary, and possibly others.

[sepehr-rs]

pip should fail loudly informing the user that this deprecated feature is gone.

I see your point, and I agree that users should be clearly informed when they're relying on a feature that no longer exists. At the same time, I think preventing users from using pip entirely, especially with the current error message, can be frustrating for users.

I think there are two possible approaches:

1. Fail with a clear, actionable error message that explains exactly what's wrong and how to fix it.
2. Show an informative warning, ignore the invalid configuration value, and continue running pip. (This is close to the current approach I took, but with a better warning.)

I'd be interested to hear which approach you think is more reasonable.

[danielhollas]

I think there are two possible approaches:

I'd say the third option is to (somehow) scope this PR down so that it only applies to the --use-feature flag, which is what the original issue asked for.

[sepehr-rs]

I'd say the third option is to (somehow) scope this PR down so that it only applies to the --use-feature flag, which is what the original issue asked for.

That's a possible solution, but I don't think it addresses the root issue. Narrowing the scope avoids the immediate problem, but we'll probably just see another issue opened later when exists-action or use-deprecated ends up soft-locking pip for the same reason.

[danielhollas]

The three options you mention are imo very much different and should be treated differently.

--exists-action seem to have a well-defined set of options so pip should error out if a wrong one is provided.

On the other hand, the list of possible options for -use-feature and -use-deprecated changes in different pip versions so it makes sense to be more lenient. Especially for use-feature, where the user is supposedly opting into a future feature so we should not punish them once the future finally arrives and the feature is stable.

[sepehr-rs]

Especially for use-feature, where the user is supposedly opting into a future feature so we should not punish them once the future finally arrives and the feature is stable.

Good point. What do you think about taking a more balanced approach? We could warn and ignore invalid values for use-feature so users aren't punished when a feature becomes stable, while still raising an error for options like use-deprecated, exists-action, and other configuration values that are expected to remain valid."

[danielhollas]

We could warn and ignore invalid values for use-feature so users aren't punished when a feature becomes stable, while still raising an error for options like use-deprecated, exists-action, and other configuration values that are expected to remain valid.

Yes, I think this would make sense.

[sepehr-rs]

I updated the code to reflect the discussion we had. Pip now warns on invalid use-feature values but throws an error on other invalid config values.

[sepehr-rs]

Since pip becomes completely unusable when a configuration value (besides use-feature) is invalid, I added the configuration directory the invalid value came from to the error message. I think this makes the error clearer and more actionable. Let me know what you think.

[sepehr-rs]

Thanks for the review!

I replaced the config file name with a link to the documentation to avoid ambiguity, and switched to self.error to make the error message more user-friendly. The only remaining issue is that optparse's error output is a bit odd:

[optparse.groups]Usage:[/]   
  pip <command> \[options]

Invalid value for configuration option 'use-deprecated': option use-deprecated: invalid choice: 'x' (choose from 'legacy-resolver', 'legacy-certs'). Check your pip configuration files for this value. See https://pip.pypa.io/en/stable/topics/configuration/#location for config file locations.

But I think that's outside the scope of this PR and would be better addressed separately.