search

AndrewAnnex / SpiceyPy

SpiceyPy: a Pythonic Wrapper for the SPICE Toolkit.
MIT License
398 stars 84 forks source link

change to src layout, arm builds #428

Closed AndrewAnnex closed 2 years ago

AndrewAnnex commented 3 years ago

combines #424, #427

pep8speaks commented 3 years ago

Hello @AndrewAnnex! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 276:1: E302 expected 2 blank lines, found 1

Line 141:1: W391 blank line at end of file Line 138:23: W291 trailing whitespace Line 129:71: W291 trailing whitespace Line 92:32: W291 trailing whitespace Line 56:11: W291 trailing whitespace Line 48:22: E225 missing whitespace around operator Line 41:8: E111 indentation is not a multiple of four Line 40:8: E111 indentation is not a multiple of four Line 36:1: E302 expected 2 blank lines, found 1

Line 147:5: E306 expected 1 blank line before a nested definition, found 0

Line 34:44: W291 trailing whitespace Line 33:1: E265 block comment should start with '# '

Line 260:78: E741 ambiguous variable name 'l'

Comment last updated at 2021-11-08 02:27:49 UTC
codecov[bot] commented 3 years ago

Codecov Report

Merging #428 (e5d8f69) into main (73bba45) will increase coverage by 0.00%. The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #428   +/-   ##
=======================================
  Coverage   99.87%   99.87%           
=======================================
  Files          12       12           
  Lines       14750    14756    +6     
=======================================
+ Hits        14732    14738    +6     
  Misses         18       18
Impacted Files Coverage Δ
src/spiceypy/__init__.py 100.00% <ø> (ø)
src/spiceypy/config.py 100.00% <ø> (ø)
src/spiceypy/spiceypy.py 99.69% <ø> (ø)
src/spiceypy/tests/gettestkernels.py 100.00% <ø> (ø)
src/spiceypy/tests/test_gettestkernels.py 100.00% <ø> (ø)
src/spiceypy/tests/test_spiceerrors.py 100.00% <ø> (ø)
src/spiceypy/tests/test_support_types.py 100.00% <ø> (ø)
src/spiceypy/tests/test_wrapper.py 100.00% <ø> (ø)
src/spiceypy/utils/callbacks.py 100.00% <ø> (ø)
src/spiceypy/utils/exceptions.py 100.00% <ø> (ø)
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 73bba45...e5d8f69. Read the comment docs.

AndrewAnnex commented 3 years ago

so another question is should I test sdist installs or how to properly do it now that python setup.py install is out of favor. It could be that I need to do pip install . or python -m build --sdist && pip install ./dist/*.tar.gz. I don't want to not be able to distribute sdists as I think that they are still useful for offline cspice installs.

pganssle commented 3 years ago

so another question is should I test sdist installs or how to properly do it now that python setup.py install is out of favor. It could be that I need to do pip install . or python -m build --sdist && pip install ./dist/*.tar.gz. I don't want to not be able to distribute sdists as I think that they are still useful for offline cspice installs.

Yeah I don't usually test my sdists separate from the repository though I do think it's a good idea to test things as you expect them to be installed, but I don't think that setup.py install makes any difference here. You can use pip install in place of setup.py install, it works on an unpacked sdist or on the sdist directly, so in some ways it's easier to do it that way.

AndrewAnnex commented 3 years ago

@pganssle this is really breaking my brain, mostly because I can't get pycharm to ever suspend the task inside distutils/command/build_py.py. Past a certain code point, the process seems to just go on forever without hitting any additional break points, so maybe there is some issue with pycharm's debugger causing this. Even adding breakpoints via breakpoint() to use pdb just caused other errors to occur. The recent pr build for 906a765 mostly worked because I added back some parameters to setup in setup.py that seem to be entirely ignored from setup.cfg, however sdist then packaged the shared library erroneously. Locally I made a slight change to exclude the shared library using the MANIFEST.in which then causes the package directory not existing bug to reoccur. One possible solution is to only distribute wheels on pypi, but that is very dissatisfying as sdist installs have been working for years.

I may update my issue on setuptools to say that setup.cfg is partially ignored if some options are provided to setup() in setup.py, but I think there is a larger issue of setuptools not being debuggable, that should be a concern I think to the maintainers.

AndrewAnnex commented 2 years ago

well, this was a bit of a mess. turns out I had a chdir inside get_spice.py that wasn't undone causing setuptools to end up in the wrong dir at the wrong time, although I haven't technically proved this to be the case because I hadn't got the debugger to work. I am still not sure what finally made me try this, possibly it was when I noticed that the sdist install wasn't working properly despite everything I was doing. If I could have debugged setuptools/pip properly, that would have saved me most of the commits and wasted ci time above... I may want to investigate caching the shared libcspice files on the pr and merge builds side to save some ci time and making the aarch64 cibuildwheel cache more robust to save the hour + it takes to compile cspice in qemu