-
Notifications
You must be signed in to change notification settings - Fork 10
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
Add support for custom cargo runner #43
Conversation
This is the recommended best practice for binaries, we should do it for `cargo-3ds` as well. A side effect of this is that we should test build and installation with the exact Cargo.lock dependencies with `--locked`.
Use `cargo config get` to check if the runner is configured, and if so use it like a normal `cargo run` invocation would. Otherwise, fall back to the default `3dslink` behavior we had before.
Minor tweaks like fn names, refactoring a little etc. Also update README to include an install command, with `--locked` as well.
To install the latest release on <https://crates.io>: | ||
|
||
```sh | ||
cargo install --locked cargo-3ds |
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 don't know if we want users to use --locked?
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 this is up for debate. Personally, I think it makes sense to keep a consistent set of dependencies (both in CI and at install time for users). I suppose one alternative would be for us to have a CI pipeline that tests installation without --locked
, just to look for breakages, but that's not a perfect solution IMO.
Installing without using the lockfile always runs the risk of some upstream dependency accidentally publishing a breaking change and cargo install
taking that new (broken) version, preventing install or resulting in a subtly broken binary. There's lots of discussion on rust-lang/cargo#7169 about this exact issue, and the takeaway I got after reading that thread was that most projects should either:
- Recommend users
cargo install --locked
- Run a periodic (nightly, etc.) build + test suite to check for upstream dependencies breaking something
To me, the first option seems like the easier of the two. But if others feel we should try for something like the second option, I can back out some of these changes and work on a follow-up PR to do that. Arguably I should have made the Cargo.lock stuff a separate PR anyway, but it seemed small enough to sneak in here at first glance.
Use
cargo config get
to check if the runner is configured, and if so use it like a normalcargo run
invocation would. Otherwise, fall back to the default3dslink
behavior we had before.Also check in
Cargo.lock
since this is best practice for binaries and probably will help keep CI more stable too.