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

add egg updater script which fixes pytest initialization issue #672

Merged
merged 14 commits into from
May 19, 2020

Conversation

Niraj-Kamdar
Copy link
Contributor

No description provided.

add update_egg to cli
@Niraj-Kamdar Niraj-Kamdar changed the title add egg updater script which fixes pytest initialization issue #605 add egg updater script which fixes pytest initialization issue May 14, 2020
@Niraj-Kamdar
Copy link
Contributor Author

Fixes issue #605

cve_bin_tool/cli.py Outdated Show resolved Hide resolved
cve_bin_tool/cli.py Outdated Show resolved Hide resolved
@terriko
Copy link
Contributor

terriko commented May 14, 2020

Also, refactoring nitpicks aside, thanks for doing the research to get this done! I'm really glad to see a solution to what looks like such a weird bug.

cve_bin_tool/cli.py Outdated Show resolved Hide resolved
Copy link
Member

@pdxjohnny pdxjohnny left a comment

Choose a reason for hiding this comment

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

Nice!!! Great work finding the fix for this. Very crafty

@codecov-io
Copy link

codecov-io commented May 15, 2020

Codecov Report

Merging #672 into master will decrease coverage by 8.69%.
The diff coverage is 82.35%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #672      +/-   ##
==========================================
- Coverage   89.70%   81.01%   -8.70%     
==========================================
  Files          66       67       +1     
  Lines        2118     2165      +47     
  Branches      282      283       +1     
==========================================
- Hits         1900     1754     -146     
- Misses        152      341     +189     
- Partials       66       70       +4     
Flag Coverage Δ
#longtests 81.01% <82.35%> (-8.70%) ⬇️
Impacted Files Coverage Δ
cve_bin_tool/cli.py 77.73% <82.35%> (-6.24%) ⬇️
cve_bin_tool/cvedb.py 52.15% <0.00%> (-30.57%) ⬇️
test/test_json.py 61.76% <0.00%> (-29.42%) ⬇️
cve_bin_tool/checkers/xml2.py 61.36% <0.00%> (-27.28%) ⬇️
cve_bin_tool/OutputEngine.py 73.10% <0.00%> (-24.54%) ⬇️
test/test_cli.py 82.05% <0.00%> (-17.95%) ⬇️
test/test_scanner.py 83.11% <0.00%> (-9.10%) ⬇️
cve_bin_tool/extractor.py 63.63% <0.00%> (-2.73%) ⬇️
cve_bin_tool/checkers/__init__.py 100.00% <0.00%> (ø)
... and 2 more

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 367926b...46a1b5e. Read the comment docs.

@Niraj-Kamdar
Copy link
Contributor Author

Nice!!! Great work finding the fix for this. Very crafty

Thank you. Always up for challenging problems.

@Niraj-Kamdar Niraj-Kamdar requested a review from terriko May 15, 2020 15:56
egg_updater.py Outdated Show resolved Hide resolved
cve_bin_tool/cli.py Outdated Show resolved Hide resolved
Copy link
Member

@pdxjohnny pdxjohnny left a comment

Choose a reason for hiding this comment

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

Looking good (after these two minor changes)

