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

Replace pipdeptree with python-inspector #5645

Merged
merged 2 commits into from
Aug 24, 2022

Conversation

pombredanne
Copy link
Contributor

This PR replaces pipdeptree with python-inspector to resolve Python packages dependencies found in requirement files.

python-inspector can resolve dependencies for any target Python version and OS (and not only the one running the tool).
In this integration in ORT, it replaces pipdeptree pretty much in place as python-inspector implements a similar output data structure by design to ease the integration.

Reference: https://github.com/nexB/python-inspector
Reference: #4637
Reference: #3671
Signed-off-by: Tushar Goel [email protected]
Signed-off-by: Philippe Ombredanne [email protected]

@pombredanne pombredanne requested a review from a team as a code owner August 11, 2022 15:06
@TG1999 TG1999 force-pushed the pyinsp branch 2 times, most recently from bed38d9 to d9a2574 Compare August 11, 2022 15:12
@@ -28,6 +28,9 @@ ARG CRT_FILES=""
# Set this to the ScanCode version to use.
ARG SCANCODE_VERSION="30.1.0"

# Set this to the Python Inspector version to use.
Copy link
Member

Choose a reason for hiding this comment

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

Commit message: Please add the details from the PR description to the commit message as well, to make sure it is recorded in the Git log. Also, please prefix the title with "Pip: Replace ...".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Member

@mnonnenmacher mnonnenmacher Aug 23, 2022

Choose a reason for hiding this comment

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

Please also remove the ticket ids from the commit message title, it is enough to mention them in the body. And we usually use Relates-to: #4637 for ticket references.

analyzer/src/main/kotlin/managers/Pip.kt Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Aug 12, 2022

Codecov Report

Merging #5645 (0ce6b0f) into main (65c1565) will increase coverage by 0.08%.
The diff coverage is 71.42%.

@@             Coverage Diff              @@
##               main    #5645      +/-   ##
============================================
+ Coverage     65.46%   65.54%   +0.08%     
+ Complexity     2219     2212       -7     
============================================
  Files           271      271              
  Lines         16594    16600       +6     
  Branches       3445     3473      +28     
============================================
+ Hits          10863    10881      +18     
+ Misses         4588     4575      -13     
- Partials       1143     1144       +1     
Flag Coverage Δ
funTest-analyzer-docker 74.58% <83.33%> (+0.09%) ⬆️
funTest-non-analyzer 46.12% <ø> (ø)
test 32.01% <0.00%> (-0.05%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
analyzer/src/main/kotlin/managers/Pip.kt 49.70% <71.42%> (+2.96%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@oheger-bosch
Copy link
Member

There are two (trivial) issues in the static code analysis: Multiple blank lines at lines 64 and 197.

@TG1999 TG1999 force-pushed the pyinsp branch 2 times, most recently from ed15766 to 190dd86 Compare August 16, 2022 15:59
@oheger-bosch
Copy link
Member

The changes look good to me, however, the commits probably need to be reworked. As stated in the contributor guide, requested changes should not be added in new commits, but the existing commits should be amended. Could you please do this?

@pombredanne
Copy link
Contributor Author

The changes look good to me, however, the commits probably need to be reworked. As stated in the contributor guide, requested changes should not be added in new commits, but the existing commits should be amended. Could you please do this?

@oheger-bosch Thanks! My mistake there.

@TG1999 TG1999 force-pushed the pyinsp branch 5 times, most recently from a56816f to 9137a68 Compare August 22, 2022 17:52

val projectDependencies = if (definitionFile.name == "setup.py") {
// The tree contains a root node for the project itself and pipdeptree's dependencies are also at the
// root next to it, as siblings.
// The tree contains a root node for the project itself.
Copy link
Member

@mnonnenmacher mnonnenmacher Aug 23, 2022

Choose a reason for hiding this comment

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

@pombredanne This assumption is not correct anymore with the python-inspector. I tested it locally and the python-inspector output is:

[
  {
    "key": "ply",
    "package_name": "ply",
    "installed_version": "3.11",
    "dependencies": []
  },
  {
    "key": "rdflib",
    "package_name": "rdflib",
    "installed_version": "6.2.0",
    "dependencies": [
      {
        "key": "isodate",
        "package_name": "isodate",
        "installed_version": "0.6.1",
        "dependencies": [
          {
            "key": "six",
            "package_name": "six",
            "installed_version": "1.16.0",
            "dependencies": []
          }
        ]
      },
      {
        "key": "pyparsing",
        "package_name": "pyparsing",
        "installed_version": "3.0.9",
        "dependencies": []
      },
      {
        "key": "setuptools",
        "package_name": "setuptools",
        "installed_version": "65.2.0",
        "dependencies": []
      }
    ]
  }
]

So it does not contain a root node with the project name. Therefore the function returns EMPTY_JSON_NODE and the spdx-tools-python test is failing. The solution is to simplify lines 367-379 to:

val projectDependencies = fullDependencyTree.filterNot {
    isPhonyDependency(it["package_name"].textValue(), it["installed_version"].textValueOrEmpty())
}

(and to remove the projectName parameter of the function because it is unused now)

The test will still fail because "38" is always passed to python-inspector as Python version and therefore different versions of the dependencies are found. I would suggest to ignore this issue for now and just update the expected results file, and I will address this topic in a separate PR as discussed in the meeting today.

@TG1999 TG1999 force-pushed the pyinsp branch 3 times, most recently from d9bf6b5 to 76f44ff Compare August 23, 2022 14:53
This PR replaces pipdeptree with python-inspector to resolve
Python packages dependencies found in requirement files.
python-inspector can resolve dependencies for any target
Python version and OS (and not only the one running the tool).
In this integration in ORT, it replaces pipdeptree pretty much
in place as python-inspector implements a similar output data
structure by design to ease the integration.

Reference: https://github.com/nexB/python-inspector
Reference: oss-review-toolkit#4637
Reference: oss-review-toolkit#3671
Signed-off-by: Philippe Ombredanne <[email protected]>
Signed-off-by: Tushar Goel <[email protected]>
Signed-off-by: Philippe Ombredanne <[email protected]>
Signed-off-by: Tushar Goel <[email protected]>
@mnonnenmacher mnonnenmacher merged commit 792344a into oss-review-toolkit:main Aug 24, 2022
@sschuberth sschuberth changed the title Replace pipdeptree with python-inspector #4637 #3671 Replace pipdeptree with python-inspector Aug 24, 2022
@sschuberth sschuberth added the release notes Changes that should be mentioned in release notes label Aug 25, 2022
@TG1999 TG1999 deleted the pyinsp branch December 20, 2022 18:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes Changes that should be mentioned in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants