fix: Fail more gracefully when pip --dry-run doesn't work #2476

metabiswadeep · 2022-12-30T06:18:23Z

metabiswadeep · 2022-12-30T21:20:10Z

@BreadGenie @terriko could anyone review this please?

terriko · 2023-01-03T21:27:00Z

cve_bin_tool/parsers/python.py

-                yield from vendor
-        self.logger.debug(f"Done scanning file: {self.filename}")
+        except subprocess.CalledProcessError as e:
+            self.logger.error(f"Status : FAIL {e.returncode} {e.output}")


You're definitely going to want a more user-friendly error message here. Probably something like
"$filename not scanned: pip --dry-run was unable to get package versions"
"pip version >= ${whatever version dry-run was added in} is required to scan python requirements files"

That said, it's giving a warning that report may change, so we may need to plan around that as much as we can. I'm not sure if that's why your tests are failing, but you might need to take a look at what report is giving you and see if it changed in the past month.

Could you please elaborate on what you mean by report and how can I access it?

If you look up a few lines in the code, you can see how pip is being run:

"pip3", "install", "-r", self.filename, "--dry-run", "--ignore-installed", "--report", "-", "--quiet",

When you run that same command manually, you'll get the "report" in question.

When I run it, it generates a warning about report not yet producing a stable API. It's possible my version of pip is a few weeks old and this has been fixed. But basically if they change pip's output, we may need to change our code, so if you happened to get a version that was a bit different it could explain some test breakages. Or it could be unrelated. I just wanted to make sure you knew that we can't currently assume that what we get from pip won't change.

And if it is fixed and pip says they've got some sort of stable report since I last checked in December, we probably want to explicitly make that version-or-later of pip a requirement.

It's possible my version of pip is a few weeks old and this has been fixed.

It's still experimental as of pip 22.3.1. Docs say this:

The report is a JSON object with the following properties:
version: the string 0, denoting that the installation report is an experimental feature. This value will change to 1, when the feature is deemed stable after gathering user feedback (likely in pip 22.3 or 23.0).

Still experimental in 22.3 and I don't see any mention of it in 23.0 milestone. At least one change is expected before it becomes stable.

Yes, for me as well, report is giving that warning --report is currently an experimental option. The output format may change in a future release without prior warning .

Perhaps we can make pip upgrade to the latest version if the error is thrown?

And later if report is removed in a future version of pip, we could make pip to the last pip version which supported report.

Perhaps we can make pip upgrade to the latest version if the error is thrown?

I don't think it's a good idea to update anything during execution, it's an unusual and undesirable side effect.

And later if report is removed in a future version of pip, we could make pip to the last pip version which supported report.

I doubt it will be removed anytime soon, it's not even stable yet.

pip is a very important part of the Python ecosystem, doing any manipulations with it at runtime and even just limiting its version as a cve-bin-tool dependency is highly undesirable IMO. Requiring at least some version that has --report added/stabilized is different.

I agree with @Molkree - we MUST not auto-update any of the installed software. Given the fact that the --report option is still experimental, I wonder if we should be (a) checking that the installed version of pip supports the option (although this could be simply done by specifyng the minimum version of pip in the requirements.txt file) (b) marking this option as 'EXPERIMENTAL' (there is an environment variable for this that I added on another PR) and only invoke if requested and not by default.

I noticed the warning from pip. We should possibly be catching the output from the subprocess call (stderr, stdout) before passing this to the json loader.

codecov-commenter · 2023-01-05T23:57:54Z

Codecov Report

Merging #2476 (dda8039) into main (fb9cca7) will increase coverage by 0.01%.
The diff coverage is 76.92%.

@@            Coverage Diff             @@
##             main    #2476      +/-   ##
==========================================
+ Coverage   76.92%   76.93%   +0.01%     
==========================================
  Files         591      591              
  Lines        9736     9741       +5     
  Branches     1323     1323              
==========================================
+ Hits         7489     7494       +5     
  Misses       1947     1947              
  Partials      300      300

Flag	Coverage Δ
longtests	`76.93% <76.92%> (+0.01%)`	⬆️

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

Impacted Files	Coverage Δ
cve_bin_tool/parsers/python.py	`89.58% <76.92%> (-5.77%)`	⬇️
cve_bin_tool/cli.py	`63.98% <0.00%> (+0.96%)`	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Molkree

Not the purpose of this PR but urgh, who uses pip3 in 2023, it should be python -m pip.

Molkree · 2023-01-07T00:45:13Z

cve_bin_tool/parsers/python.py

