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

When setting prefixes to empty strings, check the key names in addition to value #2401

Closed
wants to merge 1 commit into from

Conversation

hroncok
Copy link
Contributor

@hroncok hroncok commented Aug 9, 2022

In Fedora, we are currently trying to come up with a safe approach that
will make sudo pip and similar install to /usr/local rather than /usr.

After years of only patching distutils for this,
we need to adapt our change to use sysconfig.

In the past, we've tried to use a custom installation scheme,
but that has yielded many unexpected side effects for our users.

Now we try to set the defaults of {base} and {platbase} to /usr/local instead.
However, such value is not in the prefixes listed in this check in virtualenv.

Here, I adapted the code to be more robust for such changes:

  • if the value is listed in prefixes, we still set to empty
  • if the key is listed in prefix_vars, we now also set to empty

Let me know if this is acceptable to virtualenv. An alternative would be something like:

if hasattr(sysconfig, 'extra_prefixes'):
    prefixes = prefixes + tuple(sysconfig.extra_prefixes)

Or:

if hasattr(sys, 'insatllation_prefix'):
    prefixes = prefixes + (sys.insatllation_prefix,)

Thanks for contributing, make sure you address all the checklists (for details on how see development documentation)!

  • ran the linter to address style issues (tox -e fix_lint)
  • wrote descriptive pull request text
  • ensured there are test(s) validating the fix
  • added news fragment in docs/changelog folder
  • updated/extended the documentation

I plan to work on tests and docs after we agree on the best approach to fix this.

…on to value

In Fedora, we are currently trying to come up with a safe approach that
will make `sudo pip` and similar install to /usr/local rather than /usr.

After years of only patching distutils for this,
we need to adapt our change to use sysconfig.

In the past, we've tried to use a custom installation scheme,
but that has yielded many unexpected side effects for our users.

Now we try to set the defaults of {base} and {platbase} to /usr/local instead.
However, such value is not in the prefixes listed in this check in virtualenv.

Here, I adapted the code to be more robust for such changes:

 - if the value is listed in prefixes, we still set to empty
 - if the key is listed in prefix_vars, we now also set to empty
@hroncok hroncok marked this pull request as draft August 9, 2022 20:21
@hroncok
Copy link
Contributor Author

hroncok commented Aug 9, 2022

I am unsure if the check / test [email protected] - macos-12 failure is relevant.

@hroncok hroncok closed this Aug 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants