#167 Merged at 32e6b70
Repository
embray
Branch
default
Repository
pypa
Branch
default
Author
  1. Erik Bray
Reviewers
Description

This is my initial attempt at fixing #207. I haven't yet tested the performance impact of performing a sort every time an entry is added to a namespace package's __path__. In most cases I think it should be negligible, especially for flat installs. But it could conceivably slow things down in extreme cases with dozens of subpackages for a namespace package all in separate eggs.

It could also break if a namespace package is imported, then sys.path is mangled in some way, then a new package belonging to the same namespace is added. So maybe relying directly on sys.path for this is not a great way to go, but it was the first thing that worked.

  • Adds the regression test for distribute issue 323 that I attached to #207. This is to ensure that any fix to #207 does not introduce another regression.

  • Fixes the original root cause of #231, and re-enables the test when the tempdir is a symlink (this does not explicitly test that /tmp itself is a symlink, but the effect is the same--only one of the path levels needs to be a symlink to reproduce this isssue)

  • Sort __path__ entries for namespace packages according to their order in sys.path. This ensures that lookups in __path__ will be the same as sys.path resolution.

    This also adds a replace argument to Distribution.insert_on meant to be used with the replace argument to WorkingSet.add. This ensures that new sys.path entries added via WorkingSet.add are inserted at the beginning, rather than appended to the end. This is necessary for consistency with the above change, and kind of makes more sense anyways. This means that if a Distribution is added to a WorkingSet, that replaces a different version of that Distribution, the new version of that Distribution will have its location first on sys.path.

Comments (10)

  1. Jason R. Coombs

    Looks great. Thanks for working through this challenging issue. I did have to make a couple of small patches, but it's now passing tests, including the new ones. This will be released as 19.4 momentarily.

  2. Kenneth Hoste

    We are hitting an issue that seems to be introduced with this change; the problem occurs with setuptools 19.4 and newer, but not with setuptools 19.3.

    A call to fixup_namespace_packages is triggering a ValueError in the sort_key inner function; full traceback and more details are available at https://github.com/hpcugent/easybuild-framework/issues/1678#issuecomment-198546677.

    It's not entirely clear to me whether this is a bug introduced here, or something we are doing wrong (my guess would be the former, since it works fine with setuptools 19.3 and earlier).

    I'll be happy to open a separate issue for this and/or provide more information if desired.

    cc @Jason R. Coombs

  3. Erik Bray author

    As I wrote in this issue the sort key can fail if a package's __path__ contains an entry that is not in sys.path for some reason. Under "normal" usage I don't know how that would happen, but would be interesting to see how it does. In any case it clearly can happen somehow so if nothing else there should be a fallback. Perhaps catch the ValueError from sys.path.index and return +inf for lack of a better ordering.

  4. Kenneth Hoste

    The problem is that the logic to determine the corresponding sys.path entry is wrong for some of our packages, because we 'flatten' the namespace, i.e. a module that is located in easybuild/easyblocks/a/atlas.py can be imported as easybuild.easyblocks.atlas. See https://github.com/hpcugent/easybuild-framework/issues/1678#issuecomment-198634657 for more details on this.

    We can work around this with some sort of a hack (adding the .egg/easybuild path in sys.path), but if this worked fine before, I feel this shouldn't be broken?

    I'm putting together a minimal testcase for this, so I can open a dedicated issue for it.