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

Linux usb hook fixes #3269

Merged
merged 4 commits into from Mar 12, 2018

Conversation

Projects
None yet
2 participants
@charlesnicholson
Contributor

charlesnicholson commented Feb 3, 2018

This PR addresses the issues raised in #2633:

1. Enable, always, library_test::test_usb. Make sure a backend exists on Travis by adding libusb-1.0 to the package list. The code tests for the presence of a usb backend, which on some versions of Linux can crash Python when finalization happens (libusb_exit crashes), and seems related to whether libusb was loaded on a worker thread. Anyway, since asking if we have USB crashes, just ensure that a USB backend actually exists. This seems reasonable, since requirements-library.txt already installs pyusb and the expectation of pyusb is that you can do USB work.

  1. Ugh, ok, it's really challenging to tiptoe around the linux pyusb issue. Currently the entire test_usb function is skipped for linux because pyusb causes crashes on the version of linux that Travis uses. We see this same crash at work on Ubuntu 14.04LTS machines. It has nothing to do with PyInstaller; I suspect it's a bug in pyusb around finalizing and libusb. I ended up just clarifying the existing test_usb function, and enabling the tests for mac, which doesn't exhibit this crash. There are a bunch of places this can crash: The generated functional test in test_libraries.py can crash and the hook can crash. Even with the environment variable trick described below in 3., the functional test still crashes because it imports pyusb and works with it. I don't think there's a way to work around this without fixing the core issue in pyusb. Bummer.

  2. When using pyusb to enumerate libraries, normalize them by calling os.path.basename on them, and then get their absolute paths via _resolveCtypesImports. On Mac, pyusb returns absolute paths to so/dylib files. On Linux, pyusb returns just the filename, which causes PyInstaller to fail. This new approach works in both scenarios.

  3. If the user sets the environment variable PYINSTALLER_USB_HOOK_SKIP_PYUSB_DISCOVERY, hook-usb.py will not use pyusb's backend mechanism to discover libraries. This lets users navigate around the pyusb crash if they choose. Note that the default behavior is still to use pyusb, so the existing behavior should remain unchanged.

  4. (cleanup) Hoist the Cygwin dll path up to the top of the file, it seems less direct to use the pyusb discovery method here, since Cygwin is a much more controlled environment and there really aren't any other options. Why bother searching pyusb when you know exactly what dll's should be used? In any case, if those Cygwin dll's fail to load, pyusb interrogation still happens in case there's some other strange place Cygwin usb dll's can come from.

  5. (cleanup) Remove the number of special cases: binaries is now always a list containing the results of _resolveCtypesImports, and only at the very end of the function is the first list element selected, validated, and normalized into the form that PyInstaller expects from its hooks. This doesn't change the behavior, but it reduces the number of code paths through the file and makes each path more similar.

  6. Add .eggs/ and .pytest_cache/ to the .gitignore file; they showed up in my unstaged list after doing my first unit + functional test run, and it looks like they're not intended to be committed.

# if nothing found, try to use our custom mechanism
if not binaries:
# Try to resolve your libusb libraries in the following order:

This comment has been minimized.

@charlesnicholson

charlesnicholson Feb 4, 2018

Contributor

These comments seemed redundant.

@charlesnicholson

This comment has been minimized.

Contributor

charlesnicholson commented Feb 4, 2018

I'm seeing some build failures that don't look like they're related to my work. Python 2.7 Linux, 3.4 Linux, 3.6 Mac, and 3.6 Linux all pass.

Maybe a core team member or maintainer could help me understand if these are related, and how to move forward?

Python 3.3 Linux:

$ py.test -n 3 --maxfail 3 tests/functional/test_libraries.py tests/functional/test_hooks
usage: py.test [options] [file_or_dir] [file_or_dir] [...]
py.test: error: unrecognized arguments: -n tests/functional/test_libraries.py tests/functional/test_hooks
  inifile: None
  rootdir: /home/travis/build/pyinstaller/pyinstaller
The command "py.test -n 3 --maxfail 3 tests/functional/test_libraries.py tests/functional/test_hooks" exited with 2.

Python 3.5 Linux:

