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.

Sign up for GitHub

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

Jump to bottom

Mock the actual setup provider defined in setup.py #120

Merged
merged 4 commits into from
Mar 1, 2023
Merged

Mock the actual setup provider defined in setup.py #120

merged 4 commits into from
Mar 1, 2023

Conversation

bennati
Copy link
Contributor

@bennati bennati commented Jan 19, 2023

Currently. setup is always mocked using distutils.core but this might cause issues with certain packages. Fix this behavior by parsing the setup.py file for the correct module to import.

Closes: #116

Signed-off-by: Bennati, Stefano [email protected]

@bennati bennati mentioned this pull request Jan 19, 2023
Closed
@bennati bennati force-pushed the pr-116-2 branch 2 times, most recently from 2139cd6 to b3b27e5 Compare January 20, 2023 08:25
@TG1999
Copy link
Contributor

TG1999 commented Jan 23, 2023

@bennati thanks! please provide a few tests with the files you faced this issue.

@TG1999
Copy link
Contributor

TG1999 commented Jan 24, 2023

@bennati you need to regenerate the test outputs, the dependencies have been updated for the tests so for fixing tests in test_cli use this command:

PYINSP_REGEN_TEST_FIXTURES=yes pytest -vvs

for fixing tests in test_resolution, please compare the changes from the expected output.

@bennati
Copy link
Contributor Author

bennati commented Jan 24, 2023 •
edited
Loading

@TG1999 EDIT I cherry-picked the commits that fix the tests from the other PR #117

@bennati
Copy link
Contributor Author

bennati commented Jan 30, 2023

Is there anything left to do before we can merge this?

@pombredanne
Copy link
Member

Thanks. Let me check! there seems to be something weird with your latest commit as make valid should not have changed etc/ and src/_packagedcode/

@bennati
Copy link
Contributor Author

bennati commented Feb 1, 2023

@pombredanne should I just refactor the last commit and remove the changes to the unrelated files?

@npatel2509
Copy link

@pombredanne Please suggest if there is anything left or a blocker to merge this MR?

@TG1999
Copy link
Contributor

TG1999 commented Feb 6, 2023

@bennati please remove the changes to unrelated files.

@bennati bennati force-pushed the pr-116-2 branch 2 times, most recently from fa3a091 to 3447f9e Compare February 7, 2023 09:09
@bennati
Copy link
Contributor Author

bennati commented Feb 7, 2023

Removed the unrelated changes

TG1999
TG1999 approved these changes Feb 7, 2023
View reviewed changes
Copy link
Contributor

@TG1999 TG1999 left a comment

Choose a reason for hiding this comment

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

LGTM!

@TG1999 TG1999 requested a review from pombredanne February 7, 2023 12:24
@npatel2509
Copy link

@pombredanne please suggest if any further modifications is require or if we can proceed to merge the changes?

pombredanne
pombredanne reviewed Feb 10, 2023
View reviewed changes
Copy link
Member

@pombredanne pombredanne left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM overall! but I have a few nitpickings for your consideration.

src/python_inspector/setup_py_live_eval.py Outdated Show resolved Hide resolved
src/python_inspector/setup_py_live_eval.py Outdated
"defaulting to 'distutils.core'."
)
setup_provider = "distutils.core"
exec(f"import {setup_provider}")
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure that we can get something that's safe to work here. In particular importing setuptools since version 66 has several possible problems... Could we do better and not need to import this? If we really need to import it, we should IMHO vendor a version of setuptools that before version 66?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed it and imported both libraries

@bennati bennati requested a review from pombredanne February 14, 2023 15:50
@bennati bennati force-pushed the pr-116-2 branch 4 times, most recently from a40a70a to a4e60d9 Compare February 15, 2023 08:11
@bennati
Copy link
Contributor Author

bennati commented Feb 15, 2023

The tests keep failing despite all my attempts, help is appreciated!

@bennati bennati force-pushed the pr-116-2 branch 2 times, most recently from b82cdb1 to a57af53 Compare February 20, 2023 15:34
@bennati bennati requested review from TG1999 and removed request for pombredanne February 20, 2023 17:15
@bennati bennati requested review from pombredanne and removed request for TG1999 February 20, 2023 17:15
@bennati
Copy link
Contributor Author

bennati commented Feb 22, 2023

Tests are now successful, can we merge this?

@pombredanne
Copy link
Member

@bennati Would you mind to:

  1. undo the changes made strictly from black formatting
  2. run instead afterwards make valid that will only black format certain files but not all

And may review your commit streams for good message and a possible squash?
Thanks

@bennati
Mock the actual setup provider defined in setup.py
98fe24c 
Currently `setup` is always mocked using `distutils.core`
but this might cause issues with certain packages.
Fix this behavior by parsing the `setup.py` file for the correct
module to import.

Closes: aboutcode-org#116

Signed-off-by: Bennati, Stefano <[email protected]>
@bennati
Parse setup.py file with AST instead of using regex.
39692b2 
Signed-off-by: Bennati, Stefano <[email protected]>
@bennati bennati force-pushed the pr-116-2 branch 2 times, most recently from d049c1b to cfdcedf Compare February 23, 2023 09:44
@bennati
Update test fixtures.
50fb768 
Signed-off-by: Bennati, Stefano <[email protected]>
@bennati
Run make valid.
636bb5a 
Signed-off-by: Bennati, Stefano <[email protected]>
@bennati
Copy link
Contributor Author

bennati commented Feb 23, 2023

Done, we can squash all commits

@bennati
Copy link
Contributor Author

bennati commented Mar 1, 2023

Another week has passed

pombredanne
pombredanne approved these changes Mar 1, 2023
View reviewed changes
Copy link
Member

@pombredanne pombredanne left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks. Merging

@pombredanne pombredanne merged commit 102ef9b into aboutcode-org:main Mar 1, 2023
@mmurto mmurto mentioned this pull request Mar 10, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pombredanne pombredanne pombredanne approved these changes

@TG1999 TG1999 TG1999 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Failure parsing dependences of pypi:packer.py
4 participants
@bennati @TG1999 @pombredanne @npatel2509