search

jacob-pro / cspice-rs

Rust bindings to CSPICE
GNU Lesser General Public License v3.0
7 stars 3 forks source link

Automatically download CSPICE on build if it is not located #7

Closed mclrc closed 1 year ago

mclrc commented 1 year ago

Hello Jacob! Thank you for your work on this library! This PR modifies the cspice-sys build script to, when CSPICE_DIR isn't set, search the system library path and then, if that's unsuccessful, download CSPICE directly from NAIF servers. This does increase build time on the first build and also adds a build dependency on reqwest, however, I think the ergonomic improvement is more than worth it. If you do not consider this behavior desirable by default, it could easily be locked behind a feature flag.

I tested this on Arch Linux and a fresh install of Windows 10 with only Git Bash + Unix tools, LLVM and Rust installed. I think this should work on M1 and Intel Macs as well, I took care to handle them in the code, but unfortunately I can't test that as I do not own a Mac. Perhaps you could help with that?

The code is written to easily have new platforms/archive file types added, there are a couple more on the NAIF page, but I think the major ones are covered here. Of course, there may be issues I didn't run into with my limited testing.

Feel free to request any changes you deem appropriate, I have some free time at the moment.

Cheers, have a nice day! :)

mclrc commented 1 year ago

I just learned that syscrates aren't really supposed to download anything... oh well :/

If you're interested despite this, I'd probably implement the feature-flag idea. Actually I'll probably do it no matter what just to learn something. Feel free to close this otherwise, it's my fault for not reading up on this in advance.

Cheers!

jacob-pro commented 1 year ago

Hi @pixldemon ,

Thanks for this PR - yep that is basically what I was going to say haha

The general best practice is that crates should be self contained, build without the internet, and only write build artifacts to the build directory.

For example docs.rs doesn't allow any access to the internet when building the crate - so for that reason I bundled the Linux headers in the repo just for docs.rs.

I'm not against having this option though as long as it is behind an optional feature flag - since it would definitely make it easier for beginners to use

If you add this it would also be nice to include changes to the github workflows so that we can test it is working

mclrc commented 1 year ago

Okay I think I got it working now, with the separate job. I apologize for the now pretty messy PR, I have never used Github Actions before. If you don't mind, what do you use to test them locally? act with the complete 60GB image? or is there a better way?

jacob-pro commented 1 year ago

If you don't mind, what do you use to test them locally? act with the complete 60GB image? or is there a better way?

I've never personally been able to test it locally, I usually end up just doing trial-and-error with a lot of new commits 😆

I think Github Actions is quite basic compared to others like CircleCI which let you SSH in and debug what went wrong - but the Github one is completely free and easy to setup so that's why I use it...

The PR looks great, just wondering would you be able to update the README with updated instructions of how it works now?

https://github.com/jacob-pro/cspice-rs/blob/master/cspice-sys/README.md

mclrc commented 1 year ago

Alright hahaha good to know! :) Yeah, CI solutions have always seemed pretty complex to me, it's a good thing I finally had to use one - it was indeed simpler than I thought, may use it myself from now on. Just updated the README, feel free to tweak it further.