-
-
Notifications
You must be signed in to change notification settings - Fork 573
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
PyInstaller Support for SunPy #5224
Changes from 4 commits
b9228cf
c3ebb25
17004e1
1f4d945
fbb7a63
bf4da06
9924d42
6497950
56d76e8
b7e2022
081351d
9099bbd
e6d6991
5f8da30
93a1c4f
9221125
555e993
cfb5c9f
e4716a6
2ac7896
288e46c
d0085e0
b936a28
5be57b2
c1a0288
8b92179
eadc990
21580e0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
# ------------------------------------------------------------------ | ||
# Copyright (c) 2020 PyInstaller Development Team. | ||
# | ||
# This file is distributed under the terms of the GNU General Public | ||
# License (version 2.0 or later). | ||
# | ||
# The full license is available in LICENSE.GPL.txt, distributed with | ||
# this software. | ||
# | ||
# SPDX-License-Identifier: GPL-2.0-or-later | ||
# ----------------------------------------------------------------- | ||
|
||
from PyInstaller.utils.hooks import collect_data_files, collect_submodules | ||
|
||
# Importing Non-python files | ||
datas = collect_data_files('sunpy') | ||
hiddenimports = collect_submodules('sunpy') |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
import os | ||
import sys | ||
|
||
sys.exit(os.system('pytest -p no:warnings --doctest-rst -m "not mpl_image_compare" --pyargs sunpy')) | ||
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
Added hook file and tests for using PyInstaller with SunPy. | ||
jeffreypaul15 marked this conversation as resolved.
Show resolved
Hide resolved
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -179,3 +179,30 @@ install_command = pip install --no-deps {opts} {packages} | |
commands = | ||
conda list | ||
{env:PYTEST_COMMAND} {posargs} | ||
|
||
[testenv:pyinstaller] | ||
nabobalis marked this conversation as resolved.
Show resolved
Hide resolved
|
||
description = check that sunpy can be included in a pyinstaller bundle | ||
changedir = .pyinstaller | ||
deps = | ||
# The developer version of PyInstaller doesn't work yet because it | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this copied out of astropy? Is this still true for us, this PR linked here is pretty old. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I've taken AstroPy as a reference, I think this wouldn't be a problem for Sunpy though. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ideally if we can test with a released version that would be better. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh yeah sorry I undid these changes accidentally. |
||
# requires https://github.com/pyinstaller/pyinstaller/pull/4888 - | ||
# so for now we pin to the latest working commit. | ||
git+https://github.com/pyinstaller/pyinstaller.git@c111ec5c#egg=PyInstaller | ||
# PyInstaller is not yet compatible with Matplotlib 3.3 so we pin | ||
# Matplotlib to an older version for now - see the following issue | ||
# for more details: https://github.com/pyinstaller/pyinstaller/issues/5004 | ||
matplotlib<3.3 | ||
extras = | ||
tests | ||
net | ||
all | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't need these here do we? All we are running from tox is pyinstaller? or does pyinstaller copy from your current env? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Tox needs these to ensure that the tests run fine by itself, PyInstaller then copies the current environment (tox) and creates the executable based on the environment it was running on. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we still need all of these now? all, encompasess net. can we get away with just all and test? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All doesn't seem to load Beautiful Soup which is part of net, this gave me an error locally I might need to check what else is required from tests and add them individually There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It should, I wonder what the cause is. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My apologies, must've been some local issue, we don't need net |
||
commands = | ||
pyinstaller --onefile run_sunpy_tests.py \ | ||
--distpath . \ | ||
Cadair marked this conversation as resolved.
Show resolved
Hide resolved
|
||
--additional-hooks-dir hooks \ | ||
--exclude-module tkinter \ | ||
--hidden-import pytest \ | ||
--hidden-import pytest_doctestplus.plugin \ | ||
--hidden-import pytest_mpl.plugin | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are these just here to make it possible to run the tests? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is to create the executable file |
||
|
||
./run_sunpy_tests | ||
Cadair marked this conversation as resolved.
Show resolved
Hide resolved
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No no no! This will just run regular unfrozen pytest so it's not testing anything except
os.system()
! Usepytest.main()
instead.However, without an
import sunpy
in there, PyInstaller will have no reason to think thatsunpy
is a dependency so it won't collect it (or run the hook) unless you force it to with a--hiddenimport=sunpy
or putimport sunpy
in the script.That's still probably not the end of the story though - it depends: Does
import sunpy
load everything so that you can access any public corner without additional import statements or are you expected to usefrom sunpy.foo.bar import xyz
to pull the bits you need out? If it's the latter then PyInstaller will considersunpy.foo.bar
to be unneeded unless you add it as ahiddenimport
or an explicit reference inrun_sunpy_tests.py
too. (Note, this won't apply if you opt to keep thecollect_submodules("sunpy")
in the hook. Usingcollect_submodules("sunpy")
does simplify testing - at the expense of making builds huge.)It's quite likely that you'll have to have more than one test script, each importing and testing a different corner of
sunpy
, and each using only the optional dependencies that that corner requires.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your help on this @bwoodsend.