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

Do not run the code generator if PYERFA_USE_SYSTEM_LIBERFA=1 (Closes #38) #39

Merged
merged 2 commits into from
Jul 7, 2020

Conversation

avalentino
Copy link
Member

The proposed change do not run the code generator (erfa_generator.py) when the user requires to use the system erfa library (PYERFA_USE_SYSTEM_LIBERFA=1).

NOTE: the change only works if the core.py and ufunc.c have been already generated in some way (e.g. if one uses the source tarball available on PyPi.io). It is not expected to work for a fresh git clone.
An error is raised if the core.py and ufunc.c files are not found.

@avalentino
Copy link
Member Author

Is this also related to #37?

@mhvk
Copy link
Contributor

mhvk commented Jul 6, 2020

@avalentino - #37 is about the python wrappers only. But agreed with the issue raised here, and in fact a general problem: if one uses the system files, the system library may be out of date with the C code used here to generate the ufuncs. Though I guess there is at least some guarding against that by ufunc.c relying on the erfa header files (it is a bit the erfa docstrings are not part of the headers!).

About the PR proper, wouldn't it make more sense for the logic to be that ufunc.c and core.py only get generated if they do not exist already or are out of date? Though I must admit I don't know how to do the latter in an extension (would be easy in a Makefile...). But more simply, perhaps the logic can be that if the system library is used and the files exists, the creation step is skipped? Though then again, perhaps what you have here is cleaner.

@avalentino avalentino force-pushed the build-with-system-liberfa branch from a3e528e to 958e2f8 Compare July 6, 2020 16:16
@avalentino
Copy link
Member Author

@avalentino - #37 is about the python wrappers only. But agreed with the issue raised here, and in fact a general problem: if one uses the system files, the system library may be out of date with the C code used here to generate the ufuncs. Though I guess there is at least some guarding against that by ufunc.c relying on the erfa header files (it is a bit the erfa docstrings are not part of the headers!).

if ufuncs.c includes new functions that are not present in the system erfa, then pyerfa should not build at all (assuming PYERFA_USE_SYSTEM_LIBERFA=1).
On the other hand if the system erfa is newer than the one used to generate ufuncs.c then in the worst case it can happen that pyerfa will miss some new erfa function (but it in any case will miss new functions if one uses bundled erfa sources to generate ufuncs.c).
In all cases it should never happen that you successfully build pyerfa and then it crashes at runtime (if the system liberfa does not changes).

About the PR proper, wouldn't it make more sense for the logic to be that ufunc.c and core.py only get generated if they do not exist already or are out of date? Though I must admit I don't know how to do the latter in an extension (would be easy in a Makefile...). But more simply, perhaps the logic can be that if the system library is used and the files exists, the creation step is skipped? Though then again, perhaps what you have here is cleaner.

Just pushed a change that should go in de direction you described (I hope I have understood correctly your request).

@mhvk
Copy link
Contributor

mhvk commented Jul 7, 2020

@avalentino - this looks good but let me push a fix for the apt-based CI run, so that we can check it works also on our CI run. Also, this seems like a PR for which input from @sergiopasra would be useful - would be bad if it turns out that to make using the system library easier on one distribution, it becomes harder on another!

@mhvk
Copy link
Contributor

mhvk commented Jul 7, 2020

@avalentino - see #40 - if we put that in and you rebase, then hopefully all tests will pass here too.

@avalentino avalentino force-pushed the build-with-system-liberfa branch from 958e2f8 to 83a512c Compare July 7, 2020 17:26
@avalentino
Copy link
Member Author

@mhvk just rebased, now all tests seem to work.
I will wait for the merge of #40 before merging this one.

Copy link
Contributor

@mhvk mhvk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice now!

@avalentino avalentino merged commit a9dec94 into liberfa:master Jul 7, 2020
@avalentino avalentino deleted the build-with-system-liberfa branch July 7, 2020 18:19
eteq added a commit to eteq/astropy that referenced this pull request Oct 16, 2020
eteq added a commit to astropy/astropy that referenced this pull request Oct 21, 2020
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.

2 participants