Skip to content

Help debugging entry_points failure #1174

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

Closed
ghost opened this issue Oct 15, 2017 · 14 comments
Closed

Help debugging entry_points failure #1174

ghost opened this issue Oct 15, 2017 · 14 comments

Comments

@ghost
Copy link

ghost commented Oct 15, 2017

While trying to get pip to use the new build_meta, I ran into a problem: setuptools loaded the entry points from the global environment rather than the temporary build environment: see here. How does pkg_resources discover entry_points and how can I override it?

@ghost
Copy link
Author

ghost commented Oct 15, 2017

This was a stupid issue. Closing.

@ghost ghost closed this as completed Oct 15, 2017
@benoit-pierre
Copy link
Member

benoit-pierre commented Oct 15, 2017

Entry points are loaded from active distributions. On import, pkg_resources will activate all distributions found in sys.path. The problem with using concurrent.futures is that, like multiprocessing, the main module will be imported, resulting in a lot of modules being imported before you get a chance to fix the path.

I took a stab at monkey-patching the backend to use pip._internal.utils.misc.call_subprocess instead, and fix a few things, see here: https://travis-ci.org/benoit-pierre/pip/jobs/288266999

There are some issues with the method used by setuptools' backend to run setup.py:

  • the environment handling of locals/globals is wrong (that's why test_install_with_hacked_egg_info is failing)
  • when a package use if __name__ == '__main__': to make its setup.py importable (like setuptools itself does), it does not work (so pip installing setuptools itself from source does not work)

Both those issues are resolved with the following patch:

 setuptools/build_meta.py | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git i/setuptools/build_meta.py w/setuptools/build_meta.py
index 54f2987b..f89f82d6 100644
--- i/setuptools/build_meta.py
+++ w/setuptools/build_meta.py
@@ -65,10 +65,11 @@ def _run_setup(setup_script='setup.py'):
     # Note that we can reuse our build directory between calls
     # Correctness comes first, then optimization later
     __file__ = setup_script
+    __name__ = '__main__'
     f = getattr(tokenize, 'open', open)(__file__)
     code = f.read().replace('\\r\\n', '\\n')
     f.close()
-    exec(compile(code, __file__, 'exec'))
+    exec(compile(code, __file__, 'exec'), locals())
 
 
 def _fix_config(config_settings):

@benoit-pierre
Copy link
Member

I don't think it was stupid :)

IMHO, calling _initialize_master_working_set is not the right fix: with an unvendored pip, you'll still get a lot of modules loaded before you patch the path, even if you reinitialize pkg_resources working set, you'll still possibly end up using the wrong setuptools module, no?

@ghost
Copy link
Author

ghost commented Oct 15, 2017

So there are several points:

  1. I changed the backend to modify sys.path before spawning the subprocess. pkg_resources was still loaded from the wrong location, so I had to re-initialize.
  2. I'll include that fix in my PR.

@ghost
Copy link
Author

ghost commented Oct 15, 2017

Look here.

@ghost
Copy link
Author

ghost commented Oct 15, 2017

you'll still possibly end up using the wrong setuptools module, no?

Also, yes but no. We (pip) control all modules that are loaded before we modify the environment. I don't understand why pkg_resources is even loaded, because I thought pip used a vendored version.

@benoit-pierre
Copy link
Member

benoit-pierre commented Oct 15, 2017

Unfortunately, a lot of distributions like to mess with vendored stuff (e.g. setuptools and pip, among others), and this mean that setuptools/pkg_resources will be loaded before you get a chance to patch the path.

It's the same issue with setuptools handling of setup_requires: requiring a different version of setuptools itself does not work in all cases.

@ghost
Copy link
Author

ghost commented Oct 15, 2017

Here's what I'm doing:

I patch the path here:
https://github.com/xoviat/pip/blob/14b36efcddb4309b0a03795268be5ac687d15959/src/pip/_internal/backend.py#L63

Then I spawn the subprocess here:
https://github.com/xoviat/pip/blob/14b36efcddb4309b0a03795268be5ac687d15959/src/pip/_internal/backend.py#L118

The environment is the same, so sys.path has to be modified before the subprocess interpreter even starts.

@ghost
Copy link
Author

ghost commented Oct 15, 2017

To be clear, I:

  1. Modify the environment of the parent
  2. Spawn the child inside of the new environment (provably, because of the debugging information)

Why is this not working?

@benoit-pierre
Copy link
Member

I'll see if I can find a way to check, using the code I had made to fix unvendored tests. Did you take a look at my other changes?

@ghost
Copy link
Author

ghost commented Oct 15, 2017

Which changes are you talking about? To be clear, I think reloading pkg_resources is not necessarily the "correct" fix, but it:

  1. Has no downsides that we know of, since everything that's loaded before is controlled by pypa.
  2. Gets us to a (partial but workable) initial implementation without having to completely refactor pip.

@ghost
Copy link
Author

ghost commented Oct 15, 2017

Thanks. My plan of action is from here is to:

  1. Override sys.stdout and sys.stderr to print to a file that the parent controls.
  2. Fix the various errors that come from reading METADATA rather than dist_info.
  3. Other things that I can't think of yet.

Also, the subprocess might be the better solution. But there's just something satisfying about not having to touch the command line and letting Python handle the problem. And also it gives me an excuse to get concurrent.futures into pip.

@ghost
Copy link
Author

ghost commented Oct 15, 2017

Actually, on second thought, you have the better solution. I'll just merge your changes in.

This issue was closed.
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

No branches or pull requests

1 participant