Skip to content

Run cargo add in c2rust-instrument --set-runtime to add c2rust-analysis-rt automatically #562

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

Merged
merged 6 commits into from
Aug 16, 2022

Conversation

kkysen
Copy link
Contributor

@kkysen kkysen commented Jul 30, 2022

Currently, in #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. Note that cargo add is only run when --set-runtime is passed, so this is off by default, and must be opted-into.

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.

@rinon
Copy link
Contributor

rinon commented Aug 2, 2022

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.

Base automatically changed from kkysen/instrument-rustc-wrapper to master August 12, 2022 17:29
kkysen added 2 commits August 16, 2022 13:02
…is-rt` instead of the user having to manually do it. However, we have to use `+stable` as `cargo add` was added in 1.62 (on 1.60 now, but upgrading in #513).
@kkysen kkysen force-pushed the kkysen/instrument-cargo-add branch from e89e468 to 8d2cc30 Compare August 16, 2022 20:05
@kkysen
Copy link
Contributor Author

kkysen commented Aug 16, 2022

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 kkysen changed the title Run cargo add in c2rust-instrument to add c2rust-analysis-rt automatically Run cargo add in c2rust-instrument --set-runtime to add c2rust-analysis-rt automatically Aug 16, 2022
@kkysen kkysen merged commit 34904d7 into master Aug 16, 2022
@kkysen kkysen deleted the kkysen/instrument-cargo-add branch August 16, 2022 23:16
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