-
Notifications
You must be signed in to change notification settings - Fork 887
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
compile_rustfmt
rewrite
#6275
compile_rustfmt
rewrite
#6275
Conversation
I've been thinking about what the best return type is for pub struct RustfmtRunner {
ld_library_path: String,
binary_path: PathBuf,
}
impl RustfmtRunner {
fn run(&self, args: &[&str]) -> std::io::Result<std::process::Output> {
Command::new(&self.binary_path)
.env("LD_LIBRARY_PATH", &self.ld_library_path)
.args(args)
.output()
}
}
pub fn build_rustfmt_from_src(output_path: PathBuf) -> Result<RustfmtRunner, CheckDiffError> {
let ld_library_path = get_ld_lib_path()?;
Command::new("cargo")
.args(["build", "-q", "--release", "--bin", "rustfmt"])
.output()?;
std::fs::copy("target/release/rustfmt", &output_path)?;
Ok(RustfmtRunner {
ld_library_path,
binary_path: output_path,
})
} |
@ytmimi i think the approach on I was wondering if the error for Also, instead of returning |
Personally, I don't think we need to get extremely specific with the errors. At the end of the day we're calling If we ever need to convert the |
7e6d0d4
to
4c0d43e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your patience on this one. I've left several inline comments with suggestions for how we can improve this further.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for all your hard work on this one. I'm requesting a few changes here, and then I think we'll be good to go. One question I had is whether or not you plan on adding any unit tests? I don't think it's 100% necessary, but I wanted to ask if that was your plan.
If you make my suggested changes and add the following line to fn main()
you can run the program with CHECK_DIFF_LOG=info cargo run --bin check_diff -- https://github.com/ytmimi/rustfmt.git subtree_push_automation
and see the following output:
fn main() changes
fn main() {
tracing_subscriber::fmt()
.with_env_filter(tracing_subscriber::EnvFilter::from_env("CHECK_DIFF_LOG"))
.init();
// rest of the main function
}
Log Output
2024-09-26T02:00:07.108055Z INFO check_diff: Created tmp_dir TempDir { path: "/Users/ytmimi/Documents/Projects/rust_projects/rustfmt/check_diff/.tmpGoVKYx" }
2024-09-26T02:00:08.774163Z INFO check_diff: Successfully cloned repository https://github.com/rust-lang/rustfmt.git
2024-09-26T02:00:08.774235Z INFO check_diff: Current directory: /Users/ytmimi/Documents/Projects/rust_projects/rustfmt/check_diff/.tmpGoVKYx
2024-09-26T02:00:08.778775Z INFO check_diff: Successfully added remote: https://github.com/ytmimi/rustfmt.git
2024-09-26T02:00:10.284677Z INFO check_diff: Successfully fetched subtree_push_automation.
2024-09-26T02:00:10.312499Z INFO check_diff: Compiling with cargo 1.83.0-nightly (c1fa840a8 2024-08-29)
2024-09-26T02:00:10.345363Z INFO check_diff: Building rustfmt from source
2024-09-26T02:00:29.191443Z INFO check_diff: Successfully switched to subtree_push_automation
2024-09-26T02:00:29.223861Z INFO check_diff: Building rustfmt from source
2024-09-26T02:00:37.776857Z INFO check_diff: RUSFMT_BIN rustfmt 1.8.0-nightly (6157568e1c 2024-09-20)
2024-09-26T02:00:37.776891Z INFO check_diff: Runtime dependencies for (src) rustfmt -- LD_LIBRARY_PATH: /Users/ytmimi/.rustup/toolchains/nightly-2024-09-10-aarch64-apple-darwin/lib
2024-09-26T02:00:38.457874Z INFO check_diff: FEATURE_BIN rustfmt 1.7.1-nightly (87d9ff0652 2024-09-19)
2024-09-26T02:00:38.457961Z INFO check_diff: Runtime dependencies for (feature) rustfmt -- LD_LIBRARY_PATH: /Users/ytmimi/.rustup/toolchains/nightly-2024-09-10-aarch64-apple-darwin/lib
61aeaa1
to
4c1328c
Compare
No description provided.