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

is_module_satisfies() returns True for packages that are not installed #3428

Closed
Legrandin opened this Issue Mar 27, 2018 · 7 comments

Comments

Projects
None yet
4 participants
@Legrandin
Contributor

Legrandin commented Mar 27, 2018

is_module_satisfies() should return False if the package being provided is not installed.
Instead I get:

>>> from PyInstaller.utils.hooks import is_module_satisfies
>>> is_module_satisfies("xyz")
Traceback (most recent call last):
File "<string>", line 2, in <module>
ImportError: No module named xyz
True
>>>
@htgoebel

This comment has been minimized.

Member

htgoebel commented Mar 28, 2018

This is cause by this line setting version to an empty string if the module does not exist. That makes the test just below evaluate to true.

Needs test-case :-)

@MagnumOpus21

This comment has been minimized.

Contributor

MagnumOpus21 commented Apr 24, 2018

Hi @htgoebel can I get assigned to this issue? I think I fixed it on my branch. If you could tell me how I could start a PR it would be awesome sir.

@htgoebel

This comment has been minimized.

Member

htgoebel commented Apr 24, 2018

Feel free to provide a pull-request for this. We are not using "Assignment" in the project. (And github technically does not all allow to set you as "Assignee".) I'm looking forward to you contribution :-)

@tallforasmurf

This comment has been minimized.

Contributor

tallforasmurf commented Apr 24, 2018

See Creating pull requests and commit messages for guidelines.

@MagnumOpus21

This comment has been minimized.

Contributor

MagnumOpus21 commented Apr 24, 2018

Thank you sir. There is also one thing I wanted to get clarification on. The exec_statement function creates a new shell and tries to import the package. This throws a ModuleNotFoundException if the package is not found, but the function returns the correct value anyways. Is that ok or what should the correct behaviour be?
I am attaching the code for that part here.

def get_module_attribute(module_name, attr_name):
attr_value_if_undefined = '!)ABadCafe@(D15ea5e#*DeadBeef$&Fee1Dead%^'
    attr_value = exec_statement("""
        import %s as m
        print(getattr(m, %r, %r))
    """ % (module_name, attr_name, attr_value_if_undefined))
    if attr_value == attr_value_if_undefined:
        raise AttributeError(
            'Module %r has no attribute %r' % (module_name, attr_name))
    else:
        return attr_value

Some comments from the module were removed before pasting here. These are lines from 260-298 of the highlighted function.

>>> from PyInstaller.utils.hooks import is_module_satisfies
>>> a=is_module_satisfies('abd')
Traceback (most recent call last):
  File "<string>", line 2, in <module>
ModuleNotFoundError: No module named 'abd'
>>> a
False

This is the output after I made the change for the empty string check.

@htgoebel

This comment has been minimized.

Member

htgoebel commented Apr 25, 2018

This throws a ModuleNotFoundException if the package is not found, but the function returns the correct value anyways

Looks like you've spotted another bug. Well done! Maybe you fix #3483 first prior to continuing with #3481.

You can put both into the same pull-request of course - which should then have four commits: test for get_module_attribute, fix for get_module_attribute, test for is_module_satisfied, fix for is_module_satisfied.

@MagnumOpus21

This comment has been minimized.

Contributor

MagnumOpus21 commented Apr 27, 2018

Cool, I will finish this in the upcoming week.

htgoebel added a commit that referenced this issue Jun 28, 2018

utils/hooks: Fix result of is_module_satisfies() for packages not found.
Previously, the module returned True, even if the package is not installed 
on the system.

Includes a test-case.

This fixes #3428.

cowo78 pushed a commit to cowo78/pyinstaller that referenced this issue Dec 7, 2018

utils/hooks: Fix result of is_module_satisfies() for packages not found.
Previously, the module returned True, even if the package is not installed 
on the system.

Includes a test-case.

This fixes pyinstaller#3428.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment