Skip to content

Cleanup vendored dependencies#25

Merged
sdispater merged 4 commits into
python-poetry:masterfrom
abn:dependency-cleanup
Apr 23, 2020
Merged

Cleanup vendored dependencies#25
sdispater merged 4 commits into
python-poetry:masterfrom
abn:dependency-cleanup

Conversation

@abn

@abn abn commented Apr 16, 2020

Copy link
Copy Markdown
Member

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
Comment thread poetry/core/__init__.py
__vendor_site__ = (Path(__file__).parent / "_vendor").as_posix()

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

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

Copy link
Copy Markdown

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.

Comment thread pyproject.toml Outdated
@abn abn force-pushed the dependency-cleanup branch from a7fdee6 to 62d3678 Compare April 23, 2020 18:30
@abn abn force-pushed the dependency-cleanup branch 2 times, most recently from 54917c8 to d8ec3ee Compare April 23, 2020 19:05
@abn abn force-pushed the dependency-cleanup branch from d8ec3ee to b24e32c Compare April 23, 2020 19:09
@abn abn requested a review from sdispater April 23, 2020 19:11

@sdispater sdispater left a comment

Copy link
Copy Markdown
Member

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

Development

Successfully merging this pull request may close these issues.

3 participants