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

Deprecate legacy setup.py install when --no-binary is used #9422

Closed
wants to merge 3 commits into from

Conversation

sbidoul
Copy link
Member

@sbidoul sbidoul commented Jan 3, 2021

Towards #8102
Closes #9769

This is the first (and most difficult) leg of #8102 (comment), still with the goal of capturing user feedback about why the setup.py install code path would still be necessary, and nudging the ecosystem towards installing via wheel building.

This is still draft, but I'd appreciate feedback from @pypa/pip-committers, to get confirmation that people are on board with the general approach before going further. Individual commits should be easy to grasp.

It basically implements what was discussed around #2677 (comment) and #8102 (comment).

  • support --build-option in the install command in addition to the wheel command (as a transitory phase until the ecosystem supports PEP 517 config settings)

  • accept --build-option in requirement lines (as a transitory phase until the ecosystem supports PEP 517 config settings)

  • deprecate --install-option

  • deprecate --no-binary implying setup.py install (i.e. build wheel even if --no-binary is used)

  • add a new feature flag (always-install-via-wheel) that, when active:

    • lets a wheel build occur, even when --no-binary is used (and therefore silence the corresponding deprecation)
    • ignore and warn when --install-option is used with always-install-via-wheel
    • ignore and warn when --build-option is used without always-install-via-wheel
    • does not disable the wheel cache when --no-binary is used (Using --no-binary disables reuse of locally compiled wheels #9162)
    • errors if setup.py install is attempted for any reason

# specified on the command line, since the last option given
# to setup.py is the one that is used.
global_options = global_options + req.global_options
build_options = build_options + req.build_options
Copy link
Member Author

Choose a reason for hiding this comment

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

Does this have backward compatibility implications ?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think so?

@sbidoul sbidoul force-pushed the no-binary-setup-py-install branch 2 times, most recently from aa8ffcd to 9d761d4 Compare January 3, 2021 14:32
@sbidoul sbidoul marked this pull request as draft January 3, 2021 15:22
@BrownTruck
Copy link
Contributor

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will be eligible for code review and hopefully merging!

@BrownTruck BrownTruck added the needs rebase or merge PR has conflicts with current master label Feb 20, 2021
@pradyunsg
Copy link
Member

Closing since this is out of date and bitrotten. Please feel free to file a new PR for this! ^>^

@pradyunsg pradyunsg closed this Apr 2, 2021
@sbidoul
Copy link
Member Author

sbidoul commented Apr 2, 2021

@pradyunsg The comment above still applies, though: I'd appreciate feedback from @pypa/pip-committers, to get confirmation that people are on board with the general approach before going further. as I was basically waiting to be sure there were no objections before pushing forward.

@pradyunsg
Copy link
Member

Oh, yea, I'm 100% on board for this. :)

@pfmoore
Copy link
Member

pfmoore commented Apr 2, 2021

Sorry, it looks like I missed this - yes, I'm in favour of this.

@uranusjr
Copy link
Member

uranusjr commented Apr 2, 2021

I want to remove setup.py install entirely as soon as possible.

@sbidoul
Copy link
Member Author

sbidoul commented Apr 2, 2021

Good, thanks for the feedback. I'll get back to this soonish.

@sbidoul sbidoul reopened this Apr 2, 2021
@sbidoul sbidoul force-pushed the no-binary-setup-py-install branch from c176a49 to a4144fe Compare April 2, 2021 12:58
@sbidoul
Copy link
Member Author

sbidoul commented Apr 2, 2021

Rebased.

@xavfernandez
Copy link
Member

I'm also in favor of this and appreciate the fact that you split it in smaller PR 👍

However I do not understand why support --build-option in the install command in addition to the wheel command (as a transitory phase until the ecosystem supports PEP 517 config settings) is needed 🤔

@sbidoul
Copy link
Member Author

sbidoul commented Apr 15, 2021

However I do not understand why support --build-option in the install command in addition to the wheel command (as a transitory phase until the ecosystem supports PEP 517 config settings) is needed thinking

@xavfernandez that is a very good question and one I ask myself constantly while working on this.

My first answer is another question: why does pip wheel support --build-option ? I never use it myself but I assume it is there because there are use cases for it.

If we take for granted that it is useful for pip wheel, then adding it to pip install sounds reasonable because:

  • pip install does a wheel build, in the same way as pip wheel and passing --build-option as well as --global-option in install allows for an identical build mechanism for pip install and pip wheel
  • the setup.py install path accepts setup.py options (--global-option, --install-option) which the build+install path currently does not and I think it may be useful to allow users to pass options to the build path too
  • if we don't, users don't have any escape hatch to pass options, since PEP 517 config settings is not implemented

Now it may turn out that --build-option is actually useless. I just don't know. But I currently think it is better to have it than not, if only for symmetry between pip wheel and pip install.

@pfmoore
Copy link
Member

pfmoore commented Apr 15, 2021

if we don't, users don't have any escape hatch to pass options, since PEP 517 config settings is not implemented

Picking up on this point, we've done nothing with config_options because (from what I recall) we were waiting to see what backends did with the option, so that we could work out how best to translate --build-option. But honestly, I don't think the backends have done much with this, and even if they have it's not been co-ordinated.

So I'm starting to think that what we should do is just accept something like --build-config KEY=VAL (can be used multiple times) and build a dictionary from that. Then drop --build-option and --install-option for PEP 517 builds, with the advice that if people want to migrate, they should check with the backend how to get the same effect with config options.

But that's for later. I'm fine with @sbidoul's plan here for now.

@xavfernandez
Copy link
Member

@sbidoul My bad, concerning support --build-option in the install command in addition to the wheel command (as a transitory phase until the ecosystem supports PEP 517 config settings) for some reasons I was thinking of setup.py install command, not pip install command 🤦 .

Since pip install is likely to call bdist_wheel allowing --build-option definitely makes sense 👍

So even more in favor ^^.

I'm wondering however if gathering always-install-via-wheel feature switch (which will likely take quite a long time to be the default) and --no-binary disambiguation which could happen faster is a good idea.
I'll try to suggest an alternative plan on #9769.

So I'm starting to think that what we should do is just accept something like --build-config KEY=VAL (can be used multiple times) and build a dictionary from that.

I think we should allow for --build-config pkg_name:KEY=VAL to only pass configuration for a single package (with maybe a wildcard :all: special key to pass the value to all packages).

@github-actions github-actions bot added the needs rebase or merge PR has conflicts with current master label Aug 15, 2021
@pradyunsg
Copy link
Member

With #5771 being resolved, I think both of the following can be removed/trimmed out of this PR?

  • support --build-option in the install command in addition to the wheel command (as a transitory phase until the ecosystem supports PEP 517 config settings)

  • accept --build-option in requirement lines (as a transitory phase until the ecosystem supports PEP 517 config settings)

If there's some blocker on setuptools' end for making this possible, then consider this a moot suggestion. :)

@sbidoul
Copy link
Member Author

sbidoul commented May 3, 2022

@pradyunsg yes, that make sense. That said, I plan to put my energy in the short term on #10771 and I can come back to this end of Q3 or Q4. In the meantime if anyone wants to pick this up, I'd have no issue with that :)

@sbidoul sbidoul force-pushed the no-binary-setup-py-install branch from 5d7b2a1 to 269a773 Compare July 30, 2022 12:47
@pypa-bot pypa-bot removed the needs rebase or merge PR has conflicts with current master label Jul 30, 2022
@sbidoul
Copy link
Member Author

sbidoul commented Aug 7, 2022

If there's some blocker on setuptools' end for making this possible, then consider this a moot suggestion. :)

@pradyunsg I think the blocker is pypa/setuptools#2491

So I think I'll go for something in the vein of #9769 (comment)

@github-actions github-actions bot added the needs rebase or merge PR has conflicts with current master label Aug 12, 2022
@sbidoul sbidoul mentioned this pull request Aug 12, 2022
5 tasks
@sbidoul
Copy link
Member Author

sbidoul commented Aug 12, 2022

This continues in #11373.

@sbidoul sbidoul closed this Aug 12, 2022
@sbidoul sbidoul deleted the no-binary-setup-py-install branch August 12, 2022 16:27
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
needs rebase or merge PR has conflicts with current master
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make --no-binary build a wheel locally instead of calling setup.py install
7 participants