Fix for Issue #419: read .egg-info files to extract version

#144 Declined
  1. Eric Larson

This fixes my use case by parsing the numpy version from the .egg-info file, replacing the version extracted from the filename.

Comments (19)

  1. Jason R. Coombs

    Setuptools provides support for both .egg-info metadata and .dist-info metadata. Is there a reason that only .egg-info is supported for this functionality?

    Also, I think a unit test is appropriate here to capture the expectation and prevent regressions.

  2. Eric Larson author

    I'm just not very experienced with setuptools, so I wanted to keep the changes as minimal as possible. But if it makes sense for dist-info as you suggest, I can also try adding that. A unit test should be doable, too, assuming there are existing examples I can work off of (haven't looked yet). I can probably look today.

  3. Eric Larson author

    Test added for egg-info and code made a bit more DRY. I didn't change the handling for dist-info because it looks like it might already do the right thing -- the dist-info tests seem to have version numbers in them already. But again I'm not too familiar with this stuff so I'm not sure.

    I also noticed that there was this bit of code:

        def version(self):
                return self._version
            except AttributeError:
                version = _version_from_file(self._get_metadata(self.PKG_INFO))
                if version is None:
                    tmpl = "Missing 'Version:' header and/or %s file"
                    raise ValueError(tmpl % self.PKG_INFO, self)
                return version

    So it looks like it should almost do what I need, but when I tried setting version = None for the .egg-info parsing -- which should have made the self.version property look at the .egg-info file later, I got some other errors in tests. So the current changeset seems like the minimal amount necessary to get things working, but I suspect there might be a cleaner solution for someone who understands the code better.

  4. Jason R. Coombs

    Thanks for the additional info. I'm quite busy right now to review it in detail, which it deserves. I'll aim to review it by the end of the month.

  5. Jason R. Coombs

    I've taken a look at this, and I have some reservations.

    My first reservation is that the test is in setuptools, but the changes are in pkg_resources. I would expect since the tests are for functionality in pkg_resources, the tests should similarly be encapsulated in the pkg_resources tests.

    My second reservation is that although 'distutils' is implicated in the test name, it's obvious that setuptools is being invoked (a pure distutils install won't generate egg-info unless patched by setuptools). I don't yet understand how the run_setup_py is somehow causing setuptools to be invoked in that clear distutils-only script. I'll have to investigate more.

    On the bright side, I did rebase just the test without the fix, and the test does fail, which is good. The failure, however, doesn't match what I would expect.

    ============================================ FAILURES ============================================
    _____________________ TestEggInfoDistutils.test_egg_base_installed_egg_info ______________________
    self = <setuptools.tests.test_egg_info.TestEggInfoDistutils object at 0x107f6e3c8>
    tmpdir_cwd = local('/Users/jaraco/Dropbox/code/main/setuptools')
    env = '/var/folders/c6/v7hnmq453xb6p2dbz1gqc6rr0000gn/T/setuptools-test.bdwwlo_d'
        def test_egg_base_installed_egg_info(self, tmpdir_cwd, env):
            environ = os.environ.copy().update(
            cmd = [
                '--home', env.paths['home'],
                '--install-lib', env.paths['lib'],
                '--install-scripts', env.paths['scripts'],
                '--install-data', env.paths['data'],
            code, data = environment.run_setup_py(
                pypath=os.pathsep.join([env.paths['lib'], str(tmpdir_cwd)]),
            if code:
                raise AssertionError(data)
            actual = self._find_egg_info_file(env.paths['lib'])
    >       assert actual == 'foo-' + self.version.replace('+', '_') + '.egg-info'
    E       assert 'foo-1.11.0.d...y3.5.egg-info' == ''
    E         - foo-1.11.0.dev0_2329eae-py3.5.egg-info
    E         ?                        ------
    E         + foo-1.11.0.dev0_2329eae.egg-info
    /Users/jaraco/Dropbox/code/main/setuptools/setuptools/tests/ AssertionError

    Is that the failure you were intending to correct in the fix?

  6. Jason R. Coombs

    Indeed, even if I run the tests in the submitted commits (at 2b21ff85422d), it fails with the same error, so it seems that (a) the test is failing with the patch and (b) the patch isn't fixing anything that's being tested.

    1. Eric Larson author

      I don't think on my system the py3.5 bit is in there. Do you have a 2.7 setup you can try?

  7. Eric Larson author

    In other words, IIRC the test failed on master but passed on my branch and I think the failures at your end are due to my changes not being 3.5-general

  8. Eric Larson author

    Changing the assertion to use .startswith should fix it, I'll do that later today or tomorrow. Regarding the appropriateness of the fix, I don't know the "right" way to fix this problem. This very well might not be it, but I'm afraid I can't fully grasp the setuptools flow to know better. I am pretty sure this fixed the problem I was having, but it has been a while since I worked on and thought about it. I should be able to double check soon, though.

  9. Eric Larson author

    Okay, updated for Py3k in latest comit. When on 220d61f (which was the last commit on my branch before I changed anything) but using my new test, I get this failure as expected (i.e., TDD):

    E           pkg_resources.VersionConflict: (foo 1.11.0.dev0-2329eae (c:\users\larsoner\appdata\local\temp\setuptools-test.zvds_ca_\lib), Requirement.parse('foo>=1.9'))

    On my PR, tests pass. Hopefully you see the same.

  10. Jason R. Coombs

    Hi Eric. I took a look at the latest test commits. I rebased them onto the parent commit you mentioned, but the tests do not fail for me (nor for Travis). You can see there that all tests pass, including the one you added, with only the two commits to the test suite.

    So it seems that in a clean environment, the test isn't yet capturing what it is meant to capture. Can you have another look?

    1. Eric Larson author

      The linked comparison has changes not just to the test file but also a fix in pkg_resources so I'm not surprised it passes. I guess my commit names were not quite all encompassing :) I think to test what you meant to test you'd need to manually create a clean branch and copy in my modified file. Can you try locally and confirm the test fails for you?

  11. Jason R. Coombs

    Hmm. I did actually rebase the two test-related commits against 18.5 in the pr-144-tests branch. That (git) branch shouldn't have the pkg_resources commit, but Github does a poor job of rendering that history, showing the commits in chronological order rather than in order of ancestry.

    If you start at this commit and click on the parent commit, it takes you to ccf6e0eb, whose parent is 2ac80cd8 (Bumped to 18.6...).

    Oh, I get it. The first 'tests' commit has changes to pkg_resources as well. I see that now reading this PR history. I'll see what I can do with that.

    1. Eric Larson author

      The easy thing to do would be to take the modified from this PR and copy it into a clean master branch. You should hopefully see it fail. Then with the changes applied to pkg_resources/ (i.e. all changes from this PR), see that it passes.

      1. Eric Larson author

        But that's just what I would do since I'm not too familiar with Mercurial (more familiar with git)

  12. Jason R. Coombs

    I've managed to separate the commits as just the test and just the changes in the pr-144-tests and pr-144 branches in the jaraco/setuptools git mirror.

    In the tests branch, I've adapted the tests to more directly invoke distutils but still capture the failure. I now understand much better what's going on here.

    The crux of the issue is that the packages are not using setuptools but are only using distutils, which has older versions of the version escaping. I'm slightly tempted to say that this use-case should not be supported and that the projects that are failing should use setuptools and not rely on distutils.

    Next I'll look at the pr-144 branch and see if the changes there are acceptable in light of my new understanding.

    1. Eric Larson author

      At the same time, though, distutils is embedding the proper version number somewhere, and setuptools currently isn't using that information. The PR makes it look in the right place for it. So I consider it a bug fix for where setuptools gets the version information.

  13. Eric Larson author

    And to put it differently, if package A uses distutils for installation, package B using setuptools will experience an error during installation on master, if I understand the problem correctly. So I don't think it's a reasonable solution to require all possible package A's (i.e. all Python packages) to use setuptools to avoid errors for package B's.