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

utils/hooks : is_module_satisfies() now returns the correct result for packages not found on the system. #3481

Merged
merged 10 commits into from Jun 28, 2018

Conversation

Projects
None yet
2 participants
@MagnumOpus21
Contributor

MagnumOpus21 commented Apr 25, 2018

'' in string returns True in Python.
Previously return version in requirements_parsed returned True.
Now added an if statement to check for an empty string check for version,
before returning the value of the function.
Fixes #3428

Hooks: Added empty string check for version in is_module_satisfies()
'' in string returns True in Python.
Previously return version in requirements_parsed returned True.
Now added an if statement to check for an empty string check for version,
before returning the value of the function.
Fixes #3428
@MagnumOpus21

This comment has been minimized.

Contributor

MagnumOpus21 commented Apr 25, 2018

Hmm, can someone help me on this? I have no idea about the error messages on AppVeyor. Any pointers would be appreciated. And thank you.

@htgoebel

Thanks for the pull-request - that's been quick :-)

  • The appvayor failures are unrelated to this change, so don't worry.
  • Please add a test-case, too, so we will not be trapped by this again.
  • Please rethink the commit message.
    • Is it important that you added an empty string check, or that is_module_satisfies() returns False for packages that are not installed?
    • Please recheck also the sub-system prefix :-)
    • Also mind writing the first line in present tense, see Content of the commit message (also updating pull-reqeusts might be helpful.)
@@ -492,7 +492,10 @@ class method (e.g., `idontevenknow<1.6,>1.9,!=1.9.6,<2.0a0,==2.4c1`) is
version = get_module_attribute(module_name, version_attr)
# Compare this version against the version parsed from these requirements.
return version in requirements_parsed
if version is '':

This comment has been minimized.

@htgoebel

htgoebel Apr 25, 2018

Member

I suggest simply checking for not version which will match even if get_module_attribute() would return None.

Also this should go in front of the comment and get a comment of its own like "module does not exist".

@MagnumOpus21

This comment has been minimized.

Contributor

MagnumOpus21 commented Apr 25, 2018

I will change it sir. Also, should I rebase and make the four commits for the changes?

@htgoebel

This comment has been minimized.

Member

htgoebel commented May 27, 2018

Sorry for not answering earlier. Rebasing is a plus, although not necessary, as I can "merge rebase" :-) Not sure what you mean with "four commits". One commit would be enough.
You could also just push another commit with the updated message and I'll "sqaush merge".

@MagnumOpus21

This comment has been minimized.

Contributor

MagnumOpus21 commented May 27, 2018

Will do sir. Give me a day. :)

@MagnumOpus21

This comment has been minimized.

Contributor

MagnumOpus21 commented Jun 1, 2018

Sorry, I couldn't focus on them, during the weekdays. I'll do it before Monday. I apologize once again.

utils/hooks : is_module_satisfies() now returns the correct result fo…
…r uninstalled packages.

is_module_satisfies() returns false if the package is not installed on the system. 
Previously, the module returned true, even if the package is not installed 
on the system.
This fixes #3428.

@MagnumOpus21 MagnumOpus21 changed the title from Hooks: Added empty string check for version in is_module_satisfies() to utils/hooks : is_module_satisfies() now returns the correct result for packages not found on the system. Jun 4, 2018

MagnumOpus21 added some commits Jun 4, 2018

utils/hooks : is_module_satisfies fixed.
is_module_satisfies() returns false if the package is not installed on the system.
Previously, the module returned true, even if the package is not installed
on the system.
This fixes #3428.
@MagnumOpus21

This comment has been minimized.

Contributor

MagnumOpus21 commented Jun 4, 2018

Should the test for the module go into pyinstaller/tests/unit/test_hookutils.py? Here's the module I wrote.

def test_is_module_satisfies_package_not_installed():
    assert is_module_satisfies("xyz")

Is this correct? Or does the PR still need changes.
If you're fine with this. I can issue add this to this PR and will push the commit. 👍
Thank you and have an awesome day.

@htgoebel

This comment has been minimized.

Member

htgoebel commented Jun 4, 2018

Right, tests/unit/test_hookutils.py is the place to go.

The test-case looks good (but you should test it locally first :-) while I suggest to use a very unlikely package name, e.g. "magnumopus21-testcase-for-pytinsaller-package" ;-)

Also a "positive" test-case would be good. Maybe "pytest" is a good choice, since it is required for running test tests. As a safety belt you could agument it with @importorskip(...)

@MagnumOpus21

This comment has been minimized.

Contributor

MagnumOpus21 commented Jun 4, 2018

Thank you for the suggestion sir. I'll push a commit for the test cases soon. 👍

@MagnumOpus21

This comment has been minimized.

Contributor

MagnumOpus21 commented Jun 25, 2018

Sorry for taking a long time to get the fix. I think this issue is fixed. I'll work on #3483 . But this gives the correct result if the package is not installed on the system. Just tested locally works like a charm.

htgoebel added some commits Jun 26, 2018

@MagnumOpus21

This comment has been minimized.

Contributor

MagnumOpus21 commented Jun 27, 2018

Seems like the nighly builds are failing and AppVeyor tests have trouble on Windows tests.

htgoebel added some commits Jun 28, 2018

@htgoebel htgoebel merged commit fd00db3 into pyinstaller:develop Jun 28, 2018

1 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@htgoebel

This comment has been minimized.

Member

htgoebel commented Jun 28, 2018

Thanks again for the pull-request. Sorry for not merging earlier, I'm busy in my day-job.

@MagnumOpus21

This comment has been minimized.

Contributor

MagnumOpus21 commented Jun 28, 2018

Sir it was a pleasure working on this and other other problem. I was busy on my day job as well. So I couldn't complete it until today. And sorry for the subpar PR. This was when I barely knew git.

@htgoebel htgoebel added this to the PyInstaller 3.4 milestone Sep 2, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment