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

FawltyDeps fails on s3transfer package #429

Closed
mknorps opened this issue Apr 23, 2024 · 1 comment · Fixed by #440
Closed

FawltyDeps fails on s3transfer package #429

mknorps opened this issue Apr 23, 2024 · 1 comment · Fixed by #440
Assignees

Comments

@mknorps
Copy link
Collaborator

mknorps commented Apr 23, 2024

Describe the bug
FalwtyDeps fails when run on s3transfer package.

The problem is with the way dependencies are declared in setup.py.
Minimal not-working example of setup.py:

from setuptools import setup

setup(
    extras_require={
        'crt1': 'botocore>=1.33.2,<2.0a.0', # FawltyDeps fails
        'crt2': 'botocore[crt]', # FawltyDeps fails
        'crt3': ['botocore'], # FawltyDeps works
    },
)

To Reproduce
Download s3transfer package and run:

fawltydeps

from arbitrary environment.

Expected behavior
Minimal expectation: FawltyDeps informs of the problem and does not crash.
Ideal outcome: FalwtyDeps handles setup.py cases like the above.

Environment

  • OS name + version: Ubuntu 22.04 LTS
  • Version of the code: 0.15.0

Additional context

(fawltydeps-py3.10) ➜  s3transfer git:(master) fawltydeps         
WARNING:fawltydeps.limited_eval:Unable to resolve Call(func=Attribute(value=Attribute(value=Name(id='os', ctx=Load()), attr='path', ctx=Load()), attr='dirname', ctx=Load()), args=[Name(id='__file__', ctx=Load())], keywords=[]) from os.path.dirname(__file__) @ setup.py:7
WARNING:fawltydeps.limited_eval:Failed to parse assignment of 'ROOT': Call(func=Attribute(value=Attribute(value=Name(id='os', ctx=Load()), attr='path', ctx=Load()), attr='dirname', ctx=Load()), args=[Name(id='__file__', ctx=Load())], keywords=[]) from os.path.dirname(__file__) @ setup.py:7
WARNING:fawltydeps.limited_eval:Unable to resolve Call(func=Attribute(value=Name(id='re', ctx=Load()), attr='compile', ctx=Load()), args=[Constant(value='__version__ = [\'"]([0-9.]+)[\'"]')], keywords=[]) from re.compile('__version__ = [\'"]([0-9.]+)[\'"]') @ setup.py:8
WARNING:fawltydeps.limited_eval:Failed to parse assignment of 'VERSION_RE': Call(func=Attribute(value=Name(id='re', ctx=Load()), attr='compile', ctx=Load()), args=[Constant(value='__version__ = [\'"]([0-9.]+)[\'"]')], keywords=[]) from re.compile('__version__ = [\'"]([0-9.]+)[\'"]') @ setup.py:8
Traceback (most recent call last):
  File "/home/mknorps/.cache/pypoetry/virtualenvs/fawltydeps-88kNcXzg-py3.10/lib/python3.10/site-packages/pkg_resources/_vendor/packaging/requirements.py", line 35, in __init__
    parsed = _parse_requirement(requirement_string)
  File "/home/mknorps/.cache/pypoetry/virtualenvs/fawltydeps-88kNcXzg-py3.10/lib/python3.10/site-packages/pkg_resources/_vendor/packaging/_parser.py", line 64, in parse_requirement
    return _parse_requirement(Tokenizer(source, rules=DEFAULT_RULES))
  File "/home/mknorps/.cache/pypoetry/virtualenvs/fawltydeps-88kNcXzg-py3.10/lib/python3.10/site-packages/pkg_resources/_vendor/packaging/_parser.py", line 73, in _parse_requirement
    name_token = tokenizer.expect(
  File "/home/mknorps/.cache/pypoetry/virtualenvs/fawltydeps-88kNcXzg-py3.10/lib/python3.10/site-packages/pkg_resources/_vendor/packaging/_tokenizer.py", line 140, in expect
    raise self.raise_syntax_error(f"Expected {expected}")
  File "/home/mknorps/.cache/pypoetry/virtualenvs/fawltydeps-88kNcXzg-py3.10/lib/python3.10/site-packages/pkg_resources/_vendor/packaging/_tokenizer.py", line 165, in raise_syntax_error
    raise ParserSyntaxError(
pkg_resources.extern.packaging._tokenizer.ParserSyntaxError: Expected package name at the start of dependency specifier
    [
    ^

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/home/mknorps/.cache/pypoetry/virtualenvs/fawltydeps-88kNcXzg-py3.10/bin/fawltydeps", line 6, in <module>
    sys.exit(main())
  File "/tmp/FawltyDeps/fawltydeps/main.py", line 368, in main
    analysis = Analysis.create(settings, stdin)
  File "/tmp/FawltyDeps/fawltydeps/main.py", line 194, in create
    ret.undeclared_deps  # pylint: disable=pointless-statement
  File "/tmp/FawltyDeps/fawltydeps/utils.py", line 72, in wrapper
    calculated: T = method(self)
  File "/tmp/FawltyDeps/fawltydeps/main.py", line 162, in undeclared_deps
    return calculate_undeclared(self.imports, self.resolved_deps, self.settings)
  File "/tmp/FawltyDeps/fawltydeps/utils.py", line 72, in wrapper
    calculated: T = method(self)
  File "/tmp/FawltyDeps/fawltydeps/main.py", line 148, in resolved_deps
    (dep.name for dep in self.declared_deps),
  File "/tmp/FawltyDeps/fawltydeps/utils.py", line 72, in wrapper
    calculated: T = method(self)
  File "/tmp/FawltyDeps/fawltydeps/main.py", line 136, in declared_deps
    return list(
  File "/tmp/FawltyDeps/fawltydeps/extract_declared_dependencies.py", line 423, in parse_sources
    yield from parse_source(source)
  File "/tmp/FawltyDeps/fawltydeps/extract_declared_dependencies.py", line 414, in parse_source
    yield from parser.execute(src.path)
  File "/tmp/FawltyDeps/fawltydeps/extract_declared_dependencies.py", line 123, in parse_setup_py
    yield from _extract_deps_from_setup_call(node.value)  # type: ignore
  File "/tmp/FawltyDeps/fawltydeps/extract_declared_dependencies.py", line 98, in _extract_deps_from_setup_call
    yield parse_one_req(item, source)
  File "/tmp/FawltyDeps/fawltydeps/extract_declared_dependencies.py", line 50, in parse_one_req
    req = Requirement.parse(req_text)
  File "/home/mknorps/.cache/pypoetry/virtualenvs/fawltydeps-88kNcXzg-py3.10/lib/python3.10/site-packages/pkg_resources/__init__.py", line 3212, in parse
    (req,) = parse_requirements(s)
  File "/home/mknorps/.cache/pypoetry/virtualenvs/fawltydeps-88kNcXzg-py3.10/lib/python3.10/site-packages/pkg_resources/__init__.py", line 3171, in __init__
    super(Requirement, self).__init__(requirement_string)
  File "/home/mknorps/.cache/pypoetry/virtualenvs/fawltydeps-88kNcXzg-py3.10/lib/python3.10/site-packages/pkg_resources/_vendor/packaging/requirements.py", line 37, in __init__
    raise InvalidRequirement(str(e)) from e
pkg_resources.extern.packaging.requirements.InvalidRequirement: Expected package name at the start of dependency specifier
    [
    ^
@jherland jherland self-assigned this Jun 17, 2024
@jherland
Copy link
Member

I downloaded s3transfer to have a closer look. In the current version, the relevant lines from setup.py read:

    extras_require={
        'crt': 'botocore[crt]>=1.33.2,<2.0a.0',
    },

We expect the values in that dict to be a list of requrement strings, and then we iterate over that, we end up instead iterating over the single characters in the string.

To figure out what should be our desired behavior here, I installed s3transfer with the [crt] extra into a virtualenv, and it seems like setuptools does handle the str vs list discrepancy gracefully:

s3transfer ❯ pip install -e .[crt]
Obtaining file:///home/jherland/code/s3transfer
  Installing build dependencies ... done
  Checking if build backend supports build_editable ... done
  Getting requirements to build editable ... done
  Installing backend dependencies ... done
  Preparing editable metadata (pyproject.toml) ... done
Requirement already satisfied: botocore<2.0a.0,>=1.33.2 in ./.venv/lib/python3.11/site-packages (from s3transfer==0.10.1) (1.34.127)
Requirement already satisfied: jmespath<2.0.0,>=0.7.1 in ./.venv/lib/python3.11/site-packages (from botocore<2.0a.0,>=1.33.2->s3transfer==0.10.1) (1.0.1)
Requirement already satisfied: python-dateutil<3.0.0,>=2.1 in ./.venv/lib/python3.11/site-packages (from botocore<2.0a.0,>=1.33.2->s3transfer==0.10.1) (2.9.0.post0)
Requirement already satisfied: urllib3!=2.2.0,<3,>=1.25.4 in ./.venv/lib/python3.11/site-packages (from botocore<2.0a.0,>=1.33.2->s3transfer==0.10.1) (2.2.1)
Collecting awscrt==0.20.11 (from botocore[crt]<2.0a.0,>=1.33.2; extra == "crt"->s3transfer==0.10.1)
  Downloading awscrt-0.20.11-cp311-abi3-manylinux_2_17_x86_64.manylinux2014_x86_64.whl.metadata (3.4 kB)
...

This line in particular shows that botocore[crt] is interpreted as a proper requirement, even if it is not given as a list:

Collecting awscrt==0.20.11 (from botocore[crt]<2.0a.0,>=1.33.2; extra == "crt"->s3transfer==0.10.1)

So, I agree that the ideal outcome here is that FalwtyDeps handles setup.py cases like the above as if the string was in a single-element list instead.

jherland added a commit that referenced this issue Jun 17, 2024
As described in issue #429, the s3transfer package comes with setup.py
where the `extras_require` dictionary does not map to a list of strings
(extra requirements), but instead map to a string directly.

When s3transfer is installed with this extra (e.g. `pip install
s3transfer[crt]`), the setuptools machinery handles this variation
gracefully, i.e. it interprets the string as if it was a single-element
list containing the same string.

An experiment reveals that the same interpretation occurs if a string is
passed directly in the `install_requires` argument.

This commit teaches FawltyDeps to do the same.

Also, if parse_one_req() fails to parse a requirement string, catch that
exception (likely an `InvalidRequirement` or some other subclass of
ValueError), and convert it into a DependencyParsingError, which is
handled more gracefully (i.e. print a warning message and continue).
jherland added a commit that referenced this issue Jul 8, 2024
As described in issue #429, the s3transfer package comes with setup.py
where the `extras_require` dictionary does not map to a list of strings
(extra requirements), but instead map to a string directly.

When s3transfer is installed with this extra (e.g. `pip install
s3transfer[crt]`), the setuptools machinery handles this variation
gracefully, i.e. it interprets the string as if it was a single-element
list containing the same string.

An experiment reveals that the same interpretation occurs if a string is
passed directly in the `install_requires` argument.

This commit teaches FawltyDeps to do the same.

Also, if parse_one_req() fails to parse a requirement string, catch that
exception (likely an `InvalidRequirement` or some other subclass of
ValueError), and convert it into a DependencyParsingError, which is
handled more gracefully (i.e. print a warning message and continue).
jherland added a commit that referenced this issue Jul 8, 2024
As described in issue #429, the s3transfer package comes with setup.py
where the `extras_require` dictionary does not map to a list of strings
(extra requirements), but instead map to a string directly.

When s3transfer is installed with this extra (e.g. `pip install
s3transfer[crt]`), the setuptools machinery handles this variation
gracefully, i.e. it interprets the string as if it was a single-element
list containing the same string.

An experiment reveals that the same interpretation occurs if a string is
passed directly in the `install_requires` argument.

This commit teaches FawltyDeps to do the same.

Also, if parse_one_req() fails to parse a requirement string, catch that
exception (likely an `InvalidRequirement` or some other subclass of
ValueError), and convert it into a DependencyParsingError, which is
handled more gracefully (i.e. print a warning message and continue).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants