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

"--json-output" is ignored by tool builds #125666

Closed
RalfJung opened this issue May 28, 2024 · 3 comments · Fixed by #125781
Closed

"--json-output" is ignored by tool builds #125666

RalfJung opened this issue May 28, 2024 · 3 comments · Fixed by #125781
Labels
T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)

Comments

@RalfJung
Copy link
Member

When doing something like ./x.py build miri --json-output, then the errors/warnings in Miri itself are still reported in the normal, human-readable form, and not as JSON.

I assume something somewhere around here has to use a different function for running cargo, e.g. this one.

Cc @onur-ozkan

@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label May 28, 2024
@onur-ozkan onur-ozkan added T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels May 28, 2024
@onur-ozkan
Copy link
Member

For tools, we handle the build results later even if they fail:

let build_success = builder.run_cmd(BootstrapCommand::from(&mut cargo).allow_failure());
builder.save_toolstate(
tool,
if build_success { ToolState::TestFail } else { ToolState::BuildFail },
);
if !build_success {
crate::exit!(1);
} else {
// HACK(#82501): on Windows, the tools directory gets added to PATH when running tests, and
// compiletest confuses HTML tidy with the in-tree tidy. Name the in-tree tidy something
// different so the problem doesn't come up.
if tool == "tidy" {
tool = "rust-tidy";
}
let cargo_out = builder.cargo_out(compiler, self.mode, target).join(exe(tool, target));
let bin = builder.tools_dir(compiler).join(exe(tool, target));
builder.copy_link(&cargo_out, &bin);
bin
}

compile::run_cargo doesn't provide that interface, but we can modify it to do so (since it uses compile::stream_cargo under the hood, which returns the command result without panicking).

@RalfJung
Copy link
Member Author

All this toolstate stuff is not needed any more, though. We only use toolstate for books these days, so the tools can be cleaned up.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jun 7, 2024
…=albertlarsan68

prefer `compile::stream_cargo` for building tools

Previously, we were running bare commands for `ToolBuild` step and were unable to utilize some of the flags which  are already handled by `compile::stream_cargo`.

This change makes `ToolBuild` to use `compile::stream_cargo`, allowing us to benefit from the flags supported by the bootstrap cargo.

Resolves rust-lang#125666
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jun 7, 2024
…=albertlarsan68

prefer `compile::stream_cargo` for building tools

Previously, we were running bare commands for `ToolBuild` step and were unable to utilize some of the flags which  are already handled by `compile::stream_cargo`.

This change makes `ToolBuild` to use `compile::stream_cargo`, allowing us to benefit from the flags supported by the bootstrap cargo.

Resolves rust-lang#125666
@bors bors closed this as completed in 65fcba6 Jun 7, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Jun 7, 2024
Rollup merge of rust-lang#125781 - onur-ozkan:improve-tool-builder, r=albertlarsan68

prefer `compile::stream_cargo` for building tools

Previously, we were running bare commands for `ToolBuild` step and were unable to utilize some of the flags which  are already handled by `compile::stream_cargo`.

This change makes `ToolBuild` to use `compile::stream_cargo`, allowing us to benefit from the flags supported by the bootstrap cargo.

Resolves rust-lang#125666
@RalfJung
Copy link
Member Author

RalfJung commented Jun 8, 2024

Thanks for the fix :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants