Skip to content
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 #418

Merged
merged 1 commit into from
Mar 23, 2024

Conversation

bcrochet
Copy link
Contributor

Simple build-time code generation for now. Plan to add git info from the environment if it exists.

Fixes #361

Simple build-time code generation for now. Plan to add git info from the
environment if it exists.

Fixes containers#361

Signed-off-by: Brad P. Crochet <brad@redhat.com>

fn main() {
let out_dir = env::var_os("OUT_DIR").unwrap();
let dest_path = Path::new(&out_dir).join("version.rs");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just so I understand better...is this build.rs effectively a workaround for the constant generated by the clap tooling in https://docs.rs/clap/latest/clap/macro.crate_version.html not having a docstring?

Or basically can you just add a comment with why we need this build.rs? Something like:

// Generate a source file with the crate version that includes a documentation string
// so that our #[deny(missing_docs)] works

or something like that?

(It seems like we could send a patch to clap to add a dummy docstring too?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doing this in order to then be able to pull in other information from the environment. In this way, we could have the version string reflect that it's a dev build, or do something different at release. It doesn't do that now. If you want me to go even simpler now and include the build.rs later when I get an actual use-case, I'm fine with that too.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a strong opinion, good with this as as is. Thanks for working on this!

@cgwalters cgwalters merged commit 60552ee into containers:main Mar 23, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add --version argument
2 participants