sot / chandra_time

Convert between various time formats relevant to Chandra.
https://sot.github.io/Chandra.Time
BSD 3-Clause "New" or "Revised" License
4 stars 1 forks source link

Improve setup.py and drop the MANIFEST #56

Closed taldcroft closed 1 year ago

taldcroft commented 1 year ago

Description

Fixes (hopefully) the problem discussed on Slack "we are getting this error when building chandra_time on Windows" in #aspect_team. The original strategy of making cythonize into a lambda function that returns None was silly.

I also noticed when building that the MANIFEST.in file referenced non-existent files and was generating warnings when building. But the distribution was still working so I don't think we need the manifest. Even python setup.py sdist seems to be fine, see testing below.

Interface impacts

None.

Testing

Unit tests

No code changes.

Functional tests

Install test

I did the usual of removing the build dir and installing to a dev ska and running tests. All good.

Version

$ python setup.py --version
4.1.1.dev1+gc9e5533

MANIFEST.in

The .pyx and .h files are automatically included even without a manifest.


$ python setup.py sdist
...
copying chandra_time/Time.py -> chandra_time-4.1.1.dev1+gb53457a/chandra_time
copying chandra_time/XTime.cc -> chandra_time-4.1.1.dev1+gb53457a/chandra_time
copying chandra_time/XTime.h -> chandra_time-4.1.1.dev1+gb53457a/chandra_time
copying chandra_time/__init__.py -> chandra_time-4.1.1.dev1+gb53457a/chandra_time
copying chandra_time/_axTime3.cpp -> chandra_time-4.1.1.dev1+gb53457a/chandra_time
copying chandra_time/_axTime3.pyx -> chandra_time-4.1.1.dev1+gb53457a/chandra_time
copying chandra_time/axTime3.cc -> chandra_time-4.1.1.dev1+gb53457a/chandra_time
copying chandra_time/axTime3.h -> chandra_time-4.1.1.dev1+gb53457a/chandra_time
copying chandra_time/axTime3.py -> chandra_time-4.1.1.dev1+gb53457a/chandra_time
...
javierggt commented 1 year ago

It took me a while to realize that the issue was with the python setup.py --version call...

javierggt commented 1 year ago

I verified that this builds in the build workflow.

taldcroft commented 1 year ago

It took me a while to realize that the issue was with the python setup.py --version call...

Me too. It came to me out of nowhere when I wasn't even really thinking about this. Brain.

jeanconn commented 1 year ago

Sorry, I guess I just need to write things out more. As in my slack comment "From the setup.py it is still supposed to return version without the cython calls, so the failure on OSX to end up with version (‘chandra_time-None-py38_0.tar.bz2’) still seems odd." should have said "Our build process does a preliminary step where "python setup.py --version" is called, and the setup.py has an if statement to do that without needing Cython, but it doesn't appear to be working now on OSX or Windows". I still don't know why this stopped working but removing the lambda seems great.

jeanconn commented 1 year ago

Oh I get, "python setup.py --version" started to fail because the lambda was incompatible with the language_level=3 piece that was the change.