Fix astropy so that it can be used in app bundles, and add CI to prevent regression#8795
Conversation
|
Some of the fixes will much easier to do by adding a |
|
@astrofrog - I'm bumping the milestone to the next bugfix, to help us see how we stand. Of course if any from there is ready before the release, they can be moved back to 4.0. |
|
Sounds good, I definitely won't have time to finish this for 4.0. |
astrofrog
force-pushed
the
fix-app-bundles
branch
from
42e2923 to
fef78d6
Compare
astrofrog
force-pushed
the
fix-app-bundles
branch
2 times, most recently
from
d303bc6 to
d6c1840
Compare
|
is this still something on the table post APE17? |
|
Yes this would still be nice to get working, I just haven't had time |
|
no worries, I just remilestoned it then. We can always move it back to the bugfix milestone when it's merged. |
astrofrog
force-pushed
the
fix-app-bundles
branch
from
a6ab5d5 to
576f5b8
Compare
|
Not release-critical, so removing milestone. |
astrofrog
changed the title
… can be used properly when bundled in a pyinstaller application. This also fixes relative imports that are not to same directory and should therefore be absolute imports. This fix was done because the helper script copies the astropy tests to a separate directory as they cannot be run from the application bundle.
|
This is now ready for review - all required fixes to make astropy work properly in bundles were done in separate PRs over the last year or so, this now just add a tox environment and associated Travis cron job to make sure that we don't break this capability. The Travis job is 7 minutes in total, so not a huge overhead, and I've only enabled it as a cron job to avoid slowing down any PRs. I had a temporary commit here to make sure it ran correctly, but have now removed the commit so the job won't show up here (and CI should therefore pass anyway). A side bonus is that this makes sure that the tests are able to be extracted as-is and run against an installed version of astropy (this was needed since pytest cannot discover tests in application bundles) - normally this should work, but there were a few relative imports that have crept in since we switched to absolute imports. Relative imports to the same directory are fine, but this fixes ones where the relative import went one or more levels up (which shouldn't be there). In any case, this is now ready for review and can be backported to 4.0.x. |
|
I think with this merged we can close #7052 |
mhvk
left a comment
mhvk
left a comment
There was a problem hiding this comment.
This looks nice - I like that it also tests that relative imports do not return. One tiny comment (I'll leave approval to someone who understands the pyinstaller scheme better)
There was a problem hiding this comment.
Based on success reported in #7052 (comment) (oh, wait, maybe the success is not related to this PR but still...) and @mhvk 's favorable review, I don't see any reason to hold this off any longer. Glancing at the code did not reveal anything shocking but I am not familiar with how Pyinstaller works, so I am approving but leaving it for another to merge.
Thanks!
|
The success over in #7052 is due to fixes that were done in other PRs in the process of doing this one - this current PR essentially just adds the regression test 😄 |
|
Since this has been approved, I'll go ahead and merge so that it doesn't go stale 😄 |
Fix astropy so that it can be used in app bundles, and add CI to prevent regression
Fix astropy so that it can be used in app bundles, and add CI to prevent regression
Fix astropy so that it can be used in app bundles, and add CI to prevent regression
Fix astropy so that it can be used in app bundles, and add CI to prevent regression
|
Wow, my first attempt to solve this was 7 years ago now. Amazing to see this finally done :) |
|
@embray thank you for starting this! |
5 participants
With applications like pyinstaller, it's easy to build application bundles for different platforms. Unfortunately, astropy doesn't work out of the box with these and requires some tweaking due to various issues. In the past, @embray actually opened a PR to fix this (#960) but this was never finished.
Since it would be nice for this to work cleanly, I've decided to start off by adding a CircleCI job that will test that this works correctly and that we don't regress in future. I'll then work on fixing issues, and will see if I can cherry pick or adapt any of the solutions from #960. So this is just a WIP for now.
UPDATE: this is now ready, see #8795 (comment)
EDIT: Fix #7052