Skip to content

Update cfitsio to v3.43: includes critical bug fix - #7274

Merged
eteq merged 2 commits into
astropy:masterfrom
drdavella:update-cfitsio
Mar 9, 2018
Merged

eteq merged 2 commits into
astropy:masterfrom
drdavella:update-cfitsio

Conversation

@drdavella

Copy link
Copy Markdown
Contributor

This fixes #7272. I'll open a separate PR for backporting to 2.0.x.

@drdavella drdavella added this to the v3.0.1 milestone Mar 9, 2018
@astropy-bot

astropy-bot Bot commented Mar 9, 2018 •

Copy link
Copy Markdown

Hi there @drdavella 👋 - thanks for the pull request! I'm just a friendly 🤖 that checks for issues related to the changelog and making sure that this pull request is milestoned and labeled correctly. This is mainly intended for the maintainers, so if you are not a maintainer you can ignore this, and a maintainer will let you know if any action is required on your part 😃.

Everything looks good from my point of view! 👍

If there are any issues with this message, please report them here.

@pllim pllim added the external PRs and issues related to external packages vendored with Astropy (astropy.extern) label Mar 9, 2018
@MSeifert04

MSeifert04 commented Mar 9, 2018 •

Copy link
Copy Markdown
Contributor

LGTM provided the tests pass.

Just a question: Why the euv variable in the script?

@pllim pllim added the zzz 💤 merge-when-ci-passes Do not use: We have auto-merge option now. label Mar 9, 2018
@drdavella

Copy link
Copy Markdown
Contributor Author

@MSeifert04 the script wasn't working for me at first, so I added those flags to help me debug it. I forgot to remove them before committing but it seems like there's no reason to remove them at this point:

http://tldp.org/LDP/abs/html/options.html

@mhvk

mhvk commented Mar 9, 2018

Copy link
Copy Markdown
Contributor

Happy to see you found my script (and sorry that it didn't work immediately...)

@mhvk mhvk 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.

Yikes, buffer overflows in one of the standard forms... Good to have the update. One comment is really just a question.

Comment thread cextern/trim_cfitsio.sh Outdated
rm -f cfitsio/docs/*.doc
rm -f cfitsio/docs/*.toc
rm -f cfitsio/[^L]*.*
rm -rf cfitsio/[^L]*.*

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.

I'm surprised by the need for this change - this means that in the new cfitsio library there is a directory with a . in its name. Out of curiosity, what is it?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I guess I should wait till this is answered before merging?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

For me it was cfitsio/cfitsio.xcodeproj, which appears to be generated when building (on OSX for me). Maybe we should just explicitly remove that file rather than using -rf here?

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.

So, that must be a new directory. I think I'd prefer removing it explicitly, but honestly I don't really know why...

@eteq

eteq commented Mar 9, 2018

Copy link
Copy Markdown
Member

Alright, all passed and approved, so merging now.

@eteq
eteq merged commit 17c785a into astropy:master Mar 9, 2018
@drdavella
drdavella deleted the update-cfitsio branch March 9, 2018 21:37
bsipocz pushed a commit that referenced this pull request Mar 12, 2018
Update cfitsio to v3.43: includes critical bug fix
@bsipocz bsipocz modified the milestones: v3.0.1, v2.0.5 Mar 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

external PRs and issues related to external packages vendored with Astropy (astropy.extern) 🔥 Critical io.fits Manual Backport zzz 💤 merge-when-ci-passes Do not use: We have auto-merge option now.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Security: please update cfitsio

6 participants