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 a pyinstaller hook and simple test #387

Merged
merged 1 commit into from
Dec 18, 2020
Merged

Conversation

sirosen
Copy link
Member

@sirosen sirosen commented Dec 15, 2020

closes #384

This adds a pyinstaller hook and a simple test to the testsuite which tries to do a pyinstaller build of a miniscule (two line) app.

The pyinstaller hook is quite simple -- it just ensures that non-python files are included in the SDK package. The hook itself needs a setuptools entry point which identifies the hook location (one of the only bits of setuptools config pyinstaller respects).

In order for the testing to work, a few steps are needed:

  • add pyinstaller to 'development' requirements for py3+
  • add the test to the testsuite
  • skip the test if the environment doesn't support a pyinstaller build

Skipping in the right contexts is the trickiest part. It seems that pyinstaller needs a shared-library python build. That's the default for a lot of system packaging, but it's not guaranteed. Furthermore, pyenv builds won't include shared libraries by default, but will use static linking instead. Precisely because this is so finicky, we want to allow tests without it. That way, developer laptop testing can work even if the python env isn't "just so".

GitHub Actions should build pythons appropriately to run the pyinstaller test, so we have a good guarantee of things working.

We don't test the pyinstaller hook for py2 -- if it doesn't work for that version, that's fine (wontfix).


The one thing I wasn't sure about was where to put internal doc on this subject. Basically, I've imitated the sample repo from pyinstaller ( https://github.com/pyinstaller/hooksample ) and tried not to think too hard about this. But if we need more internal doc, maybe I could add globus_sdk/_pyinstaller/README? I'm just not certain it's necessary.

This adds a pyinstaller hook and a simple test to the testsuite which
tries to do a pyinstaller build of a miniscule (two line) app.

The pyinstaller hook is quite simple -- it just ensures that
non-python files are included in the SDK package. The hook itself
needs a setuptools entry point which identifies the hook location (one
of the only bits of setuptools config pyinstaller respects).

In order for the testing to work, a few steps are needed:
- add pyinstaller to 'development' requirements for py3+
- add the test to the testsuite
- skip the test if the environment doesn't support a pyinstaller build

Skipping in the right contexts is the trickiest part. It seems that
pyinstaller needs a shared-library python build. That's the default
for a lot of system packaging, but it's not guaranteed. Furthermore,
pyenv builds won't include shared libraries by default, but will use
static linking instead. Precisely because this is so finicky, we want
to allow tests without it. That way, developer laptop testing can work
even if the python env isn't "just so".

GitHub Actions should build pythons appropriately to run the
pyinstaller test, so we have a good guarantee of things working.

We don't test the pyinstaller hook for py2 -- if it doesn't work for
that version, that's fine (wontfix).
@sirosen sirosen requested a review from jaswilli December 15, 2020 02:56
@jaswilli
Copy link
Contributor

Without having any experience with pyinstaller this all seems pretty reasonable.

I can:

  • Produce a broken pyinstaller bundle by using the sdk as a dependency in a test script
  • See that I can build a working bundle by swapping in this branch.

Good enough for me.

@jaswilli jaswilli merged commit 0b9ab59 into master Dec 18, 2020
@jaswilli jaswilli deleted the support-pyinstaller branch December 18, 2020 05:12
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.

Support installation with pyinstaller
2 participants