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

fix: Remove reportlab from default install #1626

Merged
merged 5 commits into from
Mar 31, 2022
Merged

Conversation

terriko
Copy link
Contributor

@terriko terriko commented Mar 30, 2022

@terriko
Copy link
Contributor Author

terriko commented Mar 30, 2022

Copying over from the bug report:

The diff in output_engine/__init__.py looks terrible because of the way git is parsing it, but what I've done is made it so that if the reportlab module doesn't load, it turns the output_pdf() function into a stub that prints a warning message about reportlab not being installed (this never actually gets called in practice, but I needed to put something there to make __init__.py load correctly). Because I had to change the indentation of the (rather large) output_pdf() function, it makes it look like I changed every line of that code, but I literally just selected the whole function and hit tab to push it up a level, I promise!

Note that I also reved the version number in here; that was to facilitate some testing against testpypi for package building.

Copy link
Contributor

@anthonyharrison anthonyharrison left a comment

Choose a reason for hiding this comment

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

@terriko Looks good. Are there any changes that need to be made to the test suite to demonstrate when PDF support is available or not?

@terriko
Copy link
Contributor Author

terriko commented Mar 31, 2022

@anthonyharrison Yes, I've been trying to figure out what would be a viable test to add here.

Manually, what I did was variations on this:

  1. make a new venv, install the base requirements.txt with no reportlab
  2. try to run the tool on a small vulnerable file with -f pdf -o mytest.pdf
  3. see that it instead produces console output, check that it printed appropriate warning messages
  4. install reportlab, repeat running the tool with those flags, show that it gives console output and no warning messages

I also did a bunch of testing with testpypi trying to verify the packaging, but I don't think we can (or should) automate those.

I'm going to go ahead and file a separate bug for tests and merge this so I can start prepping a pre-release, but I think a test would be viable it's just a matter of seeing if we can strip it down so we don't have to churn on installing stuff or scan an actual file. Maybe something with mock?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

tool includes reportlab dependency which is flagged by Safety
3 participants