+                f"{filename} not scanned: pip --dry-run was unable to get package versions."
+            )
+            self.logger.error(
+                "pip version >= 22.2 is required to scan python requirements files"


Suggested change

"pip version >= 22.2 is required to scan python requirements files"

"pip version >= 22.2 is required to scan Python requirements files."

We should not log this error every time, what if the scan wasn't successful for some other reason? After all, we can check the pip version and log only if pip is not new enough.

Maybe we should still log the underlying error in addition to the user-friendly message? Unless we know for sure pip is too old, then there's no point.

This error always arises due to an older version of pip I think because in newer versions, no such error arises.

cve_bin_tool/parsers/python.py

Molkree · 2023-01-07T00:58:36Z

I think tests on Windows are failing due to fb9cca7.

metabiswadeep · 2023-01-09T10:58:54Z

Not the purpose of this PR but urgh, who uses pip3 in 2023, it should be python -m pip.

@Molkree Don't know what's up with this, but CI gives FileNotFoundError: [Errno 2] No such file or directory: 'python -m pip'.

terriko · 2023-01-11T00:10:34Z

Not the purpose of this PR but urgh, who uses pip3 in 2023, it should be python -m pip.

Hah, I have the same feeling every time I read ... I think it's the packaging docs? But the fact that they're written like that even though they've been updated recently make me think that someone does indeed use pip3 still.

metabiswadeep · 2023-01-12T10:36:12Z

@terriko @anthonyharrison @Molkree Is this okay now?

terriko

Looks good, test failures appear to be unrelated. (type should be fixed in main, windows will still be broken)

metabiswadeep added 7 commits December 30, 2022 04:57

Update python.py

f84eb85

Update python.py

916b12e

Update python.py

836d75b

Update python.py

68642bc

Update python.py

912e1fe

Update python.py

cbe944e

Update python.py

6af1c7b

metabiswadeep marked this pull request as draft December 30, 2022 06:18

Linting

2bd6116

terriko requested changes Jan 3, 2023

View reviewed changes

metabiswadeep added 2 commits January 5, 2023 23:06

Changed error message

983388e

Merge branch 'main' into parser

dda8039

Molkree suggested changes Jan 7, 2023

View reviewed changes

Update python.py

4904e7c

metabiswadeep marked this pull request as ready for review January 7, 2023 10:01

metabiswadeep added 4 commits January 12, 2023 02:17

Added checking of pip version

9b4ab76

Merge branch 'main' into parser

a0167d2

Linting

b437a16

Update python.py

cb1b2e8

terriko approved these changes Jan 18, 2023

View reviewed changes

terriko merged commit b21f142 into intel:main Jan 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Fail more gracefully when pip --dry-run doesn't work #2476

fix: Fail more gracefully when pip --dry-run doesn't work #2476

metabiswadeep commented Dec 30, 2022

metabiswadeep commented Dec 30, 2022

terriko Jan 3, 2023

metabiswadeep Jan 4, 2023

terriko Jan 5, 2023 •

edited

Loading

terriko Jan 5, 2023

Molkree Jan 5, 2023

metabiswadeep Jan 5, 2023 •

edited

Loading

metabiswadeep Jan 6, 2023

metabiswadeep Jan 6, 2023

Molkree Jan 7, 2023 •

edited

Loading

anthonyharrison Jan 9, 2023

codecov-commenter commented Jan 5, 2023

Molkree left a comment

Molkree Jan 7, 2023

Molkree Jan 7, 2023

metabiswadeep Jan 7, 2023

Molkree commented Jan 7, 2023

metabiswadeep commented Jan 9, 2023

terriko commented Jan 11, 2023

metabiswadeep commented Jan 12, 2023

terriko left a comment

	"pip version >= 22.2 is required to scan python requirements files"
	"pip version >= 22.2 is required to scan Python requirements files."

fix: Fail more gracefully when pip --dry-run doesn't work #2476

fix: Fail more gracefully when pip --dry-run doesn't work #2476

Conversation

metabiswadeep commented Dec 30, 2022

metabiswadeep commented Dec 30, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

terriko Jan 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

metabiswadeep Jan 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Molkree Jan 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Jan 5, 2023

Codecov Report

Molkree left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Molkree commented Jan 7, 2023

metabiswadeep commented Jan 9, 2023

terriko commented Jan 11, 2023

metabiswadeep commented Jan 12, 2023

terriko left a comment

Choose a reason for hiding this comment

terriko Jan 5, 2023 •

edited

Loading

metabiswadeep Jan 5, 2023 •

edited

Loading

Molkree Jan 7, 2023 •

edited

Loading