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: add version flag to all the shims #268

Merged
merged 1 commit into from
Aug 30, 2023

Conversation

Mossaka
Copy link
Member

@Mossaka Mossaka commented Aug 24, 2023

This follows the PR containerd/rust-extensions#167

@jprendes
Copy link
Collaborator

suggestion: use https://crates.io/crates/git-version and remove the build scripts.

@Mossaka
Copy link
Member Author

Mossaka commented Aug 24, 2023

https://crates.io/crates/git-version uses git describe, which fetches the tag information for a commit hash. It is a bit awkward to have the following revision

./target/debug/containerd-shim-wasmedge-v1:
  Version: 0.1.1
  Revision: containerd-shim-wasm/v0.1.2-278-ged85c04-modified

Signed-off-by: jiaxiao zhou <jiazho@microsoft.com>
@jprendes
Copy link
Collaborator

jprendes commented Aug 25, 2023

Could we use compile_time_run instead? With something like:

const fn get_rev() -> &'static str {
    compile_time_run::run_command_str!("sh", "-c", "echo -n $(git rev-parse --short=10 HEAD)$(git diff --quiet || echo .m)")
}

Comment on lines +15 to +26
pub fn parse_version() {
let os_args: Vec<_> = env::args_os().collect();
let flags = parse(&os_args[1..]).unwrap();
if flags.version {
println!("{}:", os_args[0].to_string_lossy());
println!(" Version: {}", env!("CARGO_PKG_VERSION"));
println!(" Revision: {}", env!("CARGO_GIT_HASH"));
println!();

std::process::exit(0);
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

suggestion: create a containerd_shim_wasm::run function that takes the version / revision, and handles this logic.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure

Copy link
Member Author

@Mossaka Mossaka Aug 29, 2023

Choose a reason for hiding this comment

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

I don't think there is a good way to handle this in containerd-shim-wasm crate, as each shim implementation still needs to read os args, parse flags and get the version / revision... Could not save much logic here. With that said, I will leave it as is.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I meant something like:

// somewhere in containerd-shim-wasm
pub fn run<Shim: ...>(runtime_id: &str, opts: Option<Config>, version: &str, revision: &str) {
    let os_args: Vec<_> = env::args_os().collect();
    let flags = parse(&os_args[1..]).unwrap();
    if flags.version {
        println!("{}:", os_args[0].to_string_lossy());
        println!("  Version: {}", env!("CARGO_PKG_VERSION"));
        println!("  Revision: {}", env!("CARGO_GIT_HASH"));
        println!();

        std::process::exit(0);
    }
    shim::run::<Shim>(runtime_id, opts)
}

and

// in main.rs
use containerd_shim_wasm::run;
fn main() {
    run::<ShimCli<WasiInstance>>(
        "io.containerd.wasmtime.v1",
        None,
        env!("CARGO_PKG_VERSION"),
        env!("CARGO_GIT_HASH"),
    );
}

then you can remove parse_version from all the lib.rs

@Mossaka
Copy link
Member Author

Mossaka commented Aug 29, 2023

Could we use compile_time_run instead? With something like:

I am a bit hesitate because it introduces an external library basically doing the same thing we want in build.rs, but less explictly.

@Mossaka Mossaka requested a review from jprendes August 29, 2023 19:58
@jprendes
Copy link
Collaborator

I am a bit hesitate because it introduces an external library basically doing the same thing we want in build.rs, but less explictly.

Haha, the opposite for me. I prefer the extra dep than adding a build.rs where it's not needed.
Also I think the extra dep is clearer and is less code to maintain (it has infrequent version bumps).
On the other hand, I'm not sure if that would work on Windows due to using sh / echo.
In any case, it's just a preference, and I'm happy for you to merge the build scripts.

Copy link
Collaborator

@jprendes jprendes left a comment

Choose a reason for hiding this comment

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

It's looking good. Some comments on ongoing discussions, but nothing major.

@Mossaka
Copy link
Member Author

Mossaka commented Aug 30, 2023

I will merge now! Thank you @jprendes

@Mossaka Mossaka merged commit 2653451 into containerd:main Aug 30, 2023
@Mossaka Mossaka deleted the add-version branch August 30, 2023 16:29
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.

2 participants