c2rust icon indicating copy to clipboard operation
c2rust copied to clipboard

Run `cargo add` in `c2rust-instrument` to add `c2rust-analysis-rt` automatically

Open kkysen opened this issue 2 years ago • 1 comments

Blocked on #595.

Currently, in https://github.com/immunant/c2rust/pull/554, the c2rust-analysis-rt runtime must be added as an (optional) dependency in the instrumented crate. This runs cargo add c2rust-analysis-rt --optional so that it is automatically added and up-to-date when instrumenting. This also adds an optional --runtime ${runtime} argument to c2rust-instrument. If it is given, then cargo add c2rust-analysis-rt --optional --path ${runtime} --offline is run. Thus, it works well both when c2rust-analysis-rt is already on your machine and you're developing locally, as well as a user who would download c2rust-analysis-rt from crates.io.

The reason this isn't currently added to https://github.com/immunant/c2rust/pull/554 directly is that cargo add was added as a native cargo command in 1.62 (it's also part of cargo-edit before that), but our nightly is currently pinned to 1.60. Thus, a hack to use cargo +stable is currently in place to avoid the nightly pinning. Once https://github.com/immunant/c2rust/pull/513 lands and upgrades our nightly, this should work seamlessly.

Another, more complex, option is to copy the Cargo.toml to a Cargo.instrument.toml, add the c2rust-analysis-rt dependency there, and the run with --manifest-path Cargo.instrument.toml. However, that's more complex, and not all cargo commands uniformly take a --manifest-path argument, and there's no environment variable for that, which would be seamlessly inherited.

kkysen avatar Jul 30 '22 04:07 kkysen

I don't think we should automatically run cargo add when instrumenting, that seems too magic to me. We should just document this requirement in a README. The most automatic thing I think we should possibly do is diagnose this error when instrumenting and present a friendly version.

rinon avatar Aug 02 '22 01:08 rinon

It's off by default now and enabled with --set-runtime. I still think this is good for testing especially, since it ensures the runtime crate is up-to-date and points to the correct version. This allows us to avoid keeping the runtime dependency in Cargo.toml checked into version control, too.

kkysen avatar Aug 16 '22 20:08 kkysen