=================================== FAILURES ===================================
________________________ test_pandas_extension[onefile] ________________________
[gw0] linux -- Python 3.5.4 /home/travis/virtualenv/python3.5.4/bin/python
pyi_builder = <conftest.AppBuilder object at 0x7fd4643e2ef0>
    @importorskip('pandas')
    def test_pandas_extension(pyi_builder):
        # Tests that C extension 'pandas.lib' is properly bundled. Issue #1580.
        pyi_builder.test_source(
            """
            from pandas.lib import is_float
            assert is_float(1) == 0
>           """)
E       Failed: Running exe /tmp/pytest-of-travis/pytest-0/popen-gw0/test_pandas_extension_onefile_0/dist/test_pandas_extension failed with return-code 1.
/home/travis/build/pyinstaller/pyinstaller/tests/functional/test_libraries.py:806: Failed
----------------------------- Captured stdout call -----------------------------
------- Starting build. -------
Syntax error in  /home/travis/virtualenv/python3.5.4/lib/python3.5/site-packages/jinja2/asyncsupport.py
("'yield' inside async function", ('/home/travis/virtualenv/python3.5.4/lib/python3.5/site-packages/jinja2/asyncsupport.py', 35, 12, '            yield event\n'))
------- Build finshed, now running executable. -------
RUNNING:  '/tmp/pytest-of-travis/pytest-0/popen-gw0/test_pandas_extension_onefile_0/dist/test_pandas_extension' , args:  ['./test_pandas_extension']
TIMED OUT:  '/tmp/pytest-of-travis/pytest-0/popen-gw0/test_pandas_extension_onefile_0/dist/test_pandas_extension' , args:  ['./test_pandas_extension']
----------------------------- Captured stderr call -----------------------------
@ghost

This comment has been minimized.

ghost commented Feb 4, 2018

I've already submitted gh-3026 and gh-3109, and @hugovk already submitted gh-3060 to address some of the test failures; it's a mystery why these haven't yet been merged. It's a mystery why Python 3.3 is still being tested.

@charlesnicholson

This comment has been minimized.

Contributor

charlesnicholson commented Feb 5, 2018

Thanks for the response. I'll address feedback on this PR when it comes in, and then stand by for maintainers to merge fixes to the current CI system.

@charlesnicholson charlesnicholson force-pushed the charlesnicholson:linux-usb-hook-fixes branch from 1cc1606 to db0a5de Feb 7, 2018

@charlesnicholson

This comment has been minimized.

Contributor

charlesnicholson commented Feb 16, 2018

Ping! Are any maintainers interested in reviewing this? Also, it looks from recent PRs that Python 3.3 is still broken in both Travis and AppVeyor, so getting a green build isn't possible.

@htgoebel @xoviat

@ghost

This comment has been minimized.

ghost commented Feb 16, 2018

We cannot review this PR until the CI system is fixed. There is one person who can click the merge button, and that person is not me.

@charlesnicholson

This comment has been minimized.

Contributor

charlesnicholson commented Feb 16, 2018

Bummer. I'd be happy to address feedback even while the CI system is busted, at least. Alternately I'm happy to wait; if no code is being merged then there's not much chance of ending up in merge hell.

@ghost

This comment has been minimized.

ghost commented Feb 21, 2018

@charlesnicholson Can you open and close this PR to restart the tests?

@charlesnicholson

This comment has been minimized.

Contributor

charlesnicholson commented Feb 23, 2018

The build system looks operational again, great! This PR is passing all the checks, so I'll wait for feedback.

@htgoebel

htgoebel requested changes Mar 1, 2018 edited

Thank you for all the work. I reviewed the code and have some comments, most of which are only related to style.

However, the commit-history has room for improvement. Please clean up the commits, following the Commit guideline and the Commit message guideline. This will help keeping code and changes comprehensible for years. https://pyinstaller.readthedocs.io/en/stable/development/pull-requests.html#updating-pull-request might be helpful. Thank you

.gitignore Outdated
@@ -7,6 +7,8 @@ __pycache__/
/*.egg
/*.egg-info/
/*.egg-link
.eggs/
.pytest_cache/

This comment has been minimized.

@htgoebel

htgoebel Mar 1, 2018

Member

This should be rooted in the project directory, I assume, thus start with a slash.

Please put this into a commit of its own.

# loading usb.core.find in this script crashes Ubuntu 14.04LTS,
# let users circumvent pyusb discovery with an environment variable.
skip_pyusb_discovery = \
getenv('PYINSTALLER_USB_HOOK_SKIP_PYUSB_DISCOVERY') is not None

This comment has been minimized.

@htgoebel

htgoebel Mar 1, 2018

Member
  1. Setting en environment variable to an empty string (FOO=) instead of unsetting it (unset FOO) thus I suggest do simply use
    skip_pyusb_discovery = bool(getenv('PYINSTALLER_USB_HOOK_SKIP_PYUSB_DISCOVERY'))
    What do you think?

  2. How does this "crash" show up? Can we catch an exception in that case and print a message telling to set the env-var? This trick is not documented anywhere.

  3. Anyway please put this into a commit of its own.

# special-case cygwin, try to use the explicit cygwin usb dlls.
if is_cygwin:
bins = ['cygusb-1.0-0.dll', 'cygusb0.dll']
binaries = _resolveCtypesImports(bins)

This comment has been minimized.

@htgoebel

htgoebel Mar 1, 2018

Member

Please put this int a commit of its own.

# try to resolve the library names to absolute paths.
binaries = _resolveCtypesImports(backend_lib_basenames)
except (ValueError, usb.core.USBError) as exc:

This comment has been minimized.

@htgoebel

htgoebel Mar 1, 2018

Member

Please remove all the blank lines above. PyInstaller is using this style.

binaries = [(binaries[0][1], '')]
backend_library_basenames.append(os.path.basename(libname))
if backend_library_basenames:

This comment has been minimized.

@htgoebel

htgoebel Mar 1, 2018

Member

No blank line here.

binaries = _resolveCtypesImports(backend_library_basenames)
# However a supported usb library was found, validate it and normalize it.

This comment has been minimized.

@htgoebel

htgoebel Mar 1, 2018

Member

"However" is the wrong word, it means something like "but", "yet". IMHO this should be "No matter how"

@charlesnicholson charlesnicholson force-pushed the charlesnicholson:linux-usb-hook-fixes branch 2 times, most recently from 0956491 to 930179b Mar 2, 2018

@charlesnicholson

This comment has been minimized.

Contributor

charlesnicholson commented Mar 2, 2018

@htgoebel PTAL, all feedback responded to. If you approve, though, please don't merge yet! I want to run a bunch of tests while this PR grinds through the automated build.

BTW "However" has a few meanings in English. As you mentioned, one meaning is contradictory and similar to "but" and "yet". The other common meaning is close to "in whatever way" or "through whatever means". Both forms are very commonly used in at least American and British English.

At any rate, I realized that "If" would work just as well and lead to a shorter sentence:

However a supported usb library was found, validate it and normalize it.
If a supported usb library was found, validate it and normalize it.

So I went with that. Hopefully it's more consumable by people everywhere :)

@charlesnicholson

This comment has been minimized.

Contributor

charlesnicholson commented Mar 2, 2018

Regarding the crash and environment variable:

Unfortunately, the crash comes from native code in libusb by way of the pyusb module finalization. When the hook-usb module exits, the pyusb module finalizes and eventually calls the native function libusb_exit(), which crashes. Unloading so / dll files from Python is subtle, complex, and a common source of crashes. It's likely that this particular crash comes from a double-shutdown, a shutdown from a different thread than the one that loaded it, a shutdown from any non-primary thread, a shutdown from a different module, or any number of things.

If you want to see the (212 deep!) crash stack, check out this gist:
https://gist.github.com/charlesnicholson/24167145427a04886d30060df68e0e7d

There's no portable way in Python to handle internal native seg faults. Python 3.3 introduced the faulthandler module but as I understand it it doesn't work on all versions of Python that PyInstaller supports? Either way, integrating something like that is more work than I can take on right now, but it might be a good PR in the future.

I agree that a magic and undocumented environment variable is not optimal, but at least it's better than nothing, that was my motivation. For people who are so motivated to understand the usb-hook.py source code, at least they'll see that option. Do you agree?

@charlesnicholson

This comment has been minimized.

Contributor

charlesnicholson commented Mar 2, 2018

@xoviat @htgoebel It looks like the build may be broken again? Python3.6 is failing with what look like unrelated errors to this PR:

https://travis-ci.org/pyinstaller/pyinstaller/jobs/348301413

@charlesnicholson

This comment has been minimized.

Contributor

charlesnicholson commented Mar 2, 2018

Windows failure (https://ci.appveyor.com/project/matysek/pyinstaller/build/3114/job/cqq35uw9kxeyn4lk):

================================== FAILURES ===================================
_________________________ TestPythonBehaviour.testSub _________________________
[gw1] win32 -- Python 3.5.3 c:\python35\python.exe
self = <test_modulegraph.test_setuptools_nspkg.TestPythonBehaviour testMethod=testSub>
    def testSub(self):
>       m = self.importModule('nspkg.nssubpkg.sub')
tests\unit\test_modulegraph\test_setuptools_nspkg.py:94: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
tests\unit\test_modulegraph\test_setuptools_nspkg.py:85: in importModule
    self.fail("import of %r failed"%(name,))
E   AssertionError: import of 'nspkg.nssubpkg.sub' failed
---------------------------- Captured stdout call -----------------------------
Traceback (most recent call last):
  File "<string>", line 4, in <module>
ImportError: No module named 'nspkg'
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
  File "<string>", line 6, in <module>
ImportError: No module named 'nspkg'
______________________ TestPythonBehaviour.testToplevel _______________________
[gw1] win32 -- Python 3.5.3 c:\python35\python.exe
self = <test_modulegraph.test_setuptools_nspkg.TestPythonBehaviour testMethod=testToplevel>
    def testToplevel(self):
>       m = self.importModule('nspkg.module')
tests\unit\test_modulegraph\test_setuptools_nspkg.py:90: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
tests\unit\test_modulegraph\test_setuptools_nspkg.py:85: in importModule
    self.fail("import of %r failed"%(name,))
E   AssertionError: import of 'nspkg.module' failed
---------------------------- Captured stdout call -----------------------------
Traceback (most recent call last):
  File "<string>", line 4, in <module>
ImportError: No module named 'nspkg'
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
  File "<string>", line 6, in <module>
ImportError: No module named 'nspkg'
= 2 failed, 543 passed, 104 skipped, 25 
@charlesnicholson

This comment has been minimized.

Contributor

charlesnicholson commented Mar 2, 2018

Travis failure (3.6) https://travis-ci.org/pyinstaller/pyinstaller/jobs/348301413:

Error: python 2.7.14 is already installed
To upgrade to 3.6.4_3, run `brew upgrade python`
/Users/travis/.travis/job_stages: line 60: python3: command not found
/Users/travis/.travis/job_stages: line 61: venv/bin/activate: No such file or directory
The command "if [[ $TRAVIS_OS_NAME == 'osx' ]]; then
      brew update;
      brew install python3;
      python3 -m venv venv;
      source venv/bin/activate;
  fi
  " failed and exited with 1 during .
Your build has been stopped.
/Users/travis/.travis/job_stages: line 166: shell_session_update: command not found

charlesnicholson added some commits Mar 2, 2018

Tests: Simplify USB library tests.
'usb.core.find()' raises 'usb.core.NoBackendError' in the case where there
isn't a USB backend, which is enough to determine whether the test
should be attempted. Similarly, the test only needs to call
'usb.core.find()'.
Hooks: Guard pyusb-based discovery with an environment variable.
Add a check for an environment variable named
PYINSTALLER_USB_HOOK_SKIP_PYUSB_DISCOVERY. If the environment
variable is set and non-empty, the USB hook will skip pyusb-based
backend discovery and prefer the filesystem library search method
instead.

On some Linux machines (at least some Ubuntu 14.04 deployments),
unloading pyusb causes a segfault inside of libusb_exit() and crashes
the entire Python process. When this happens inside of the USB hook,
it crashes the PyInstaller hook, which crashes the entire PyInstaller
process.

There is no portable way to detect and gracefully recover from a
native segfault crash in Python. This approach is not ideal, but at
least provides a way for users to avoid crashes through
configuration.
Hooks: Clean up the USB hook logic.
* Normalize logic: however libraries are discovered, they are
accumulated into the 'binaries' list. At the end of the hook, they
are validated and transformed into the structure that the hook engine
requires.

* Put Cygwin logic through the same code path that regular filesystem
library discovery uses, just with a different set of candidate
filenames.

* Update the comments to be more descriptive, add some small grammar
fixes.

* Delete spurious whitespace in response to PR feedback.

@charlesnicholson charlesnicholson force-pushed the charlesnicholson:linux-usb-hook-fixes branch from 930179b to cefc3e5 Mar 11, 2018

@charlesnicholson

This comment has been minimized.

Contributor

charlesnicholson commented Mar 12, 2018

@htgoebel build looks fixed, all checks passing, PTAL when you get a chance!

@htgoebel

Thanks you for updating the pull-request. These are exemplary commit messages :-) You are great!

@htgoebel htgoebel merged commit d9ca1d3 into pyinstaller:develop Mar 12, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@charlesnicholson charlesnicholson deleted the charlesnicholson:linux-usb-hook-fixes branch Mar 12, 2018

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

@htgoebel htgoebel added the hooks label Sep 2, 2018

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