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

Setuptools should always use its own implementation when upgrading #315

Closed
bb-migration opened this Issue Dec 28, 2014 · 28 comments

Comments

Projects
None yet
1 participant
@bb-migration

bb-migration commented Dec 28, 2014

Originally reported by: jaraco (Bitbucket: jaraco, GitHub: jaraco)


In #314, Tres reported that Setuptools cannot upgrade itself if new metadata is present which references functionality not available in the version being upgraded. It should be possible and straightforward to always use the code from the version being installed to install that version.


@bb-migration

This comment has been minimized.

bb-migration commented Dec 28, 2014

Original comment by jaraco (Bitbucket: jaraco, GitHub: jaraco):


Remove setuptools modules from sys.modules before invoking setup script. Fixes #315.

@bb-migration

This comment has been minimized.

bb-migration commented Dec 28, 2014

Original comment by jaraco (Bitbucket: jaraco, GitHub: jaraco):


Committed technique has other issues:

Traceback (most recent call last):
File "setup.py", line 219, in <module>
  dist = setuptools.setup(**setup_params)
File "/opt/python/3.2.5/lib/python3.2/distutils/core.py", line 109, in setup
  _setup_distribution = dist = klass(attrs)
File "/home/travis/build/jaraco/setuptools/setuptools/dist.py", line 266, in __init__
  self.fetch_build_eggs(attrs['setup_requires'])
File "/home/travis/build/jaraco/setuptools/setuptools/dist.py", line 312, in fetch_build_eggs
  replace_conflicting=True,
File "/home/travis/build/jaraco/setuptools/pkg_resources/__init__.py", line 752, in resolve
  dist = best[req.key] = env.best_match(req, ws, installer)
File "/home/travis/build/jaraco/setuptools/pkg_resources/__init__.py", line 1004, in best_match
  return self.obtain(req, installer)
File "/home/travis/build/jaraco/setuptools/pkg_resources/__init__.py", line 1016, in obtain
  return installer(requirement)
File "/home/travis/build/jaraco/setuptools/setuptools/dist.py", line 379, in fetch_build_egg
  return cmd.easy_install(req)
File "/home/travis/build/jaraco/setuptools/setuptools/command/easy_install.py", line 618, in easy_install
  return self.install_item(spec, dist.location, tmpdir, deps)
File "/home/travis/build/jaraco/setuptools/setuptools/command/easy_install.py", line 648, in install_item
  dists = self.install_eggs(spec, download, tmpdir)
File "/home/travis/build/jaraco/setuptools/setuptools/command/easy_install.py", line 838, in install_eggs
  return self.build_and_install(setup_script, setup_base)
File "/home/travis/build/jaraco/setuptools/setuptools/command/easy_install.py", line 1060, in build_and_install
  self.run_setup(setup_script, setup_base, args)
File "/home/travis/build/jaraco/setuptools/setuptools/command/easy_install.py", line 1045, in run_setup
  run_setup(setup_script, args)
File "/home/travis/build/jaraco/setuptools/setuptools/sandbox.py", line 176, in run_setup
  DirectorySandbox(setup_dir).run(runner)
File "/home/travis/build/jaraco/setuptools/setuptools/sandbox.py", line 207, in run
  return func()
File "/home/travis/build/jaraco/setuptools/setuptools/sandbox.py", line 175, in runner
  _execfile(setup_script, ns)
File "/home/travis/build/jaraco/setuptools/setuptools/sandbox.py", line 44, in _execfile
  exec(code, globals, locals)
File "/tmp/easy_install-fl2ohe/pytest-runner-2.1.2/setup.py", line 34, in <module>
  def _gen_console_scripts():
File "/opt/python/3.2.5/lib/python3.2/distutils/core.py", line 148, in setup
  dist.run_commands()
File "/opt/python/3.2.5/lib/python3.2/distutils/dist.py", line 917, in run_commands
  self.run_command(cmd)
File "/opt/python/3.2.5/lib/python3.2/distutils/dist.py", line 934, in run_command
  cmd_obj = self.get_command_obj(command)
File "/opt/python/3.2.5/lib/python3.2/distutils/dist.py", line 809, in get_command_obj
  cmd_obj = self.command_obj[command] = klass(self)
File "/home/travis/build/jaraco/setuptools/setuptools/__init__.py", line 125, in __init__
  _Command.__init__(self,dist)
File "/opt/python/3.2.5/lib/python3.2/distutils/cmd.py", line 57, in __init__
  raise TypeError("dist must be a Distribution instance")
TypeError: dist must be a Distribution instance
@bb-migration

This comment has been minimized.

bb-migration commented Dec 28, 2014

Original comment by jaraco (Bitbucket: jaraco, GitHub: jaraco):


That error emerged in this Travis-CI run.

I believe what's happening is that by removing the setuptools modules from sys.modules (and re-importing it in a different context), the monkey-patched version in distutils no longer reflects the Distribution used by the current context.

@bb-migration

This comment has been minimized.

bb-migration commented Dec 28, 2014

Original comment by jaraco (Bitbucket: jaraco, GitHub: jaraco):


I tried also removing distutils from sys.modules during a sandbox install, but that has triggered different test failures.

@bb-migration

This comment has been minimized.

bb-migration commented Dec 29, 2014

Original comment by jaraco (Bitbucket: jaraco, GitHub: jaraco):


Disable purging of distutils/setuptools during sandbox operations. Ref #315.

@bb-migration

This comment has been minimized.

bb-migration commented Dec 29, 2014

Original comment by pje (Bitbucket: pje, GitHub: pje):


Per your request for comment on the other issue, the way I handled these kinds of issues was to avoid them occurring in the first place, or, failing that, have installation fail with a message indicating a requirement to install setuptools in a separate process.

i.e., this is why ez_setup always refused to update an existing version of setuptools. IOW, I never had any reasonably straightforward solutions for this problem, and so designed around it. Good luck. ;-)

@bb-migration

This comment has been minimized.

bb-migration commented Dec 29, 2014

Original comment by jaraco (Bitbucket: jaraco, GitHub: jaraco):


The two tests that are failing with the use of sandbox.hide_modules are failing because those tests also monkey-patch distutils when calling reset_start_stop_context, and the use of hide_modules is suppressing that monkey patch. Those tests are:

  • test_easy_install:TestUserInstallTest.test_setup_requires and
  • test_easy_install:TestSetupRequires.test_setup_requires_honors_fetch_params.

Interestingly, TestSetupRequires.test_setup_requires_overrides_version_conflict also calls reset_start_stop_context, but that test doesn't appear to be affected by hide_modules.

@bb-migration

This comment has been minimized.

bb-migration commented Dec 29, 2014

Original comment by jaraco (Bitbucket: jaraco, GitHub: jaraco):


On further examination, I think my last comment was wrong. The use of hide_modules should not break reset_start_stop_context, but would actually obviate it.

For TestUserInstallTest.test_setup_requires, I now think the issue is the patching of easy_install_pkg.__file__ (in setUp). I'm still unclear on why setup_test_requires_honors_fetch_params is failing.

@bb-migration

This comment has been minimized.

bb-migration commented Dec 30, 2014

Original comment by jaraco (Bitbucket: jaraco, GitHub: jaraco):


The issue with the second failure in setup_test_requires_honors_fetch_params appears to be that the DistutilsError is not being caught by distutils, probably because the error being thrown is an instance of the sandboxed import of distutils, whereas the except clause is looking for the unsandboxed manifestation.

@bb-migration

This comment has been minimized.

bb-migration commented Dec 30, 2014

Original comment by jaraco (Bitbucket: jaraco, GitHub: jaraco):


The fact that invocations of easy_install, which in turn may make calls to sandbox.run_setup but whose exceptions are expected to bubble up and be caught by distutils means it will not be straightforward to isolate those calls, as much as sandbox.run_setup would like to do so.

@bb-migration

This comment has been minimized.

bb-migration commented Dec 30, 2014

Original comment by jaraco (Bitbucket: jaraco, GitHub: jaraco):


Catch, save, and restore any exceptions across the save_modules context. This corrects the latter of the two test failures. Ref #315.

@bb-migration

This comment has been minimized.

bb-migration commented Dec 30, 2014

Original comment by jaraco (Bitbucket: jaraco, GitHub: jaraco):


Backed out changeset: 40cc1fbecb1c
Restores purging of distutils/setuptools during sandbox operations. Ref #315.

@bb-migration

This comment has been minimized.

bb-migration commented Dec 30, 2014

Original comment by jaraco (Bitbucket: jaraco, GitHub: jaraco):


Make sure to monkey-patch the easy_install module in the setup context. Fixes the other former test failure. Ref #315.

@bb-migration

This comment has been minimized.

bb-migration commented Dec 30, 2014

Original comment by jaraco (Bitbucket: jaraco, GitHub: jaraco):


I've pushed a couple of fixes and now tests are passing again. I'm not particularly happy with the fixes. They seemed very heavy-handed and somewhat convoluted. I wouldn't expect myself to understand them in a few months, much less anybody else. For now, these changes are in the issue-315 bookmark.

@bb-migration

This comment has been minimized.

bb-migration commented Dec 30, 2014

Original comment by jaraco (Bitbucket: jaraco, GitHub: jaraco):


Despite my reservations, I've released this as 10.0.

@bb-migration

This comment has been minimized.

bb-migration commented Jan 20, 2015

Original comment by embray (Bitbucket: embray, GitHub: embray):


Is there some way you could summarize what finally actually changed here? This broke a lot of stuff for me. Lots of dist must be a Distribution instance and such and such must be a subclass of Command, etc. I can probably find a workaround on my end, but it's just hard to follow the changes that happened here.

@bb-migration

This comment has been minimized.

bb-migration commented Jan 20, 2015

Original comment by jaraco (Bitbucket: jaraco, GitHub: jaraco):


The main change is that in sandbox.py in setup_context, hide_setuptools is invoked and in save_modules, exceptions are serialized (if possible) to transform any exceptions from the sandboxed versions to their natural form.

The intention here is that anything invoked in a 'setup_context' will re-import setuptools and distutils for the duration of that context, allowing the latest setuptools call to be used during an upgrade of setuptools. It's meant to provide subprocess-like isolation without actually invoking the command in a separate process.

@bb-migration

This comment has been minimized.

bb-migration commented Jan 20, 2015

Original comment by embray (Bitbucket: embray, GitHub: embray):


Thanks Jason, I think that explains what's happening then. This seems to only be a problem in my tests, where I'm testing setuptools plugins using sandbox.run_setup. The modules being tested are imported before run_setup is called, and they import parts of setuptools. So they're having setuptools swept out from under the rug. For the time being I might just try monkey-patching hide_setuptools in my tests.

@bb-migration

This comment has been minimized.

bb-migration commented Jan 20, 2015

Original comment by embray (Bitbucket: embray, GitHub: embray):


(Or, ideally, it might be nice to have an option to disable this behavior, which in general is very nice but for testing purposes is a bit problematic :/)

@bb-migration

This comment has been minimized.

bb-migration commented Jan 20, 2015

Original comment by jaraco (Bitbucket: jaraco, GitHub: jaraco):


So they're having setuptools swept out from under the rug.

That's the intention, but it also attempts to serialize and re-hydrate (using pickle) any exceptions so that those exceptions can be trapped by services outside of the context. I'm surprised the re-hydration doesn't allow your tests to run as expected. Can you point me to a use case that fails, because setuptools uses run_setup in its own unit tests (and in upgrades) with success.

@bb-migration

This comment has been minimized.

bb-migration commented Jan 21, 2015

Original comment by richardipsum (Bitbucket: richardipsum, GitHub: richardipsum):


Thanks for fixing this! With this issue resolved would a pull request to add the setup_requires metadata back be welcome?

@bb-migration

This comment has been minimized.

bb-migration commented Jan 21, 2015

Original comment by jaraco (Bitbucket: jaraco, GitHub: jaraco):


@richardipsum That was my intention after it stabilized. A PR isn't necessary but would help serve as a reminder. Thanks.

@bb-migration

This comment has been minimized.

bb-migration commented Jan 22, 2015

Original comment by embray (Bitbucket: embray, GitHub: embray):


Can you point me to a use case that fails, because setuptools uses run_setup in its own unit tests (and in upgrades) with success.

This came up specifically in the test suite for astropy-helpers (I suspect it might also affect the d2to1 tests but I haven't checked yet. I won't send you digging around, but this test should give the basic idea of the kind of tests we're talking about.

The reason this broke for me is that the astropy-helpers test suite goes through loops to test setup.py calls (using run_setup) that use code in the astropy_helpers package, and specifically using modules already imported from that package, in the same process, so that the test suite can collect coverage data. So those modules are typically already imported (and usually relying on setuptools; e.g. setup.py commands) before run_setup is called. Then, when the setuptools switcheroo happens they get confused.

In any case, I have a workaround for now that I'm pretty okay with. Having an option in run_setup/setup_context to disable this functionality might be nice, but it isn't essential. Thanks otherwise for fixing this understandably hairy issue.

@bb-migration

This comment has been minimized.

bb-migration commented Jan 29, 2015

Original comment by stefanholek (Bitbucket: stefanholek, GitHub: stefanholek):


Since hide_setuptools has gone in, I have tests that hang the interpreter instead of raising setuptools.sandbox.SandboxViolation. This is presumably because the necessary stack frames have disappeared. Once I remove hide_setuptools, SandboxViolations are propagated again.

To reproduce with the setuptools test suite, I looked at TestUserInstallTest.test_setup_requires in test_easy_install.py. First I needed to make sure user installs are enabled for the test. Then I removed the fix for 318 to see the SandboxViolation triggered. Instead I got a hang. Without hide_setuptools the exception is raised as expected.

Apply the patch below to try for yourself:

#!diff

diff -r 9db269094f1d setuptools/dist.py
--- a/setuptools/dist.py        Mon Jan 26 08:35:43 2015 -0500
+++ b/setuptools/dist.py        Thu Jan 29 20:25:58 2015 +0100
@@ -371,7 +371,7 @@
             cmd = easy_install(
                 dist, args=["x"], install_dir=install_dir, exclude_scripts=True,
                 always_copy=False, build_directory=None, editable=False,
-                upgrade=False, multi_version=True, no_report=True, user=False
+                upgrade=False, multi_version=True, no_report=True,
             )
             cmd.ensure_finalized()
             self._egg_fetcher = cmd
diff -r 9db269094f1d setuptools/tests/test_easy_install.py
--- a/setuptools/tests/test_easy_install.py     Mon Jan 26 08:35:43 2015 -0500
+++ b/setuptools/tests/test_easy_install.py     Thu Jan 29 20:25:58 2015 +0100
@@ -233,6 +233,7 @@
         appear as user-installed.
         """
         with self.orig_context(*args, **kwargs):
+            site.ENABLE_USER_SITE = True
             import setuptools.command.easy_install as ei
             ei.__file__ = site.USER_SITE
             yield

I'd prefer to continue without hide_setuptools as it seems brittle and prone to causing hard to debug problems. Especially in tests that themselves use fixtures and mocks I can see it wreaking all kinds of havoc.

Thank you.

@bb-migration

This comment has been minimized.

bb-migration commented Jan 30, 2015

Original comment by embray (Bitbucket: embray, GitHub: embray):


I wonder if it could just be enabled for the case it was meant to enable, of setuptools upgrading itself.

@bb-migration

This comment has been minimized.

bb-migration commented Feb 6, 2015

Original comment by vaab (Bitbucket: vaab, GitHub: vaab):


d2to1 appears to be broken by this. It nailed down to::

https://bitbucket.org/pypa/setuptools/commits/f29ad5f78ef72f1712e29acfea92a6f5ca47b428

Which brings me here. The case is that d2to1 that does also some black magic will get lost and can't hook up properly to setuptools.

#!python

  File "/usr/lib/python2.7/distutils/dist.py", line 287, in __init__
    self.finalize_options()
  File "/home/vaab/dev/python/_sandbox/clean2/src/setuptools/setuptools/dist.py", line 326, in finalize_options
    ep.load()(self, ep.name, value)
  File "/home/vaab/dev/python/kids/kids.sh/.eggs/d2to1-0.2.11-py2.7.egg/d2to1/core.py", line 70, in d2to1
    _Distribution.finalize_options(dist)
TypeError: unbound method finalize_options() must be called with Distribution instance as first argument (got Distribution instance instead)

Travis just changed their VM and upgraded setuptools to version >= 10, and I have 10+ project that relies on d2to1 that are breaking at each build for installing their dependency. (this means, that this change breaks python setup.py install on any package that has a dependency that uses d2to1). That's nasty.

On Travis, I'm forcing now to downgrade setuptools before testing the packaging of my packets. But I can't prevent failure for people installing my package that depends on mine.

@bb-migration

This comment has been minimized.

bb-migration commented Feb 6, 2015

Original comment by jaraco (Bitbucket: jaraco, GitHub: jaraco):


I've created #343 to track the emergent issue stemming from this change.

@bb-migration

This comment has been minimized.

bb-migration commented Feb 6, 2015

Original comment by embray (Bitbucket: embray, GitHub: embray):


Ah, I hadn't even tried it with d2to1 yet since where we use it locally we haven't upgraded setuptools to >= 8.0 yet for our clients. I wonder if OpenStack has encountered this though, since pbr is more or less a fork of d2to1. I think probably worse comes to worse the workaround I used in astropy-helpers to just monkey-patch hide_setuptools to be a no-op should work for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment