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

Improve setup.py and drop the MANIFEST #56

Merged
merged 1 commit into from
Jan 5, 2023
Merged

Improve setup.py and drop the MANIFEST #56

merged 1 commit into from
Jan 5, 2023

Conversation

taldcroft
Copy link
Member

@taldcroft taldcroft commented Jan 5, 2023

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
Copy link
Contributor

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

@javierggt
Copy link
Contributor

javierggt commented Jan 5, 2023

I verified that this builds in the build workflow.

@taldcroft taldcroft merged commit 2c12c8d into master Jan 5, 2023
@taldcroft
Copy link
Member Author

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.

@taldcroft taldcroft deleted the better-setup branch January 5, 2023 16:20
@jeanconn
Copy link
Contributor

jeanconn commented Jan 5, 2023

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
Copy link
Contributor

jeanconn commented Jan 5, 2023

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.

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.

3 participants