Co-authored-by: John Andersen <johnandersenpdx@gmail.com>
anthonyharrison added a commit to anthonyharrison/cve-bin-tool that referenced this pull request May 16, 2020
egg_updater.py Outdated
def update_egg():
with StringIO() as f:
sys.stdout = f
dist = Distribution(
Copy link
Member

@pdxjohnny pdxjohnny May 16, 2020

Choose a reason for hiding this comment

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

We should probably do a try: os.chdir to os.path.join(os.path.dirname(__file__), "..") and then a finally: os.chdir(wherever_os.getcwd()_reported_before_first_chdir). Since I'd guess that this fails if the user is not in the root of the source tree

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes this will work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have tried it, it's not working

Copy link
Member

Choose a reason for hiding this comment

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

What's not working?

Copy link
Member

Choose a reason for hiding this comment

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

In the above example: os.path.join(os.path.dirname(__file__), "..") should be os.path.dirname(__file__)

Copy link
Contributor Author

@Niraj-Kamdar Niraj-Kamdar May 18, 2020

Choose a reason for hiding this comment

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

I have put egg_updater inside cve_bin_tool and save cwd by cwd = os.getcwd() and then changed diectory to os.chdir(os.path.join(os.path.dirname(__file__), "..")) and after execution of script, I have reverted back to the previous cwd but it is giving same error as we get without applying chdir hack.

(venv) C:\Users\Root\Documents\cve-bin-tool>python -m cve_bin_tool.cli build
cve_bin_tool - WARNING -
                          **********************************************
                          Warning: this utility was developed for Linux.
                          You may need to install additional utilities
                          to use it on other operating systems.
                          **********************************************

cve_bin_tool.CVEDB - INFO - Using cached CVE data (<24h old). Use -u now to update immediately.
cve_bin_tool.Scanner - INFO - Updating egg_info
C:\Users\Root\Documents\cve-bin-tool C:\Users\Root\Documents\cve-bin-tool\cve_bin_tool
Traceback (most recent call last):
  File "C:\Users\Root\AppData\Local\Programs\Python\Python37\lib\runpy.py", line 193, in _run_module_as_main
    "__main__", mod_spec)
  File "C:\Users\Root\AppData\Local\Programs\Python\Python37\lib\runpy.py", line 85, in _run_code
    exec(code, run_globals)
  File "C:\Users\Root\Documents\cve-bin-tool\cve_bin_tool\cli.py", line 587, in <module>
    sys.exit(main())
  File "C:\Users\Root\Documents\cve-bin-tool\cve_bin_tool\cli.py", line 471, in main
    scanner = Scanner(cvedb)
  File "C:\Users\Root\Documents\cve-bin-tool\cve_bin_tool\cli.py", line 72, in __init__
    egg_updater.update_egg()
  File "C:\Users\Root\Documents\cve-bin-tool\cve_bin_tool\egg_updater.py", line 35, in update_egg
    "checkers",
FileNotFoundError: [WinError 3] The system cannot find the path specified: 'C:\\Users\\Root\\Documents\\cve-bin-tool\\cve_bin_tool\\cve_bin_tool\\c
heckers'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's solved.

cve_bin_tool/cli.py Outdated Show resolved Hide resolved
cve_bin_tool/cli.py Outdated Show resolved Hide resolved
@Niraj-Kamdar

This comment has been minimized.

@Niraj-Kamdar

This comment has been minimized.

@Niraj-Kamdar

This comment has been minimized.

@Niraj-Kamdar

This comment has been minimized.

update cli.py
@Niraj-Kamdar
Copy link
Contributor Author

Niraj-Kamdar commented May 18, 2020

My alternative didn't work. We can't exclude a single script from the package using find_packages's exclude option. It was working before because root-level script automatically get converted into an package by python but since now we have put egg_updater script inside cve_bin_tool package, egg_updater won't get promoted to the package. So, for now developer has to install cve_bin_tool using pip install -e command to get benefit of automatic egg updater.

spec.loader.exec_module(egg_updater)
else:
egg_updater = None
DEVELOP = True
Copy link
Member

Choose a reason for hiding this comment

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

This block isn't necessary now that we've moved the module into the library, we can import with from . import egg_updater

Copy link
Contributor Author

@Niraj-Kamdar Niraj-Kamdar May 19, 2020

Choose a reason for hiding this comment

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

I have done it.

Copy link
Member

@pdxjohnny pdxjohnny left a comment

Choose a reason for hiding this comment

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

That's a bummer about exclude_files :( Just one thing left I think

@codecov-commenter
Copy link

codecov-commenter commented May 19, 2020

Codecov Report

Merging #672 into master will decrease coverage by 0.38%.
The diff coverage is 61.29%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #672      +/-   ##
==========================================
- Coverage   80.95%   80.56%   -0.39%     
==========================================
  Files          67       68       +1     
  Lines        2168     2187      +19     
  Branches      281      284       +3     
==========================================
+ Hits         1755     1762       +7     
- Misses        346      357      +11     
- Partials       67       68       +1     
Flag Coverage Δ
#longtests 80.56% <61.29%> (-0.39%) ⬇️
Impacted Files Coverage Δ
cve_bin_tool/egg_updater.py 43.75% <43.75%> (ø)
cve_bin_tool/cli.py 77.58% <80.00%> (-0.82%) ⬇️

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 3ebc7da...06b09a1. Read the comment docs.

@pdxjohnny
Copy link
Member

Awesome!! Thank you!!

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.

None yet

5 participants