-
Notifications
You must be signed in to change notification settings - Fork 15
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
Simplify build.rs of fitsio-sys #377
Conversation
Hey thanks for this PR. I am glad someone wanted to tackle simplifying the build script. We have had some unrelated issues with the CI that are now preventing the CI from passing. Before I review this properly, would you mind rebasing onto |
While trying to add alternative Windows build mode, I couldn't help but notice that build.rs has a lot of duplicate code that could be better expressed through cfg branching. I extracted it as such, so now each build step is defined only in one place.
938430b
to
d4a1f8b
Compare
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 unable to robustly test this right now, but the code looks good 👍
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 think we have enough coverage in the tests where we use all combinations of the with-bindgen
and cfitsio-src
features to show that this implementation works. I also think the code restructuring is nice, leaving just the unique parts of each code path in the main
function (which is feature dependent) and moving the bindings generation into its own function.
You mention that you are working on a Windows build for this project. It would be great to increase our Windows support generally, as we don't have a lot of coverage in that area. Please keep us posted on your progress there, and feel free to reach out with any questions!
## 🤖 New release * `fitsio`: 0.21.6 -> 0.21.7 (✓ API compatible changes) * `fitsio-sys`: 0.5.4 -> 0.5.5 (✓ API compatible changes) <details><summary><i><b>Changelog</b></i></summary><p> ## `fitsio` <blockquote> ## [0.21.7](fitsio-v0.21.6...fitsio-v0.21.7) - 2025-01-02 ### Other - Include function to get cfitsio version ([#379](#379)) - Fix macos tests ([#380](#380)) </blockquote> ## `fitsio-sys` <blockquote> ## [0.5.5](fitsio-sys-v0.5.4...fitsio-sys-v0.5.5) - 2025-01-02 ### Other - Simplify build.rs of fitsio-sys ([#377](#377)) - Include function to get cfitsio version ([#379](#379)) - *(deps)* update bindgen requirement from 0.70 to 0.71 in /fitsio-sys in the cargo-packages group (#372) </blockquote> </p></details> --- This PR was generated with [release-plz](https://github.com/release-plz/release-plz/).
While trying to add alternative Windows build mode, I couldn't help but notice that build.rs has a lot of duplicate code that could be better expressed through cfg branching.
I extracted it as such, so now each build step is defined only in one place.