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

Cleanup vendored dependencies #25

Merged
merged 4 commits into from
Apr 23, 2020

Conversation

abn
Copy link
Member

@abn abn commented Apr 16, 2020

This pull request contains 3 relevant changes to package vendoring.

  1. Remove unused vendored dependencies.
  2. Debundle compatibility only dependencies. These are only required in smaller scenarios and will be dropped by poetry 1.3 releases anyway.
  3. Prototype sys.path injection I am not sure how reliable this is, feedback would be nice. Considering poetry use case in isolated, I do not expect this to have any issues. This would also mean that distribution package maintainers can debundle extremely easily by dropping the _vendor direcotry and removing the injection. We could go further by testing if the directory exists before pre-pending.

Relates-to: #2346

@abn abn changed the title Dependency cleanup Cleanup vendored dependencies Apr 16, 2020
@abn abn requested a review from a team April 16, 2020 19:31
@abn abn force-pushed the dependency-cleanup branch 2 times, most recently from 5168c49 to a7fdee6 Compare April 16, 2020 23:49
__vendor_site__ = (Path(__file__).parent / "_vendor").as_posix()

if __vendor_site__ not in sys.path:
sys.path.insert(0, __vendor_site__)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I agree it would be better in theory, I wonder if this won't conflict with what we do in Poetry, and if the vendors of poetry-core will not take precedence.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question, I want to do a bit more testing on this one actually. We could move it into poetry/__init__.py. Although, I am not a 100% on the ordering.

One thing I was wondering if we could provide a kind of proxy instead that can be smart enough to pick system one if the vendor site is not available. That would mean it would achieve what we want, while maintaining explicit imports.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For posterity, if the sys path option does need to be replaced later. This can be an alternative.

import sys
import importlib

from poetry.core.utils._compat import Path

__all__ = []
__vendored__ = {}

try:
    # poor developer's requirements parser
    with open(Path(__file__).parent / "vendor.txt") as f:
        for line in f.readlines():
            name, version = line.split("==")
        __vendored__[name] = version
        module_name = "poetry.core._vendor.{}".format(name)

        try:
            locals()[name] = importlib.import_module(module_name)
        except ImportError:
            locals()[name] = importlib.import_module(name)

        sys.modules[module_name] = locals()[name]
        __all__.append(name)
except (FileNotFoundError, ValueError, IndexError) as e:
    pass

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At that point, it might make sense to just switch to using pip's internal helper: pip._vendor:vendored.

pyproject.toml Outdated Show resolved Hide resolved
@abn abn force-pushed the dependency-cleanup branch 2 times, most recently from 54917c8 to d8ec3ee Compare April 23, 2020 19:05
Copy link
Member

@sdispater sdispater left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me 👍

@sdispater sdispater merged commit e1c6978 into python-poetry:master Apr 23, 2020
@sdispater sdispater mentioned this pull request Apr 24, 2020
@abn abn deleted the dependency-cleanup branch June 30, 2020 08:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants