refactor: replace pkg_resources with importlib (#1521) #1542

XDRAGON2002 · 2022-01-24T11:21:52Z

Fixes #1521
Removed occurrences of pkg_resources from cli.py, version_scanner.py and test_checkers.py.

Molkree

Hm, I don't think we use update type of commit? It should be refactor IMO.

Molkree · 2022-01-24T17:33:52Z

cve_bin_tool/cli.py

+if sys.version_info >= (3, 8):
+    from importlib import metadata as importlib_metadata
+else:
+    import importlib_metadata


Just checking this is not enough, we will actually need to include importlib_metadata dependency for Python less than 3.8 (which is only 3.7 at this point).

You will need to add this line to requirements.txt:

importlib_metadata;python_version<"3.8"

You may ask: but why do all tests pass on Python 3.7 in CI? That's because this package is required by some doc package that we use (sphinx-markdown-tables requires Markdown which in turn requires our importlib_metadata) but the end user might not use docs functionality. And it's better to be explicit about it. Especially because we will most likely remove this workaround at the end of the year when Python 3.7 goes out of support.

Oh okay, I was actually just going to ask about the 3.7 tests, thanks for the explanation.
Will add importlib to the requirements file as well as rename the commit to adhere to standards.

BreadGenie · 2022-01-24T18:00:15Z

.github/workflows/pythonapp.yml

@@ -288,7 +288,7 @@ jobs:
      - name: Install dependencies
        run: |
          python -m pip install --upgrade pip setuptools wheel
-          pip install -r requirements.txt -r doc/requirements.txt
+          pip install . -r doc/requirements.txt
      - name: Test to check for CVEs for python requirements and HTML report dependencies


Is there a need to change it? Both does the same right?

As far as I understand it, we were not installing the current version of cve-bin-tool to be used (hence the . to specify the updated version)

we were not installing the current version of cve-bin-tool to be used

I didn't get this part. Did you mean current versions of dependencies?

No, I mean the cve-bin-tool as a whole (instead of pypi we get it from setup.py i.e. the current copy on which the tests are running)

The old one just installs all required packages for cve-bin-tool but not the tool itself. The new one installs the tool (with all dependencies of course).

This is needed to fix the test, copying my comments from gitter

this is an interesting one, I'm actually not sure how this test worked before because it doesn't install cve-bin-tool explicitly
in .github/workflows/pythonapp.yml replace line 291 with this one

-pip install -r requirements.txt -r doc/requirements.txt +pip install . -r doc/requirements.txt

ah, I know why
it's because of the caching that we use. This job workflow has the same cache key as other jobs so when restoring the cache it already had cve-bin-tool installed
in other jobs it is explicitly reinstalled to use the latest commit and in this job we ended up with some old version and never reinstalled the tool

codecov-commenter · 2022-01-24T18:14:30Z

Codecov Report

Merging #1542 (e173876) into main (b2262e0) will decrease coverage by 0.02%.
The diff coverage is 53.84%.

@@            Coverage Diff             @@
##             main    #1542      +/-   ##
==========================================
- Coverage   79.18%   79.15%   -0.03%     
==========================================
  Files         281      281              
  Lines        5548     5555       +7     
  Branches      905      908       +3     
==========================================
+ Hits         4393     4397       +4     
  Misses        971      971              
- Partials      184      187       +3

Flag	Coverage Δ
longtests	`79.15% <53.84%> (-0.03%)`	⬇️

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

Impacted Files	Coverage Δ
cve_bin_tool/cli.py	`71.10% <33.33%> (-0.66%)`	⬇️
cve_bin_tool/version_scanner.py	`56.86% <50.00%> (+0.42%)`	⬆️
test/test_checkers.py	`93.54% <66.66%> (-3.07%)`	⬇️
cve_bin_tool/cvedb.py	`72.22% <0.00%> (+0.27%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b2262e0...e173876. Read the comment docs.

XDRAGON2002 · 2022-01-24T18:22:17Z

The dependency tests failed again, maybe because the requirements.txt and requirements.csv are out of sync? (as stated in the
error log).

Molkree · 2022-01-24T18:40:21Z

maybe because the requirements.txt and requirements.csv are out of sync

Yes, but I'm not sure what to put as a vendor, py?

XDRAGON2002 · 2022-01-24T18:45:28Z

maybe because the requirements.txt and requirements.csv are out of sync

Yes, but I'm not sure what to put as a vendor, py?

I was looking around for the same, didn't find anything yet.

BreadGenie · 2022-01-25T03:48:11Z

Usually we put the vendor name as in the CVE database. If they don't exist in the database we put org/maintainer_not_in_db

XDRAGON2002 · 2022-01-25T06:32:30Z

Added dependency in requirements.csv as well, I guess the PR is complete now?

terriko

Looks like you got hit with my bad CI change yesterday. (you can tell because only 4 things ran in Github Actions and there should be a lot more) Can you rebase to origin/main so we can make sure the tests are running in CI for this PR?

XDRAGON2002 · 2022-01-26T15:17:23Z

Rebased to main and resolved all merge conflicts.

Molkree · 2022-01-26T16:18:12Z

Rebased to main and resolved all merge conflicts.

It looks like you did a combination of merges and rebase. For future reference, try squashing commits in your rebase, it is a really powerful tool that allows you to keep commit history clean.
https://docs.github.com/en/get-started/using-git/about-git-rebase

You now need to rerun isort on this branch to fix the failing lint check.

XDRAGON2002 · 2022-01-26T16:22:29Z

Rebased to main and resolved all merge conflicts.

It looks like you did a combination of merges and rebase. For future reference, try squashing commits in your rebase, it is a really powerful tool that allows you to keep commit history clean. https://docs.github.com/en/get-started/using-git/about-git-rebase

You now need to rerun isort on this branch to fix the failing lint check.

Right, I didn't look into squashing the commits (which I should have done considering I have numerous small commits), should have done that from the start.
Thanks for the pointer though! Really appreciate it.

terriko

Merging #1552 broke this one, but may have fixed the issue you were encountering that required that change? You'll probably need to rebase, you may no longer need the changes in CI.

XDRAGON2002 · 2022-02-03T17:48:36Z

Strange, Windows tests failed because "pip" threw an error. I don't know what could have caused this.
So did the python long tests.
Maybe rerunning CI would help?

Molkree · 2022-02-03T21:45:12Z

Strange, Windows tests failed because "pip" threw an error. I don't know what could have caused this.

This is indeed strange and I can replicate it outside of this repo. It seems like there is something wrong with setup-python Action with pip caching enabled/pip/GitHub Actions runner or a combination of those. I will file a bug report in the setup-python repo first 🤔
Edit: seems like it's caused by the change in the runner environment, issue upstream.

So did the python long tests.

It is 403 from NVD again so just rate limiting I guess. Won't be fixed until we fix passing NVD API key in CI.

Maybe rerunning CI would help?

I don't think it would help until the first issue is fixed upstream (Windows tests).

XDRAGON2002 · 2022-02-04T06:55:17Z

Thanks for pinpointing the issue!
I suppose as the cause is outside the scope of this repo, we have no choice but to wait until it gets patched.

Molkree · 2022-02-04T15:07:16Z

setup-python@v2.3.2 was released that fixes this issue. No changes from us are required because v2 tag was updated to point at the new release. Just rerunning CI should be enough.

XDRAGON2002 · 2022-02-04T18:05:55Z

Rerun CI, again got NVD rate limited, but all other tests have passed.

terriko

Looks like this is ready to go, only the nvd rate limiting is hitting it. I'm going to go ahead and merge.

XDRAGON2002 added 4 commits December 29, 2021 17:51

Updated spelling.yml

ffba885

Merge branch 'main' of https://github.com/intel/cve-bin-tool into main

c191f20

Merge branch 'main' of https://github.com/intel/cve-bin-tool into main

576871b

Merge branch 'main' of https://github.com/intel/cve-bin-tool into main

5707392

XDRAGON2002 force-pushed the issue_1521 branch from aa4f5d1 to cbf573a Compare January 24, 2022 11:26

XDRAGON2002 mentioned this pull request Jan 24, 2022

Move to importlib from pkg_resources #1521

Closed

Molkree suggested changes Jan 24, 2022

View reviewed changes

XDRAGON2002 force-pushed the issue_1521 branch from 0cd9608 to 8c588ba Compare January 24, 2022 17:50

BreadGenie reviewed Jan 24, 2022

View reviewed changes

XDRAGON2002 changed the title ~~update: replace pkg_resources with importlib (#1521)~~ refactor: replace pkg_resources with importlib (#1521) Jan 24, 2022

XDRAGON2002 force-pushed the issue_1521 branch from e173876 to 2981fe1 Compare January 25, 2022 06:24

terriko requested changes Jan 26, 2022

View reviewed changes

XDRAGON2002 added 7 commits January 26, 2022 20:26

Merge branch 'main' of https://github.com/intel/cve-bin-tool into main

63981f0

update: replace pkg_resources with importlib

ac33bdd

chore: formatting

fcf0a6c

ci: update pythonapp.yml

1ca9403

chore: update requirements.txt

23fc23e

chore: resolve conflict

4850b83

chore: update requirements.csv

47eab1f

XDRAGON2002 force-pushed the issue_1521 branch from 2981fe1 to 47eab1f Compare January 26, 2022 15:14

fix: resolve merge conflict

96282f8

chore: formatting

2677e89

XDRAGON2002 requested a review from terriko January 28, 2022 20:56

terriko requested changes Feb 2, 2022

View reviewed changes

chore: resolve merge conflict

7c37cee

XDRAGON2002 requested a review from terriko February 3, 2022 17:46

chore: rerun CI

0940c4d

terriko approved these changes Feb 7, 2022

View reviewed changes

terriko merged commit 8d95852 into intel:main Feb 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: replace pkg_resources with importlib (#1521) #1542

refactor: replace pkg_resources with importlib (#1521) #1542

XDRAGON2002 commented Jan 24, 2022

Molkree left a comment

Molkree Jan 24, 2022

XDRAGON2002 Jan 24, 2022

BreadGenie Jan 24, 2022

XDRAGON2002 Jan 24, 2022

BreadGenie Jan 24, 2022

XDRAGON2002 Jan 24, 2022

Molkree Jan 24, 2022 •

edited

codecov-commenter commented Jan 24, 2022

XDRAGON2002 commented Jan 24, 2022

Molkree commented Jan 24, 2022

XDRAGON2002 commented Jan 24, 2022

BreadGenie commented Jan 25, 2022 •

edited

XDRAGON2002 commented Jan 25, 2022

terriko left a comment

XDRAGON2002 commented Jan 26, 2022

Molkree commented Jan 26, 2022

XDRAGON2002 commented Jan 26, 2022

terriko left a comment

XDRAGON2002 commented Feb 3, 2022 •

edited

Molkree commented Feb 3, 2022 •

edited

XDRAGON2002 commented Feb 4, 2022 •

edited

Molkree commented Feb 4, 2022

XDRAGON2002 commented Feb 4, 2022

terriko left a comment

refactor: replace pkg_resources with importlib (#1521) #1542

refactor: replace pkg_resources with importlib (#1521) #1542

Conversation

XDRAGON2002 commented Jan 24, 2022

Molkree left a comment

Choose a reason for hiding this comment

Molkree Jan 24, 2022

Choose a reason for hiding this comment

XDRAGON2002 Jan 24, 2022

Choose a reason for hiding this comment

BreadGenie Jan 24, 2022

Choose a reason for hiding this comment

XDRAGON2002 Jan 24, 2022

Choose a reason for hiding this comment

BreadGenie Jan 24, 2022

Choose a reason for hiding this comment

XDRAGON2002 Jan 24, 2022

Choose a reason for hiding this comment

Molkree Jan 24, 2022 • edited

Choose a reason for hiding this comment

codecov-commenter commented Jan 24, 2022

Codecov Report

XDRAGON2002 commented Jan 24, 2022

Molkree commented Jan 24, 2022

XDRAGON2002 commented Jan 24, 2022

BreadGenie commented Jan 25, 2022 • edited

XDRAGON2002 commented Jan 25, 2022

terriko left a comment

Choose a reason for hiding this comment

XDRAGON2002 commented Jan 26, 2022

Molkree commented Jan 26, 2022

XDRAGON2002 commented Jan 26, 2022

terriko left a comment

Choose a reason for hiding this comment

XDRAGON2002 commented Feb 3, 2022 • edited

Molkree commented Feb 3, 2022 • edited

XDRAGON2002 commented Feb 4, 2022 • edited

Molkree commented Feb 4, 2022

XDRAGON2002 commented Feb 4, 2022

terriko left a comment

Choose a reason for hiding this comment

Molkree Jan 24, 2022 •

edited

BreadGenie commented Jan 25, 2022 •

edited

XDRAGON2002 commented Feb 3, 2022 •

edited

Molkree commented Feb 3, 2022 •

edited

XDRAGON2002 commented Feb 4, 2022 •

edited