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

Disallow explicitly passing install-location-related arguments in --install-options #7309

Open
chrahunt opened this issue Nov 7, 2019 · 9 comments

Comments

@chrahunt
Copy link
Member

@chrahunt chrahunt commented Nov 7, 2019

What's the problem this feature will solve?

Currently the --install-option parameter allows passing arbitrary arguments to setup.py for packages using the legacy-installation path. If any options/flags that control install location are passed, such as:

  • --exec-prefix
  • --home
  • --install-base
  • --install-data
  • --install-headers
  • --install-lib
  • --install-platlib
  • --install-purelib
  • --install-scripts
  • --prefix
  • --root
  • --user

then it causes issues. Not every package uses setup.py, so in the general case this causes legacy packages to be installed with the the scheme provided in --install-option and PEP 517/wheel packages to be installed with the scheme pip is aware of. This has already hit two users (#7253, #7240).

Pip knows how to use scheme information properly for PEP 517, wheel, and legacy installs. Enforcing that pip is the only one passing this kind of information will make the user experience more consistent.

Describe the solution you'd like

Explicitly fail pip install if --install-option includes any of the above arguments, whether provided on the command-line or in a requirements file.

In addition to the consistent user experience, this would also allow pip to pass the more explicit

  • --install-{purelib,platlib,headers,scripts,data} (for install)
  • --install-dir, --script-dir (for install -e)

to setup.py instead of the current --prefix, --home, --root, --user, and --install-headers.

This may also be required to implement PEP 582 (__pypackages__) since doing legacy installs to a plain lib directory isn't possible with the current arguments being passed to setup.py, and passing an explicit --install-lib would break any usages of the other arguments in --install-option.

IMO this could be implemented without deprecation because the current behavior demonstrated in #7253 and #7240 causes failures in non-obvious ways, and the remediation is simple in most cases - just pass the parameters to pip instead.

Alternative Solutions

  • do nothing - as we accumulate more features that conflict with these arguments being passed to --install-option we will get more support tickets. These will taper off eventually as people stop using these flags in order to install non-legacy packages. We eventually forget that the user can pass these options, and implement new features without worrying about it.
  • automatically determine a scheme based on --install-option - this defeats the purpose of having pip-level arguments
  • only fail the install if the scheme in --install-option conflicts with the one specified at the pip level - this is more friendly, but I feel that it would also be more confusing and add to the implementation burden in pip (reconciling --install-option across different packages in a requirements file)

Additional context

  • #7253 - user already having a related issue
  • #7240 - user already having a related issue
  • #6052 - without executing the current proposal the refactoring mentioned here gets much more difficult, since we must continue to pass --home, --prefix, --user, and --root to setup.py
@pradyunsg

This comment has been minimized.

Copy link
Member

@pradyunsg pradyunsg commented Nov 7, 2019

Sounds like a good idea to me.

We'd have to go through a regular deprecation cycle here but I don't see any reason to not do this.

@chrahunt

This comment has been minimized.

Copy link
Member Author

@chrahunt chrahunt commented Nov 7, 2019

I would argue that this is an extension of #6606/fix for #7240, and thus doesn't require a deprecation cycle. As mentioned above, the current behavior when using these flags is broken unless all builds are going through the legacy code path.

@pradyunsg

This comment has been minimized.

Copy link
Member

@pradyunsg pradyunsg commented Nov 7, 2019

nods

I don't think this is an extension of #6606 -- that PR changed when pip used PEP 517, which is something we're still working on. This, however, affects everyone on the legacy code path. We'd be removing functionality that user might depend on directly, so we should go through the deprecation cycle for it.

FWIW, I now know that basically any change in pip's build logic will affect someone (i.e. break their deployment / CI pipeline) and they're usually not happy about it. With the power of retrospect, I feel we should've gone for at least short deprecation cycle (1 release) for #6606 since that was a (minor) change in end-user-affecting behavior as well.

@chrahunt

This comment has been minimized.

Copy link
Member Author

@chrahunt chrahunt commented Nov 7, 2019

OK. After thinking on it more, we can communicate the workaround in the deprecation notice. Hopefully that means currently impacted users will not have to search the issue tracker when some packages are missing, so I don't mind that approach as much as I did initially. It might even be worth including in a fix release for 19.3? Then we could also close #7240.

@pradyunsg

This comment has been minimized.

Copy link
Member

@pradyunsg pradyunsg commented Nov 7, 2019

Given that we've already closed that issue (which is a form of communication on our release processes), I'm inclined to say no -- but it's really an RM's decision so... pinging @xavfernandez! :P

FWIW, I'm in no hurry to close #7240 but yes, even the deprecation messaging is enough to justify closing it.

@xavfernandez

This comment has been minimized.

Copy link
Contributor

@xavfernandez xavfernandez commented Nov 7, 2019

I'm on board with forbidding conflicting options in --install-options and also in favor of a deprecation period.
Concerning a 19.3.2 release, it seems strange to make a patch release just to include a warning... but if everyone agrees (and if the patch is clean), why not :)

@chrahunt

This comment has been minimized.

Copy link
Member Author

@chrahunt chrahunt commented Nov 7, 2019

Note that #7002 (unreleased) will also break some usages of these options in --install-option, since we only take into account the --user, --prefix, --target, and --root options explicitly passed to pip when determining whether to do a user install.

@chrahunt

This comment has been minimized.

Copy link
Member Author

@chrahunt chrahunt commented Nov 9, 2019

After talking this over with @pradyunsg, our conclusions are:

  1. we should maintain the 6 month deprecation period for this (so if we make a patch release or get this in 20.0, we remove in 20.2)
  2. #6606 and #7002 should not be reverted, and we can treat the deprecation notice as a warning that things may not work as expected (with wheels and automatically-determined user-mode installation)
  3. in the interim, we may be able to distill the options passed to --install-option into input for our scheme (potentially blocking if there is some conflict between --install-option and arguments passed directly to pip), which would let us press forward with other refactoring that requires pip to have more control over the setuptools arguments
@chrahunt

This comment has been minimized.

Copy link
Member Author

@chrahunt chrahunt commented Dec 1, 2019

For the more fine-grained option handling, I'm thinking the following:

install options mapped to pip options

if provided in any --install-option, these options would be compared against all other requirements' --install-options and the options provided to pip itself. If the pip equivalents are unset then they would be set to the provided options and the options removed from --install-option. If the pip equivalents are set to a different value, then we raise an error. If other requirements have a different value or do not have the same value in their --install-options then we raise an error.

  1. --home maps to --target
  2. --prefix maps to --prefix
  3. --root maps to --root
  4. --user maps to --user

install options that override scheme values directly

If provided in any --install-option, these options would be compared against all other requirements' --install-options. If the other requirements have a different value or do not have the same value in their --install-option then we raise an error. If there are any pip-level location options then we raise an error. Otherwise we explicitly set the corresponding field on our install scheme after we generate the default one and remove the argument from --install-option.

  1. --install-data
  2. --install-headers
  3. --install-lib
  4. --install-platlib
  5. --install-purelib
  6. --install-scripts

unsupported install options

These options, which do not have an equivalent in the scheme that we install packages to, are unsupported and if they are provided in any --install-options then we raise an error

  1. --exec-prefix
  2. --install-base

I think this is the minimum (in terms of user-facing impact) we can do while still being able to move forward on creating an explicit scheme for install.

Just to reiterate why this is important: once the above is in place, we would be able to do the following:

  1. reconcile input arguments
  2. map input arguments to a Scheme object in InstallCommand.run
  3. use that Scheme to instantiate a working set
  4. use that working set for finding installed distributions during dependency resolution (cleaning up a lot of Resolver/RequirementPreparer/InstallRequirement in the process)
  5. use that scheme as the basis for installation (and conflicting requirement uninstallation), which should fix several bugs (#1056, #4296, #5595)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.