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
Update cfitsio to v3.43: includes critical bug fix #7274
Conversation
|
Hi there @drdavella Everything looks good from my point of view! If there are any issues with this message, please report them here. |
|
LGTM provided the tests pass. Just a question: Why the euv variable in the script? |
|
@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: |
|
Happy to see you found my script (and sorry that it didn't work immediately...) |
There was a problem hiding this comment.
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.
cextern/trim_cfitsio.sh
Outdated
| @@ -29,7 +31,7 @@ rm -f cfitsio/docs/*.ps | |||
| rm -f cfitsio/docs/*.pdf | |||
| rm -f cfitsio/docs/*.doc | |||
| rm -f cfitsio/docs/*.toc | |||
| rm -f cfitsio/[^L]*.* | |||
| rm -rf cfitsio/[^L]*.* | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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...
|
Alright, all passed and approved, so merging now. |
Update cfitsio to v3.43: includes critical bug fix
This fixes #7272. I'll open a separate PR for backporting to 2.0.x.