Skip to content

Remove ability to link against external installation of CFITSIO - #14311

Merged
saimn merged 1 commit into
astropy:mainfrom
astrofrog:remove-external-cfitsio-linking
Jan 24, 2023
Merged

Remove ability to link against external installation of CFITSIO#14311
saimn merged 1 commit into
astropy:mainfrom
astrofrog:remove-external-cfitsio-linking

Conversation

@astrofrog

@astrofrog astrofrog commented Jan 24, 2023

Copy link
Copy Markdown
Member

Description

We now only use a few files from CFITSIO including functions that aren't really public API from CFITSIO, so it makes sense to no longer offer the option to link against an external installation of CFITSIO.

See discussion in #14252 (comment)

cc @saimn @olebole

Checklist for package maintainer(s)

This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.

  • Do the proposed changes actually accomplish desired goals?
  • Do the proposed changes follow the Astropy coding guidelines?
  • Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
  • Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
  • Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see "When to rebase and squash commits".
  • Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the Extra CI label. Codestyle issues can be fixed by the bot.
  • Is a change log needed? If yes, did the change log check pass? If no, add the no-changelog-entry-needed label. If this is a manual backport, use the skip-changelog-checks label unless special changelog handling is necessary.
  • Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
  • Is a milestone set? Milestone must be set but astropy-bot check might be missing; do not let the green checkmark fool you.
  • At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate backport-X.Y.x label(s) before merge.

@astrofrog
astrofrog requested a review from saimn as a code owner January 24, 2023 09:57
@olebole

olebole commented Jan 24, 2023

Copy link
Copy Markdown
Member

I feel fine with it, as it would be overkill to link to cfitsio just for the few compression routines. Removing this also gives the freedom to design the API optimally for astropy if needed. 👍

saimn
saimn approved these changes Jan 24, 2023
View reviewed changes

@saimn saimn left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Great, thanks @astrofrog and thanks @olebole for the feedback, lets' merge then.

@saimn saimn added this to the v5.3 milestone Jan 24, 2023
@saimn
saimn merged commit ce12627 into astropy:main Jan 24, 2023
@pllim

pllim commented Jan 24, 2023

Copy link
Copy Markdown
Member

Let me try to understand, so now if someone wants to use astropy and cfitsio, they would have two different cfitsio installed -- one is the partially bundled one and one is a separate installation?

@astrofrog

astrofrog commented Jan 24, 2023 •
edited
Loading

Copy link
Copy Markdown
Member Author

No, someone installing astropy doesn't have CFITSIO 'installed' as a result, they just have a Python C extension that has been linked against a few files that happen to have been extracted from CFITSIO (in fact this was the case before too). The CFITSIO code in astropy is not (and never was) publicly exposed.

This was referenced Feb 2, 2023
Merged
Closed
bmwiedemann pushed a commit to bmwiedemann/openSUSE that referenced this pull request Jul 1, 2023
bnavigator @bmwiedemann
Update python-astropy to version 5.3 / rev 42 via SR 1096299
4405778 
https://build.opensuse.org/request/show/1096299
by user bnavigator + dimstar_suse
- Update to 5.3
  * New flat astropy.cosmology classes
  * New union operators for Table
  * Efficient data access for compressed FITS files
  * Added support for NOCOMPRESS for compressed FITS files
  * New fraction option for representing units as strings
  * Change in order in unit string representations
  * Support for collapse operations on arbitrary axes in nddata
  * Refresh cached observatory site registry for EarthLocation
    methods
  * Support for collapse operations on arbitrary axes in nddata
  * New LombScargleMultiband class for multiband datasets
- Remove unbundling of cfitsio -- gh#astropy/astropy#14311
@Hellseher Hellseher mentioned this pull request Nov 1, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@saimn saimn saimn approved these changes

Assignees

No one assigned

Labels

Docs installation io.fits testing

Projects

None yet

Milestone

v5.3

Development

Successfully merging this pull request may close these issues.

4 participants

@astrofrog @olebole @pllim @saimn