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

Optionally load C dependencies based on platform #4142

Closed

Conversation

mattlong
Copy link

@mattlong mattlong commented Nov 29, 2016

PR for #4098.

I'm not thrilled importing from pip.compat to detect windows since it requires a del statement to leave no trace. Could of course duplicate the same logic to avoid the import + del, but that seems even worse.

@mattlong mattlong force-pushed the optionally-disable-c-dependencies branch from 06c6342 to 45849c7 Compare November 29, 2016 03:57
@xavfernandez
Copy link
Member

Changes to _vendor files should either be upstreamed to the vendored project (in this case requests) or use patches in https://github.com/pypa/pip/tree/master/tasks/vendoring.

@mattlong
Copy link
Author

mattlong commented Dec 6, 2016

@xavfernandez The instructions for updating vendor patches seem incomplete. First, the pathlib package needs to be installed in addition to invoke. After that I'm still getting an error when running invoke vendoring.update as show below. Please advise.

[vendoring.update] Rewriting all imports related to vendored libs
Traceback (most recent call last):
  File "/Users/xxx/.virtualenvs/pip/bin/invoke", line 11, in <module>
    sys.exit(program.run())
  File "/Users/xxx/.virtualenvs/pip/lib/python2.7/site-packages/invoke/program.py", line 274, in run
    self.execute()
  File "/Users/xxx/.virtualenvs/pip/lib/python2.7/site-packages/invoke/program.py", line 389, in execute
    executor.execute(*self.tasks)
  File "/Users/xxx/.virtualenvs/pip/lib/python2.7/site-packages/invoke/executor.py", line 113, in execute
    result = call.task(*args, **call.kwargs)
  File "/Users/xxx/.virtualenvs/pip/lib/python2.7/site-packages/invoke/tasks.py", line 111, in __call__
    result = self.body(*args, **kwargs)
  File "/Users/xxx/projects/pip/tasks/vendoring/__init__.py", line 128, in main
    vendor(ctx, vendor_dir)
  File "/Users/xxx/projects/pip/tasks/vendoring/__init__.py", line 111, in vendor
    rewrite_file_imports(item, vendored_libs)
  File "/Users/xxx/projects/pip/tasks/vendoring/__init__.py", line 58, in rewrite_file_imports
    text = item.read_text()
AttributeError: 'PosixPath' object has no attribute 'read_text'

@mattlong
Copy link
Author

mattlong commented Dec 6, 2016

@dstufft (or anyone else) What are your thoughts on this PR?

@xavfernandez
Copy link
Member

@xavfernandez The instructions for updating vendor patches seem incomplete. First, the pathlib package needs to be installed in addition to invoke.

The vendoring.update task is expected to be run with python 3.5

@mattlong mattlong force-pushed the optionally-disable-c-dependencies branch from 45849c7 to d2de289 Compare December 6, 2016 22:39
@xavfernandez xavfernandez added this to the 9.1 milestone Dec 30, 2016
@pradyunsg
Copy link
Member

@xavfernandez Anything outstanding on this?

@xavfernandez
Copy link
Member

This will need rebasing after #4342

@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 eligible for code review and hopefully merging!

@BrownTruck BrownTruck added the needs rebase or merge PR has conflicts with current master label Apr 1, 2017
@alex
Copy link
Member

alex commented Jul 8, 2017

Thing that just occurred to me: If pip starts supporting loading C-extensions again, we probably ought to make the openssl_version in the User-Agent reflect the version loaded by cryptography, not the one loaded by Python's ssl module.

@pradyunsg
Copy link
Member

Removing from the 10.0 milestone since the linked issue is already in the milestone. :)

@amp343
Copy link

amp343 commented Aug 25, 2017

@mattlong I had opened a similar pr to this (#4612) without seeing yours first, but would be happy to consolidate around this one instead if you're still interested in working on it.

It looks like this one needs:

  • a rebase
  • a news entry in order to pass CI (a news/4098.bugfix file that describes the fix)

I would also recommend updating the readme here: https://github.com/pypa/pip/blame/master/pip/_vendor/README.rst#L100 to reflect this change to the treatment of c dependencies.

@lock
Copy link

lock bot commented Jun 2, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Jun 2, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jun 2, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
auto-locked Outdated issues that have been locked by automation needs rebase or merge PR has conflicts with current master type: enhancement Improvements to functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants