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

checking via zest-releaser fails if setuptools is provided by a buildout #35

Open
woutervh opened this issue Nov 25, 2014 · 9 comments
Open

Comments

@woutervh
Copy link

my use-case, I have a "tools"-buildout that installs zest.releaser, fabric, and check-manifest,...
this buildout is bootstrapped from a clean python that deliberatetly has no setuptools installed,
(so it can never conflict with a buildout-provided setuptools).

the check_manifest takes sys.executable as default python
see https://github.com/mgedmin/check-manifest/blob/master/check_manifest.py#L572
while this is correct, the executable looses its sys.path,

A similar issue was fixed in zest.releaser, see
zestsoftware/zest.releaser#57
https://github.com/zestsoftware/zest.releaser/pull/58/files

as far as I see, the same solution ban be re-used.

@mgedmin
Copy link
Owner

mgedmin commented Nov 25, 2014

I don't quite understand the situation. Any chance for me to reproduce the issue?

@mgedmin
Copy link
Owner

mgedmin commented Nov 30, 2014

Steps to reproduce:

  1. cd /tmp && mkdir example && cd example
  2. virtualenv -p /usr/bin/python2.7 --no-setuptools /tmp/cleanenv
  3. wget http://downloads.buildout.org/2/bootstrap.py
  4. vi buildout.cfg:
[buildout]
parts = check-manifest

[check-manifest]
recipe = zc.recipe.egg
  1. /tmp/cleanenv/bin/python bootstrap.py
  2. bin/buildout
  3. bin/check-manifest ~/src/check-manifest

And here's the error:

building an sdist
['/tmp/cleanenv/bin/python', 'setup.py', 'sdist', '-d', '/tmp/check-manifest-nLHfR_-sdist'] failed (status 1):
Traceback (most recent call last):
  File "setup.py", line 3, in <module>
    from setuptools import setup
ImportError: No module named setuptools

@mgedmin
Copy link
Owner

mgedmin commented Nov 30, 2014

Actually the interesting zest.releaser commit is zestsoftware/zest.releaser@48ab8433.

@mgedmin
Copy link
Owner

mgedmin commented Nov 30, 2014

So the thing is: check-manifest itself doesn't depend on setuptools. Therefore when you use it with zc.buildout, the bin/check-manifest script won't have setuptools added to sys.path. Therefore setting PYTHONPATH for the child process won't help.

It would likely help your use case, where check-manifest is invoked from zest.releaser, since zest.releaser depend on setuptools. I'd appreciate it if you could test this patch:

diff --git a/check_manifest.py b/check_manifest.py
index 75c251e..43773aa 100755
--- a/check_manifest.py
+++ b/check_manifest.py
@@ -127,9 +127,15 @@ def run(command, encoding=None):
     """
     if not encoding:
         encoding = locale.getpreferredencoding()
+    if command and command[0] == sys.executable:
+        # Workaround for zc.buildout, bootstrapped from a Python that lacks
+        # setuptools (see https://github.com/mgedmin/check-manifest/issues/35)
+        env = {'PYTHONPATH': os.pathsep.join(sys.path)}
+    else:
+        env = None
     try:
         pipe = subprocess.Popen(command, stdout=subprocess.PIPE,
-                                stderr=subprocess.STDOUT)
+                                stderr=subprocess.STDOUT, env=env)
     except OSError as e:
         raise Failure("could not run %s: %s" % (command, e))
     output = pipe.communicate()[0].decode(encoding)

In the end, I'm not sure what to do. The process of making source distributions is python setup.py sdist. If your setup.py needs setuptools, you should run it with a python that has setuptools. check-manifest lets you specify a python command to use with the -p command-line flag. In other words, I don't think there's need to add a workaround to the main check-manifest script. But the zest.releaser plugin, OTOH, has no way to supply a different Python command, and if this workaround fixes your use case, I'll commit it.

@mgedmin
Copy link
Owner

mgedmin commented Dec 12, 2014

@woutervh any chance you can test this patch? I'd like to make a check-manifest release soon.

@woutervh
Copy link
Author

I confirm this patch fixes the problem. Tested with python 2.7.10 and check_manifest 0.25

@mgedmin
Copy link
Owner

mgedmin commented Oct 30, 2015

Thank you!

@mgedmin
Copy link
Owner

mgedmin commented Oct 30, 2015

Hm. Should I also add a dependency on setuptools in my setup.py?

mgedmin added a commit that referenced this issue Oct 30, 2015
Fixes the example in
#35 (comment)
mgedmin added a commit that referenced this issue Nov 26, 2015
It introduced bug #56.

This reverts commit 396fda8 (and
90b8961).

This reopens bug #35.
mgedmin added a commit that referenced this issue Nov 26, 2015
Explain why the fix for #35 was reverted: because reverting it fixes #56.
@mgedmin
Copy link
Owner

mgedmin commented Nov 26, 2015

I had to revert the zc.buildout support hack because it breaks other things -- see #56.

@mgedmin mgedmin reopened this Nov 26, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants