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

analyze: add C2RUST_ANALYZE_NO_CARGO env var to disable cargo integration #1070

Merged
merged 2 commits into from
Apr 11, 2024

Conversation

spernsteiner
Copy link
Collaborator

Adding cargo integration made it a bit tricky to access the pre-cargo behavior, in which c2rust-analyze would behave like rustc. This behavior was useful for experimenting on small single-file example programs (e.g. c2rust-analyze foo.rs). This commit adds a new env var, C2RUST_ANALYZE_NO_CARGO, which can be set to restore the old behavior.

c2rust-analyze/src/main.rs Outdated Show resolved Hide resolved
c2rust-analyze/src/main.rs Show resolved Hide resolved
Copy link
Contributor

@kkysen kkysen left a comment

Choose a reason for hiding this comment

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

Alternatively, what about achieving this similar to how cargo rustc works? That is, c2rust-analyze rustc ... runs as rustc directly, and we check this by seeing if argv1 is rustc. Then when we invoke rustc_wrapper, the logic for at_args doesn't need to change, since argv1 is rustc, just like when it was invoked by the cargo wrapper. And for is_primary_compilation, we just do the env::var_os(RUSTC_WRAPPER_VAR).as_deref() == Some(own_exe.as_os_str()) check again (if it's not equal, is_primary_compilation should be true).

c2rust-analyze/src/main.rs Show resolved Hide resolved
Comment on lines 279 to 289
let no_cargo = env::var_os(NO_CARGO_VAR).is_some();
let mut at_args = if no_cargo {
env::args().collect::<Vec<_>>()
} else {
env::args().skip(1).collect::<Vec<_>>()
};
Copy link
Contributor

Choose a reason for hiding this comment

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

With the cargo wrapper, the first arg is c2rust-analyze and the second arg is rustc. If we're running as a rustc wrapper, rustc is not going to the first arg, so we need to set it.

Copy link
Contributor

@fw-immunant fw-immunant left a comment

Choose a reason for hiding this comment

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

I kind of wish this made rustc_wrapper accept at_args and is_primary_package (or a clearer name for controlling which callbacks set is passed) as arguments instead of adding branches to its body.

Correctness looks fine, but I wish this were documented externally. I don't know if I fully understand how this changes the interface of the program--is it just making it easier to access the behavior that would be provided by CARGO_PRIMARY_PACKAGE= RUSTC_WRAPPER=/path/to/c2rust-analyze /path/to/c2rust-analyze foo.rs?

@spernsteiner
Copy link
Collaborator Author

what about achieving this similar to how cargo rustc works?

cargo rustc still requires a Cargo.toml, and chooses which file to compile based on the Cargo targets. This mode is meant to work like plain rustc, which can compile small examples via rustc foo.rs without needing any supporting Cargo infrastructure. This is useful for running our filecheck test cases and similar kinds of examples.

@spernsteiner
Copy link
Collaborator Author

That is, c2rust-analyze rustc ... runs as rustc directly, and we check this by seeing if argv1 is rustc.

Ah, I misunderstood what you meant.

I think it would be confusing for c2rust-analyze <subcmd> to work like cargo <subcmd> except in the specific case where subcmd == "rustc". If we were to define our own set of subcommands independent of cargo's, then it might make sense to define c2rust-analyze rustc this way, but that's a more significant refactor than I wanted to do for this branch (the idea here is just to bring back the old rustc-like behavior in the simplest way possible).

@spernsteiner spernsteiner merged commit 982bafb into master Apr 11, 2024
9 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.

4 participants