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.

Sign up for GitHub

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

Jump to bottom

Deprecate --install-options #11359

Merged
merged 5 commits into from
Oct 6, 2022
Merged

Deprecate --install-options #11359

merged 5 commits into from
Oct 6, 2022

Conversation

sbidoul
Copy link
Member

@sbidoul sbidoul commented Aug 7, 2022 •
edited
Loading

Towards #8102 and #11358.

This PR does the following:

  • It deprecates --install-options.
  • It refactors check_install_build_global into check_legacy_setup_py_options that is called after parsing the requirements from CLI options and req files. This allows us to reason about these options in the context of the command for which they are used, and also avoids a side effect (disallow_binaries) in the req file parser
  • It clarifies the WARNING message Disabling all use of wheels due to the use of --build-option / --global-option / --install-option. into Implying --no-binary=:all: due to the presence of --build-option / --global-option / --install-option. Consider using --config-settings for more flexibility.. This is clearer and will help understanding when we disentangle --no-binary. Fixes Confusing message when pip wheel is used with --(global|build)-option #8406.

TODO

  • tests

@sbidoul sbidoul force-pushed the deprecate-install-options branch 2 times, most recently from 3980c84 to a124c41 Compare August 7, 2022 16:38
@sbidoul sbidoul added this to the 22.3 milestone Aug 7, 2022
@uranusjr uranusjr changed the title Deprecate --install-option Deprecate --install-options Aug 7, 2022
@sbidoul sbidoul mentioned this pull request Aug 9, 2022
Open
1 task
@sbidoul sbidoul force-pushed the deprecate-install-options branch 2 times, most recently from 8625949 to 5958129 Compare August 12, 2022 11:52
@sbidoul sbidoul mentioned this pull request Aug 12, 2022
Closed
5 tasks
@sbidoul
Copy link
Member Author

sbidoul commented Sep 16, 2022

Added a couple of tests and rebased. This is ready to review and merge.

@sbidoul sbidoul added the type: deprecation Related to deprecation / removal. label Sep 17, 2022
@sbidoul sbidoul mentioned this pull request Sep 17, 2022
Closed
@sbidoul
Copy link
Member Author

sbidoul commented Sep 25, 2022 •
edited
Loading

I've rebased this to check how it combines with the other related deprecations.

Using --install-option triggers all of them which is a little bit noisy. The messages are coherent in my view, but I may be biased due to having spent way too much time on this. So I very much welcome feedback by other testers. cc/ @pypa/pip-committers

sbidoul
sbidoul commented Sep 25, 2022
View reviewed changes
src/pip/_internal/req/req_install.py Outdated
"--install-option is deprecated because "
"it forces pip to use the 'setup.py install' "
"command which is itself deprecated."
),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be precise, --install-option is ignored when --use-pep517 is enabled, so this deprecation may now appear when it is actually ignored.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I added some logic to print a warning instead when pep 517 is enabled.

@sbidoul
Pass global_options when building for install
77c8301 
Before 'build' was never called in presence of
install_option/global_option/build_option.
Now that it can build in such cases, pass these options as
well, for consistency with the wheel command.
@sbidoul
Warn when --install-option is ignored
fe58a42 
Due to building with pep 517.
uranusjr
uranusjr reviewed Sep 26, 2022
View reviewed changes
src/pip/_internal/commands/install.py
Comment on lines -449 to +455
global_options=[],
global_options=global_options,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm this one’s unexpected. Why was it []…?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, the only way to reach this point when --global-option is present, is to --use-pep517.
By passing it here, we trigger the warning in wheel_builder that global options are ignored when doing pep 517 builds.
This should help nudging users to use --config-settings.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess what I’m wondering is mostly, was it a bug global_options wasn’t passed previously?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was it a bug

Yes and no. The only "bug" was that --global-option was ignored without warning in pep 517 mode.

@sbidoul sbidoul mentioned this pull request Oct 6, 2022
Closed
@pradyunsg pradyunsg merged commit 7311c82 into pypa:main Oct 6, 2022
@sbidoul sbidoul deleted the deprecate-install-options branch October 6, 2022 09:13
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 22, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@uranusjr uranusjr uranusjr left review comments

@pradyunsg pradyunsg pradyunsg approved these changes

Assignees
No one assigned
Labels
type: deprecation Related to deprecation / removal.
Projects
None yet
Milestone
22.3
Development

Successfully merging this pull request may close these issues.

Confusing message when pip wheel is used with --(global|build)-option
3 participants
@sbidoul @uranusjr @pradyunsg