Allow cfitsio to be compiled from source.

[cjordan]

Hey @mindriot101, hope you're doing well. This commit looks rather scary, but that's just because it includes all the cfitsio source code. Feel free to review this if you like and have the time.

One potential point for apprehension here is that the code is not tested under OSX. At my work, we recently got a Mac-using developer, so they can test my commit before it gets merged, should you not want to or have time.

Cheers!

[simonrw]

In general, I think this is a good idea, up to a point. I agree that providing the ability to compile cfitsio from source is a great idea 👍

I have reservations about vendoring the entire cfitsio codebase into this source. It ties us to a specific version of cfitsio until we update, and removes the ability to choose which version from the user. It does however remove a concern that I have in that the original bindings were generated for macos, and are luckily applicable to linux. I would feel much better allowing the user to compile against the actual version they have, which 1. I added with the bindgen feature, and 2. this PR has. Can you find out what other crates are doing when they bundle the source code? Do they download it as part of the build, or do they include it as a submodule/subtree or similar?

I am happy to test on macos when I get the chance, as I'd like it to compile on as many platforms as possible. It would be really good to have CI checking that, but my time is fairly limited. Similarly, I think it would be nice to include a test that compiles with this new feature. Can you add something to bin/test, where the CI tests are run from?

I've mentioned in the diff comments, but I'm not keen on the feature name static. I think something like fitsio-source/fitsio-src, or something?

Thanks for the PR, I really appreciate your contributions. I'm really glad the crate is being used 😂

[cjordan]

Thanks for getting back to me so quickly! I'll be happy to change things to your satisfaction, but just to respond to your comments/questions.

I have reservations about vendoring the entire cfitsio codebase into this source. It ties us to a specific version of cfitsio until we update, and removes the ability to choose which version from the user. It does however remove a concern that I have in that the original bindings were generated for macos, and are luckily applicable to linux. I would feel much better allowing the user to compile against the actual version they have, which 1. I added with the bindgen feature, and 2. this PR has. Can you find out what other crates are doing when they bundle the source code? Do they download it as part of the build, or do they include it as a submodule/subtree or similar?

My inspiration comes from hdf5-rust (and I've done a similar thing for ERFA). HDF5 has the luxury of having its C source code in a git repo, so yes, the Rust crate uses a submodule. It seems that cfitsio has no development area; only the main NASA website. A quick search did find a group with a cfitsio git page, however, it doesn't appear to be actively maintained, although that might be fixed with a PR over there. I don't like my current solution's copying the source code, so if you agree, I can also use a git submodule for the aforementioned page. Otherwise, something else we could do is just have the cfitsio .tar.gz sitting in this repo, which gets untar-ed when needed.

I am happy to test on macos when I get the chance, as I'd like it to compile on as many platforms as possible. It would be really good to have CI checking that, but my time is fairly limited. Similarly, I think it would be nice to include a test that compiles with this new feature. Can you add something to bin/test, where the CI tests are run from?

Ah, yes CI is a great idea (and I'm a little embarrassed I didn't think of it). Will do, and I'll try to get OSX tested as well.

I've mentioned in the diff comments, but I'm not keen on the feature name static. I think something like fitsio-source/fitsio-src, or something?

Sure, easy.

I have one question for you too (and this is actually what triggered my work here): How did you get docs.rs to build rust-fitsio? (i.e. when you push rust-fitsio to crates.io, docs.rs automatically kicks in and builds the documentation). AFAICT, docs.rs would need a pre-built cfitsio C library available when compiling rust-fitsio before it can generate the docs. I can't see anything in this git repo that gets around that issue, but I suppose I'm not understanding something.

Also, as I'll be making commits in this branch, I'd like to squash them all into one when we're done, so you can leave the merging to me when I have your approval.

[simonrw]

My inspiration comes from hdf5-rust (and I've done a similar thing for ERFA). HDF5 has the luxury of having its C source code in a git repo, so yes, the Rust crate uses a submodule. It seems that cfitsio has no development area; only the main NASA website. A quick search did find a group with a cfitsio git page, however, it doesn't appear to be actively maintained, although that might be fixed with a PR over there. I don't like my current solution's copying the source code, so if you agree, I can also use a git submodule for the aforementioned page. Otherwise, something else we could do is just have the cfitsio .tar.gz sitting in this repo, which gets untar-ed when needed.

I think you're right and you raise good points about vendoring the source code. I had forgotten that cfitsio is not "really" distributed as a git repository, but as a tarball. As i said in my initial reply, I do quite like the fact that we can pin to a specific version of cfitsio and have to be concerned about a version mismatch. Let's do as you've done and commit the source code. At least we can see what changes when we upgrade. I would prefer a git submodule, but in the absence of an official git repository, I think vendoring the contents of the tarball would be simplest. Incidentally, over the last 12 years of using the library, I've never encountered a backwards incompatible change, or the need to upgrade to a later version. It has been pretty stable for me, so pinning to whatever is the latest now seems like a good plan 👍

Ah, yes CI is a great idea (and I'm a little embarrassed I didn't think of it). Will do, and I'll try to get OSX tested as well.

No worries, I think getting the CI to compile with the feature on linux should be simple, but automated testing on macos may be more tricky. If your mac-using developer can test and I can test, perhaps we can look into it at some point.

I have one question for you too (and this is actually what triggered my work here): How did you get docs.rs to build rust-fitsio? (i.e. when you push rust-fitsio to crates.io, docs.rs automatically kicks in and builds the documentation). AFAICT, docs.rs would need a pre-built cfitsio C library available when compiling rust-fitsio before it can generate the docs. I can't see anything in this git repo that gets around that issue, but I suppose I'm not understanding something.

I can't find it now, but I do remember submitting a PR to docs.rs that included the following packages to their build environment:

• libcfitsio-dev
• libcfitsio-doc
• libcfitsio8

I guess the repo was migrated at some point in the past and my PR was lost, but the current practice is to add your dependencies to this file: https://github.com/rust-lang/crates-build-env/blob/master/packages.txt. It looks like they've upped the amount of developer docs as well so the process should be quite straightforward if you have dependencies you want installed.

Also, as I'll be making commits in this branch, I'd like to squash them all into one when we're done, so you can leave the merging to me when I have your approval.

If you don't mind, I'd prefer you to not squash the commits. Feel free to tidy the commits up (e.g. interactive rebase then force push) but I quite like seeing a bit of history in git branches.

Thanks again for this contribution. As I said before, it's nice to have a user (that I know of 😂 )

[cjordan]

I think you're right and you raise good points about vendoring the source code. I had forgotten that cfitsio is not "really" distributed as a git repository, but as a tarball. As i said in my initial reply, I do quite like the fact that we can pin to a specific version of cfitsio and have to be concerned about a version mismatch. Let's do as you've done and commit the source code. At least we can see what changes when we upgrade. I would prefer a git submodule, but in the absence of an official git repository, I think vendoring the contents of the tarball would be simplest. Incidentally, over the last 12 years of using the library, I've never encountered a backwards incompatible change, or the need to upgrade to a later version. It has been pretty stable for me, so pinning to whatever is the latest now seems like a good plan +1

Great, easy.

I can't find it now, but I do remember submitting a PR to docs.rs that included the following packages to their build environment:

* libcfitsio-dev

* libcfitsio-doc

* libcfitsio8

I guess the repo was migrated at some point in the past and my PR was lost, but the current practice is to add your dependencies to this file: https://github.com/rust-lang/crates-build-env/blob/master/packages.txt. It looks like they've upped the amount of developer docs as well so the process should be quite straightforward if you have dependencies you want installed.

Ah, thanks for clearing that up! I feel like I had come across this before, but obviously forgot when it became important. I see libhdf5 is up there -- that simplifies another one of my projects...

If you don't mind, I'd prefer you to not squash the commits. Feel free to tidy the commits up (e.g. interactive rebase then force push) but I quite like seeing a bit of history in git branches.

Sure, it's not a big deal; my OCD will run rampant when unrestricted, but I'll just try to keep successive commits tidy.

Thanks again for this contribution. As I said before, it's nice to have a user (that I know of joy )

No worries! As I alluded to last year, soon this library will be seeing a lot of indirect use! A few of us are hoping that this helps in our "Rust crucade" (which is only pragmatic, of course)...

[cjordan]

Howdy! I believe I've addressed all of the points we discussed some weeks ago here. Please let me know what you think. If you feel that it's ready, it would be nice to make a new crates.io release. I would be happy to do it too, although I'll need permission over on crates.io.

I haven't yet poked our resident MacOS-using dev to test the new feature, but I'll go on record and claim that I think it'll be fine. Without knowing much about Macs, surely having a C compiler, configure and make isn't a big ask?

Also, I have recently done work with GitHub actions automated testing on Linux and MacOS. As of last week, it worked well, but now there's an obscure compile-time error in the MacOS runner... hopefully that's just a transient thing on GitHub's side. Anyway, I'm bringing it up in case that's of interest. It would not be difficult to copy my yaml files and just run cargo test in all of the sub-crates.

[simonrw]

Hi, thanks for posting the update. I will take a look ASAP.

If you feel that it's ready, it would be nice to make a new crates.io release. I would be happy to do it too, although I'll need permission over on crates.io.

I'm happy for this, I can investigate what I need to do to let you publish.

I haven't yet poked our resident MacOS-using dev to test the new feature, but I'll go on record and claim that I think it'll be fine. Without knowing much about Macs, surely having a C compiler, configure and make isn't a big ask?

It will probably be fine, you're right. I will try to test it locally when I can however my personal mac finally gave up recently. I have a work mac and I can try with that.

Also, I have recently done work with GitHub actions automated testing on Linux and MacOS. As of last week, it worked well, but now there's an obscure compile-time error in the MacOS runner... hopefully that's just a transient thing on GitHub's side. Anyway, I'm bringing it up in case that's of interest. It would not be difficult to copy my yaml files and just run cargo test in all of the sub-crates.

You have this in your fork? I would be interested in this. I have just found out that my Travis credits have expired for this month (I have no idea how) and I'd like to migrate the CI over to actions. I have a branch github-actions where I've started, but if you have already made progress then I'd be really interested to see. I'd like CI to be running again before I merge this PR if that's ok, just so we can keep an eye on the new build process. Incidentally I have built it once locally and it passed all of the tests so I'm sure it's going to work, but I'd like some automation to ensure this going forward. 👍

Again I'm really glad that you're helping me with this project, it's great to have a users' experience and I cannot provide that any more unfortunately 😢

[simonrw]

turns out it's super easy to add additional owners of crates so I've added you as an additional owner of fitsio. I can add you to the other crates if you need as well.

[simonrw]

I've just merged the github actions branch into master, so I'll rebase this PR and check the CI build works

[simonrw]

Cool to answer the macos question, I've added macos as a test target for the CI, and now passes. I think I'm happy about this PR apart from the outstanding question about making with -j4.

[cjordan]

Thanks for jumping on this PR again so eagerly. I've commented on the make threads issue above.

And the GitHub actions CI you've done looks good! Mine (associated with another crate) is more or less the same.

[simonrw]

Could it be worth adding -march=native instead of these options? That might prevent cross compiling but I don't think that's an immediate use case. It can be done by removing these --enable flags, and including -march=native in the CFLAGS environment variable on line 124.

[cjordan]

I was going to speak out against putting -march=native here, but now I've changed my mind. See, I was intending this feature that this whole PR revolves around to provide various Rust-compiled libraries and executables via something like GitHub actions, but, it makes more sense to have GitHub actions to have the portable builds, and any downstream users get full performance out of cfitsio by using this feature. So yes, good catch! I'll push these last two changes to build.rs.

[simonrw]

And the GitHub actions CI you've done looks good! Mine (associated with another crate) is more or less the same.

Not a coincidence! I used your hd5 library github actions as inspiration.

[simonrw]

This looks really good, thanks for providing this. I will merge now 🎉👍🤘🚀

[cjordan]

Thanks for your help!