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.

Sign up for GitHub

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

Jump to bottom

Fix abort when open FITS with cfitsio 4.2.0 #1232

Merged
merged 2 commits into from
Jan 12, 2023
Merged

Fix abort when open FITS with cfitsio 4.2.0 #1232

merged 2 commits into from
Jan 12, 2023

Conversation

pford
Copy link
Collaborator

@pford pford commented Jan 11, 2023 •
edited by kswang1029
Loading

Closes #1231

  • What is implemented or fixed? Mention the linked issue(s), if any.
    Fixes Backend aborts with cfitsio 4.2.0 #1231
  • How does this PR solve the issue? Give a brief summary.
    Uses cfitsio constants for char array size used for input to fits_read_key, or a char* nullptr when the return value is not needed. In 4.2.0, cfitsio uses strncpy with n set to length constant in cfitsio instead of length of char array.
  • Are there any companion PRs (frontend, protobuf)?
    No
  • Is there anything else that testers should know (e.g. exactly how to reproduce the issue)?
    Build carta-backend with cfitsio 4.2.0 and open FITS file (file list info or open image).

Checklist

  • changelog updated / no changelog update needed
  • e2e test passing / added corresponding fix
  • protobuf updated to the latest dev commit / no protobuf update needed
  • added reviewers and assignee
  • added ZenHub estimate, milestone, and release

@pford pford added this to the v4.0-b1 milestone Jan 11, 2023
@github-actions
Copy link

Code Coverage

Package Line Rate Health
src.Cache 68% ➖
src.DataStream 69% ➖
src.FileList 67% ➖
src.Frame 47% ➖
src.HttpServer 44% ➖
src.ImageData 28% ❌
src.ImageFitter 92% ✔
src.ImageGenerators 52% ➖
src.ImageStats 74% ➖
src.Logger 44% ➖
src.Main 54% ➖
src.Region 22% ❌
src.Session 30% ❌
src.Table 52% ➖
src.ThreadingManager 87% ✔
src.Timer 85% ✔
src.Util 50% ➖
Summary 40% (6754 / 16858) ➖

kswang1029
kswang1029 approved these changes Jan 12, 2023
View reviewed changes
Copy link
Contributor

@kswang1029 kswang1029 left a comment

Choose a reason for hiding this comment

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

no regression is observed from e2e tests. With some manual tests with some FITS images, I do not see odd things.

ajm-ska
ajm-ska approved these changes Jan 12, 2023
View reviewed changes
Copy link
Collaborator

@ajm-ska ajm-ska left a comment

Choose a reason for hiding this comment

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

Tested with cfitsio 4.1.0 and 4.2.0. It continues to work with 4.1.0, and has fixed the crash issue with 4.2.0.

@confluence confluence merged commit a6da548 into dev Jan 12, 2023
@confluence confluence deleted the pam/1231_cfitsio_4.2.0 branch January 12, 2023 11:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@confluence confluence confluence approved these changes

@jolopezl jolopezl jolopezl approved these changes

@ajm-ska ajm-ska ajm-ska approved these changes

@kswang1029 kswang1029 kswang1029 approved these changes

@crocka crocka crocka approved these changes

Assignees

@confluence confluence

Labels
None yet
Projects
None yet
Milestone
v4.0-b1
Development

Successfully merging this pull request may close these issues.

Backend aborts with cfitsio 4.2.0
6 participants
@pford @confluence @jolopezl @ajm-ska @kswang1029 @crocka