Fail gracefully at parsing setup.py with no deps.#117
Fail gracefully at parsing setup.py with no deps.#117pombredanne merged 10 commits intoaboutcode-org:mainfrom
Conversation
|
@bennati please update your branch |
|
@TG1999 I rebased the branch, is this what you were asking? |
|
@bennati see https://github.com/nexB/python-inspector/pull/117/checks?check_run_id=10682725875: which is very minor, setting it to pass. Also the failing tests are from pycodestyle, can you run |
|
@bennati I also had this issue yesterday. Running these included the following sign-off message in my earlier commit: |
ec067a5 to
9b7b489
Compare
|
I ran |
|
@bennati just run |
|
@bennati please add some tests to showcase the changes you have done fixes the said issue |
Raise an exception in 'get_requirements_from_python_manifest' only if dependencies are defined in 'setup.py'. Signed-off-by: Bennati, Stefano <stefano.bennati@here.com>
Signed-off-by: Bennati, Stefano <stefano.bennati@here.com>
Signed-off-by: Bennati, Stefano <stefano.bennati@here.com>
|
Is there anything left before we can merge this? |
|
@bennati please remove the unrelated changes and update your tests. |
Signed-off-by: Bennati, Stefano <stefano.bennati@here.com>
Signed-off-by: Bennati, Stefano <stefano.bennati@here.com>
pombredanne
left a comment
There was a problem hiding this comment.
Thanks!
I wonder if we could do better wrt. using the AST parsing instead?
Signed-off-by: Bennati, Stefano <stefano.bennati@here.com>
Signed-off-by: Bennati, Stefano <stefano.bennati@here.com>
Signed-off-by: Bennati, Stefano <stefano.bennati@here.com>
|
Can we please merge this one?? It's been open for more than a month now. |
|
@bennati Thaanks for using an AST... I reviewed this in details Friday and did not post a comment then. I realized that there may be a lot of similarities (and opportunities to reuse code and update the code as needed) with https://github.com/nexB/python-inspector/blob/main/src/_packagedcode/pypi_setup_py.py that's already doing some of the basic features and is used in https://github.com/nexB/python-inspector/blob/9e765ec50ed4c30eca20ef56686c9bcbe07fb89e/src/_packagedcode/pypi.py#L1557 ? |
|
@pombredanne Great that there is some code already for processing the AST. I'd be happy to refactor this code to maximize reuse, as part of a separate PR. Can we merge this? Thanks! |
| if len(install_requires) != 0: | ||
| raise Exception( | ||
| f"Unable to collect setup.py dependencies securely: {setup_py_location}" | ||
| ) |
There was a problem hiding this comment.
You are missing one case which is when there are install_requires that are processed dynamically commonly either loaded from a requirements file (that we do not handle and needs "insecure loading") or from a variable (that we handle and that would be regression See https://github.com/nexB/python-inspector/blob/9e765ec50ed4c30eca20ef56686c9bcbe07fb89e/src/_packagedcode/pypi_setup_py.py#L155 )
But I ran a test with this setup.py and it works, so there is no issue:
from distutils.core import setup
reqs = ["boolean.py"]
setup(
name="foo",
version="0.3.0",
install_requires=reqs,
)
pombredanne
left a comment
There was a problem hiding this comment.
LGTM!
(just a reminder for the future to make the commit messages a bit more explicit or to squash them).
I am merging this now. Thanks!
|
@pombredanne Thanks! Yeah I meant to squash the commits, next time I'll improve the commit messages. |
Raise an exception in 'get_requirements_from_python_manifest' only if dependencies are defined in 'setup.py'.
Signed-off-by: Stefano Bennati stefano.bennati@here.com
Mentions: #116