-
Notifications
You must be signed in to change notification settings - Fork 70
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
feat(build): Add extra version information #415
Conversation
Introduce shadow-rs as a dependency. This gives some additional git information for version info, and integrates with clap very nicely. Fixes containers#361 Signed-off-by: Brad P. Crochet <brad@redhat.com> rh-pre-commit.version: 2.2.0 rh-pre-commit.check-secrets: ENABLED
@@ -39,6 +40,7 @@ serde_ignored = "0.1.10" | |||
serde_json = "1.0.114" | |||
serde_yaml = "0.9.33" | |||
serde_with = ">= 3.7.0, < 4" | |||
shadow-rs = { version = "0.27", default-features = false, features = ["git2"] } |
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 tend to glance at e.g. https://crates.io/crates/shadow-rs/reverse_dependencies for stuff like this. And it seems popular, which hopefully implies it's well maintained.
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.
This all said...and sorry I should have mentioned this in the original ticket. The thing is that this is approach is unfortunately unlikely to be really useful to those people (like us) who end up shipping bootc as an RPM or dpkg or equivalent. At least Fedora derivatives don't build from upstream git, they build from a tarball - without any git information.
And of course, patches can be added there without changing the git hash...so it's really hard to get away from needing to run rpm -q foo
or equivalent as the real source of truth.
It seems a lot simpler to me to start with just say https://docs.rs/clap/latest/clap/macro.crate_version.html ?
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 okay with stepping back from it and doing something simpler. I was curious what your thoughts with this were.
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.
This type of stuff is always tricky...if you tell me "I really like what this shadow-rs is doing" - then OK!
One case I can certainly see here is having the git commit in --version
would help us debug cases when doing upstream development. And there are hopefully OSes/distributions out there which properly reflect the upstream commit and don't build from tarballs.
My instinct says we get 80% of the value (simply having a --version
argument) with the basic clap integration and properly bumping the version in Cargo.toml as you're doing below - which would be 5% of the change cost/maintenance. (Making up percentages of course 😄 )
But pull request review is in a way a form of negotiation...so again if based on this and our shared understanding of the tradeoffs you'd like to go with shadow-rs then I'm good with that too!
It is reassuring to see shadow-rs is at least used by nushell, which I use as my login shell.
@@ -47,6 +49,9 @@ toml = "0.8.12" | |||
xshell = { version = "0.2", optional = true } | |||
uuid = { version = "1.7.0", features = ["v4"] } | |||
|
|||
[build-dependencies] | |||
shadow-rs = { version = "0.27", default-features = false, features = ["git2"] } |
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 confused, why does it need to be both a runtime and build dependency? Isn't it just a build dependency? Is that for the clap integration? I guess this is an upstream question, as we're just following their recommendations here.
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.
Good question. I'm not completely sure, tbh
@@ -4,8 +4,7 @@ | |||
//! to provide a fully "container native" tool for using | |||
//! bootable container images. | |||
|
|||
// See https://doc.rust-lang.org/rustc/lints/listing/allowed-by-default.html | |||
#![deny(missing_docs)] | |||
#![warn(missing_docs)] |
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.
Why? What was failing? I like having this on by default...
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.
As-is, it generates a const CLAP_VERSION
that has no docstring and is marked as deprecated. I'm trying to figure out if it's possible to just not generate that since we don't need it.
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.
Hmm. Yes, or I think it would work to add an #[allow(missing_docs)]
just on that struct if we had to.
@@ -1,6 +1,6 @@ | |||
[package] | |||
name = "bootc" | |||
version = "0.1.0" | |||
version = "0.1.9" |
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.
Yeah we definitely need to do this and make it part of our release process. cargo xtask package
today runs off git tags, but I guess we should verify the two are in sync.
Closing in favor of #418 |
Introduce shadow-rs as a dependency. This gives some additional git information for version info, and integrates with clap very nicely.
Fixes #361
Signed-off-by: Brad P. Crochet brad@redhat.com
rh-pre-commit.version: 2.2.0
rh-pre-commit.check-secrets: ENABLED