From 2c89a5104c7d6b997e3368e1bb991a48152fa3ca Mon Sep 17 00:00:00 2001 From: Patrick Walton Date: Mon, 31 Oct 2022 20:38:40 -0700 Subject: [PATCH 01/14] rustc_target: Add alignment to indirectly-passed by-value types, correcting the alignment of `byval` on x86 in the process. Commit 88e4d2c2918428d55e34cd57c11279ea839c8822 from five years ago removed support for alignment on indirectly-passed arguments because of problems with the `i686-pc-windows-msvc` target. Unfortunately, the `memcpy` optimizations I recently added to LLVM 16 depend on this to forward `memcpy`s. This commit attempts to fix the problems with `byval` parameters on that target and now correctly adds the `align` attribute. The problem is summarized in [this comment] by @eddyb. Briefly, 32-bit x86 has special alignment rules for `byval` parameters: for the most part, their alignment is forced to 4. This is not well-documented anywhere but in the Clang source. I looked at the logic in Clang `TargetInfo.cpp` and tried to replicate it here. The relevant methods in that file are `X86_32ABIInfo::getIndirectResult()` and `X86_32ABIInfo::getTypeStackAlignInBytes()`. The `align` parameter attribute for `byval` parameters in LLVM must match the platform ABI, or miscompilations will occur. Note that this doesn't use the approach suggested by eddyb, because I felt it was overkill to store the alignment in `on_stack` when special handling is really only needed for 32-bit x86. As a side effect, this should fix #80127, because it will make the `align` parameter attribute for `byval` parameters match the platform ABI on LLVM x86-64. [this comment]: https://github.com/rust-lang/rust/pull/80822#issuecomment-829985417 --- compiler/rustc_target/src/abi/call/m68k.rs | 2 +- compiler/rustc_target/src/abi/call/mod.rs | 19 ++++-- compiler/rustc_target/src/abi/call/wasm.rs | 2 +- compiler/rustc_target/src/abi/call/x86.rs | 35 +++++++++-- compiler/rustc_target/src/abi/call/x86_64.rs | 2 +- src/test/codegen/align-byval.rs | 56 +++++++++++++++++ src/test/codegen/function-arguments-noopt.rs | 4 +- src/test/codegen/function-arguments.rs | 4 +- .../extern-fn-explicit-align/Makefile | 5 ++ .../extern-fn-explicit-align/test.c | 35 +++++++++++ .../extern-fn-explicit-align/test.rs | 61 +++++++++++++++++++ 11 files changed, 208 insertions(+), 17 deletions(-) create mode 100644 src/test/codegen/align-byval.rs create mode 100644 src/test/run-make-fulldeps/extern-fn-explicit-align/Makefile create mode 100644 src/test/run-make-fulldeps/extern-fn-explicit-align/test.c create mode 100644 src/test/run-make-fulldeps/extern-fn-explicit-align/test.rs diff --git a/compiler/rustc_target/src/abi/call/m68k.rs b/compiler/rustc_target/src/abi/call/m68k.rs index c1e0f54af5f1e..1d4649ed8678e 100644 --- a/compiler/rustc_target/src/abi/call/m68k.rs +++ b/compiler/rustc_target/src/abi/call/m68k.rs @@ -10,7 +10,7 @@ fn classify_ret(ret: &mut ArgAbi<'_, Ty>) { fn classify_arg(arg: &mut ArgAbi<'_, Ty>) { if arg.layout.is_aggregate() { - arg.make_indirect_byval(); + arg.make_indirect_byval(None); } else { arg.extend_integer_width_to(32); } diff --git a/compiler/rustc_target/src/abi/call/mod.rs b/compiler/rustc_target/src/abi/call/mod.rs index c5a6f9893b6f9..e1893d96ac632 100644 --- a/compiler/rustc_target/src/abi/call/mod.rs +++ b/compiler/rustc_target/src/abi/call/mod.rs @@ -499,9 +499,7 @@ impl<'a, Ty> ArgAbi<'a, Ty> { .set(ArgAttribute::NonNull) .set(ArgAttribute::NoUndef); attrs.pointee_size = layout.size; - // FIXME(eddyb) We should be doing this, but at least on - // i686-pc-windows-msvc, it results in wrong stack offsets. - // attrs.pointee_align = Some(layout.align.abi); + attrs.pointee_align = Some(layout.align.abi); let extra_attrs = layout.is_unsized().then_some(ArgAttributes::new()); @@ -518,11 +516,19 @@ impl<'a, Ty> ArgAbi<'a, Ty> { self.mode = Self::indirect_pass_mode(&self.layout); } - pub fn make_indirect_byval(&mut self) { + pub fn make_indirect_byval(&mut self, byval_align: Option) { self.make_indirect(); match self.mode { - PassMode::Indirect { attrs: _, extra_attrs: _, ref mut on_stack } => { + PassMode::Indirect { ref mut attrs, extra_attrs: _, ref mut on_stack } => { *on_stack = true; + + // Some platforms, like 32-bit x86, change the alignment of the type when passing + // `byval`. Account for that. + if let Some(byval_align) = byval_align { + // On all targets with byval align this is currently true, so let's assert it. + debug_assert!(byval_align >= Align::from_bytes(4).unwrap()); + attrs.pointee_align = Some(byval_align); + } } _ => unreachable!(), } @@ -659,7 +665,8 @@ impl<'a, Ty> FnAbi<'a, Ty> { { if abi == spec::abi::Abi::X86Interrupt { if let Some(arg) = self.args.first_mut() { - arg.make_indirect_byval(); + // FIXME(pcwalton): This probably should use the x86 `byval` ABI... + arg.make_indirect_byval(None); } return Ok(()); } diff --git a/compiler/rustc_target/src/abi/call/wasm.rs b/compiler/rustc_target/src/abi/call/wasm.rs index 44427ee5317c1..0eb2309ecb28b 100644 --- a/compiler/rustc_target/src/abi/call/wasm.rs +++ b/compiler/rustc_target/src/abi/call/wasm.rs @@ -36,7 +36,7 @@ where { arg.extend_integer_width_to(32); if arg.layout.is_aggregate() && !unwrap_trivial_aggregate(cx, arg) { - arg.make_indirect_byval(); + arg.make_indirect_byval(None); } } diff --git a/compiler/rustc_target/src/abi/call/x86.rs b/compiler/rustc_target/src/abi/call/x86.rs index 7c26335dcf4cd..58c0717b7d1b3 100644 --- a/compiler/rustc_target/src/abi/call/x86.rs +++ b/compiler/rustc_target/src/abi/call/x86.rs @@ -1,5 +1,5 @@ use crate::abi::call::{ArgAttribute, FnAbi, PassMode, Reg, RegKind}; -use crate::abi::{HasDataLayout, TyAbiInterface}; +use crate::abi::{Align, HasDataLayout, TyAbiInterface}; use crate::spec::HasTargetSpec; #[derive(PartialEq)] @@ -53,11 +53,38 @@ where if arg.is_ignore() { continue; } - if arg.layout.is_aggregate() { - arg.make_indirect_byval(); - } else { + if !arg.layout.is_aggregate() { arg.extend_integer_width_to(32); + continue; } + + // We need to compute the alignment of the `byval` argument. The rules can be found in + // `X86_32ABIInfo::getTypeStackAlignInBytes` in Clang's `TargetInfo.cpp`. Summarized here, + // they are: + // + // 1. If the natural alignment of the type is less than or equal to 4, the alignment is 4. + // + // 2. Otherwise, on Linux, the alignment of any vector type is the natural alignment. + // (This doesn't matter here because we ensure we have an aggregate with the check above.) + // + // 3. Otherwise, on Apple platforms, the alignment of anything that contains a vector type + // is 16. + // + // 4. If none of these conditions are true, the alignment is 4. + let t = cx.target_spec(); + let align_4 = Align::from_bytes(4).unwrap(); + let align_16 = Align::from_bytes(16).unwrap(); + let byval_align = if arg.layout.align.abi < align_4 { + align_4 + } else if t.is_like_osx && arg.layout.align.abi >= align_16 { + // FIXME(pcwalton): This is dubious--we should actually be looking inside the type to + // determine if it contains SIMD vector values--but I think it's fine? + align_16 + } else { + align_4 + }; + + arg.make_indirect_byval(Some(byval_align)); } if flavor == Flavor::FastcallOrVectorcall { diff --git a/compiler/rustc_target/src/abi/call/x86_64.rs b/compiler/rustc_target/src/abi/call/x86_64.rs index c0c071a614f50..54abf23c15054 100644 --- a/compiler/rustc_target/src/abi/call/x86_64.rs +++ b/compiler/rustc_target/src/abi/call/x86_64.rs @@ -213,7 +213,7 @@ where match cls_or_mem { Err(Memory) => { if is_arg { - arg.make_indirect_byval(); + arg.make_indirect_byval(None); } else { // `sret` parameter thus one less integer register available arg.make_indirect(); diff --git a/src/test/codegen/align-byval.rs b/src/test/codegen/align-byval.rs new file mode 100644 index 0000000000000..35b3b1adc2cdc --- /dev/null +++ b/src/test/codegen/align-byval.rs @@ -0,0 +1,56 @@ +// ignore-x86 +// ignore-aarch64 +// ignore-aarch64_be +// ignore-arm +// ignore-armeb +// ignore-avr +// ignore-bpfel +// ignore-bpfeb +// ignore-hexagon +// ignore-mips +// ignore-mips64 +// ignore-msp430 +// ignore-powerpc64 +// ignore-powerpc64le +// ignore-powerpc +// ignore-r600 +// ignore-amdgcn +// ignore-sparc +// ignore-sparcv9 +// ignore-sparcel +// ignore-s390x +// ignore-tce +// ignore-thumb +// ignore-thumbeb +// ignore-xcore +// ignore-nvptx +// ignore-nvptx64 +// ignore-le32 +// ignore-le64 +// ignore-amdil +// ignore-amdil64 +// ignore-hsail +// ignore-hsail64 +// ignore-spir +// ignore-spir64 +// ignore-kalimba +// ignore-shave +// +// Tests that `byval` alignment is properly specified (#80127). +// The only targets that use `byval` are m68k, wasm, x86-64, and x86. Note that +// x86 has special rules (see #103830), and it's therefore ignored here. + +#[repr(C)] +#[repr(align(16))] +struct Foo { + a: [i32; 16], +} + +extern "C" { + // CHECK: declare void @f({{.*}}byval(%Foo) align 16{{.*}}) + fn f(foo: Foo); +} + +pub fn main() { + unsafe { f(Foo { a: [1; 16] }) } +} diff --git a/src/test/codegen/function-arguments-noopt.rs b/src/test/codegen/function-arguments-noopt.rs index ff76405a4ea32..5b41ab7f8dc0e 100644 --- a/src/test/codegen/function-arguments-noopt.rs +++ b/src/test/codegen/function-arguments-noopt.rs @@ -36,7 +36,7 @@ pub fn borrow_call(x: &i32, f: fn(&i32) -> &i32) -> &i32 { f(x) } -// CHECK: void @struct_({{%S\*|ptr}} sret(%S){{( %0)?}}, {{%S\*|ptr}} %x) +// CHECK: void @struct_({{%S\*|ptr}} sret(%S) align 4{{( %0)?}}, {{%S\*|ptr}} align 4 %x) #[no_mangle] pub fn struct_(x: S) -> S { x @@ -45,7 +45,7 @@ pub fn struct_(x: S) -> S { // CHECK-LABEL: @struct_call #[no_mangle] pub fn struct_call(x: S, f: fn(S) -> S) -> S { - // CHECK: call void %f({{%S\*|ptr}} sret(%S){{( %0)?}}, {{%S\*|ptr}} %{{.+}}) + // CHECK: call void %f({{%S\*|ptr}} sret(%S) align 4{{( %0)?}}, {{%S\*|ptr}} align 4 %{{.+}}) f(x) } diff --git a/src/test/codegen/function-arguments.rs b/src/test/codegen/function-arguments.rs index 44fee952307f2..fd413bcf1d57f 100644 --- a/src/test/codegen/function-arguments.rs +++ b/src/test/codegen/function-arguments.rs @@ -127,7 +127,7 @@ pub fn mutable_notunpin_borrow(_: &mut NotUnpin) { pub fn notunpin_borrow(_: &NotUnpin) { } -// CHECK: @indirect_struct({{%S\*|ptr}} noalias nocapture noundef readonly dereferenceable(32) %_1) +// CHECK: @indirect_struct({{%S\*|ptr}} noalias nocapture noundef readonly align 4 dereferenceable(32) %_1) #[no_mangle] pub fn indirect_struct(_: S) { } @@ -151,7 +151,7 @@ pub fn _box(x: Box) -> Box { x } -// CHECK: @struct_return({{%S\*|ptr}} noalias nocapture noundef sret(%S) dereferenceable(32){{( %0)?}}) +// CHECK: @struct_return({{%S\*|ptr}} noalias nocapture noundef sret(%S) align 4 dereferenceable(32){{( %0)?}}) #[no_mangle] pub fn struct_return() -> S { S { diff --git a/src/test/run-make-fulldeps/extern-fn-explicit-align/Makefile b/src/test/run-make-fulldeps/extern-fn-explicit-align/Makefile new file mode 100644 index 0000000000000..4f5d026f213f2 --- /dev/null +++ b/src/test/run-make-fulldeps/extern-fn-explicit-align/Makefile @@ -0,0 +1,5 @@ +include ../tools.mk + +all: $(call NATIVE_STATICLIB,test) + $(RUSTC) test.rs + $(call RUN,test) || exit 1 diff --git a/src/test/run-make-fulldeps/extern-fn-explicit-align/test.c b/src/test/run-make-fulldeps/extern-fn-explicit-align/test.c new file mode 100644 index 0000000000000..a015fc9aaf606 --- /dev/null +++ b/src/test/run-make-fulldeps/extern-fn-explicit-align/test.c @@ -0,0 +1,35 @@ +#include +#include +#include +#include + +struct TwoU64s +{ + uint64_t a; + uint64_t b; +} __attribute__((aligned(16))); + +struct BoolAndU32 +{ + bool a; + uint32_t b; +}; + +int32_t many_args( + void *a, + void *b, + const char *c, + uint64_t d, + bool e, + struct BoolAndU32 f, + void *g, + struct TwoU64s h, + void *i, + void *j, + void *k, + void *l, + const char *m) +{ + assert(strcmp(m, "Hello world") == 0); + return 0; +} diff --git a/src/test/run-make-fulldeps/extern-fn-explicit-align/test.rs b/src/test/run-make-fulldeps/extern-fn-explicit-align/test.rs new file mode 100644 index 0000000000000..ba6cc87bd1857 --- /dev/null +++ b/src/test/run-make-fulldeps/extern-fn-explicit-align/test.rs @@ -0,0 +1,61 @@ +// Issue #80127: Passing structs via FFI should work with explicit alignment. + +use std::ffi::CString; +use std::ptr::null_mut; + +#[derive(Clone, Copy, Debug, PartialEq)] +#[repr(C)] +#[repr(align(16))] +pub struct TwoU64s { + pub a: u64, + pub b: u64, +} + +#[repr(C)] +#[derive(Debug, Copy, Clone)] +pub struct BoolAndU32 { + pub a: bool, + pub b: u32, +} + +#[link(name = "test", kind = "static")] +extern "C" { + fn many_args( + a: *mut (), + b: *mut (), + c: *const i8, + d: u64, + e: bool, + f: BoolAndU32, + g: *mut (), + h: TwoU64s, + i: *mut (), + j: *mut (), + k: *mut (), + l: *mut (), + m: *const i8, + ) -> i32; +} + +fn main() { + let two_u64s = TwoU64s { a: 1, b: 2 }; + let bool_and_u32 = BoolAndU32 { a: true, b: 3 }; + let string = CString::new("Hello world").unwrap(); + unsafe { + many_args( + null_mut(), + null_mut(), + null_mut(), + 4, + true, + bool_and_u32, + null_mut(), + two_u64s, + null_mut(), + null_mut(), + null_mut(), + null_mut(), + string.as_ptr(), + ); + } +} From 02397c808e975b4e843d595713f03a9b442d352e Mon Sep 17 00:00:00 2001 From: Yuki Okushi Date: Thu, 22 Dec 2022 20:18:30 +0900 Subject: [PATCH 02/14] Add regression test for #86106 Signed-off-by: Yuki Okushi --- src/test/codegen/issue-86106.rs | 56 +++++++++++++++++++++++++++++++++ 1 file changed, 56 insertions(+) create mode 100644 src/test/codegen/issue-86106.rs diff --git a/src/test/codegen/issue-86106.rs b/src/test/codegen/issue-86106.rs new file mode 100644 index 0000000000000..e81cab9f1df71 --- /dev/null +++ b/src/test/codegen/issue-86106.rs @@ -0,0 +1,56 @@ +// min-llvm-version: 15.0 +// compile-flags: -C opt-level=3 -C target-cpu=native + +// The below two functions ensure that both `String::new()` and `"".to_string()` +// produce the identical code. + +#![crate_type = "lib"] + +// CHECK-LABEL: @string_new = unnamed_addr alias void (ptr), ptr @empty_to_string +// CHECK: define void @empty_to_string +#[no_mangle] +pub fn string_new() -> String { + String::new() +} + +#[no_mangle] +pub fn empty_to_string() -> String { + // CHECK-NOT: load i8 + // CHECK: store i64 + // CHECK-NEXT: getelementptr + // CHECK-NEXT: store ptr + // CHECK-NEXT: getelementptr + // CHECK-NOT: store i8 + // CHECK-NEXT: store i64 + // CHECK-NEXT: ret void + "".to_string() +} + +// The below two functions ensure that both `vec![]` and `vec![].clone()` +// produce the identical code. + +// CHECK-LABEL: @empty_vec +#[no_mangle] +pub fn empty_vec() -> Vec { + // CHECK: store i64 + // CHECK-NOT: load i8 + // CHECK-NEXT: getelementptr + // CHECK-NEXT: store ptr + // CHECK-NEXT: getelementptr + // CHECK-NEXT: store i64 + // CHECK-NEXT: ret void + vec![] +} + +// CHECK-LABEL: @empty_vec_clone +#[no_mangle] +pub fn empty_vec_clone() -> Vec { + // CHECK: store i64 + // CHECK-NOT: load i8 + // CHECK-NEXT: getelementptr + // CHECK-NEXT: store ptr + // CHECK-NEXT: getelementptr + // CHECK-NEXT: store i64 + // CHECK-NEXT: ret void + vec![].clone() +} From 3b16aea1237617578451d947a87f057fcc1250b3 Mon Sep 17 00:00:00 2001 From: Yuki Okushi Date: Thu, 22 Dec 2022 20:45:36 +0900 Subject: [PATCH 03/14] Apply review suggestions Signed-off-by: Yuki Okushi --- src/test/codegen/issue-86106.rs | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/src/test/codegen/issue-86106.rs b/src/test/codegen/issue-86106.rs index e81cab9f1df71..e647dbe2514db 100644 --- a/src/test/codegen/issue-86106.rs +++ b/src/test/codegen/issue-86106.rs @@ -1,5 +1,5 @@ // min-llvm-version: 15.0 -// compile-flags: -C opt-level=3 -C target-cpu=native +// compile-flags: -C opt-level=3 // The below two functions ensure that both `String::new()` and `"".to_string()` // produce the identical code. @@ -7,21 +7,20 @@ #![crate_type = "lib"] // CHECK-LABEL: @string_new = unnamed_addr alias void (ptr), ptr @empty_to_string -// CHECK: define void @empty_to_string #[no_mangle] pub fn string_new() -> String { String::new() } +// CHECK-LABEL: define void @empty_to_string #[no_mangle] pub fn empty_to_string() -> String { // CHECK-NOT: load i8 - // CHECK: store i64 + // CHECK: store i{{32|64}} // CHECK-NEXT: getelementptr // CHECK-NEXT: store ptr // CHECK-NEXT: getelementptr - // CHECK-NOT: store i8 - // CHECK-NEXT: store i64 + // CHECK-NEXT: store i{{32|64}} // CHECK-NEXT: ret void "".to_string() } @@ -32,12 +31,12 @@ pub fn empty_to_string() -> String { // CHECK-LABEL: @empty_vec #[no_mangle] pub fn empty_vec() -> Vec { - // CHECK: store i64 + // CHECK: store i{{32|64}} // CHECK-NOT: load i8 // CHECK-NEXT: getelementptr // CHECK-NEXT: store ptr // CHECK-NEXT: getelementptr - // CHECK-NEXT: store i64 + // CHECK-NEXT: store i{{32|64}} // CHECK-NEXT: ret void vec![] } @@ -45,12 +44,12 @@ pub fn empty_vec() -> Vec { // CHECK-LABEL: @empty_vec_clone #[no_mangle] pub fn empty_vec_clone() -> Vec { - // CHECK: store i64 + // CHECK: store i{{32|64}} // CHECK-NOT: load i8 // CHECK-NEXT: getelementptr // CHECK-NEXT: store ptr // CHECK-NEXT: getelementptr - // CHECK-NEXT: store i64 + // CHECK-NEXT: store i{{32|64}} // CHECK-NEXT: ret void vec![].clone() } From cbede85538d3ee59819c5d069ffe8d2dd7931749 Mon Sep 17 00:00:00 2001 From: Joshua Nelson Date: Thu, 29 Dec 2022 20:57:54 +0000 Subject: [PATCH 04/14] Support `x clean --stage 1 rustc_query_impl` Previously, clean only supported `--stage 0` for specific crates. The new `crate_description` function generates a string that looks like ``` : {rustc_query_impl} ``` --- src/bootstrap/builder.rs | 19 +++++++++++++++++++ src/bootstrap/clean.rs | 22 ++++++++++------------ src/bootstrap/compile.rs | 15 +++++++++++---- 3 files changed, 40 insertions(+), 16 deletions(-) diff --git a/src/bootstrap/builder.rs b/src/bootstrap/builder.rs index 707e4169002d9..97b353b5462f5 100644 --- a/src/bootstrap/builder.rs +++ b/src/bootstrap/builder.rs @@ -109,6 +109,25 @@ impl RunConfig<'_> { } } +/// A description of the crates in this set, suitable for passing to `builder.info`. +/// +/// `crates` should be generated by [`RunConfig::cargo_crates_in_set`]. +pub fn crate_description(crates: Interned>) -> String { + if crates.is_empty() { + return "".into(); + } + + let mut descr = String::from(": {"); + for krate in &*crates { + write!(descr, "{}, ", krate.strip_prefix("-p=").unwrap()).unwrap(); + } + + descr.pop(); + descr.pop(); + descr.push('}'); + descr +} + struct StepDescription { default: bool, only_hosts: bool, diff --git a/src/bootstrap/clean.rs b/src/bootstrap/clean.rs index 8e363ee1290ee..d887495d633f0 100644 --- a/src/bootstrap/clean.rs +++ b/src/bootstrap/clean.rs @@ -9,11 +9,10 @@ use std::fs; use std::io::{self, ErrorKind}; use std::path::Path; -use crate::builder::{Builder, RunConfig, ShouldRun, Step}; +use crate::builder::{crate_description, Builder, RunConfig, ShouldRun, Step}; use crate::cache::Interned; -use crate::config::TargetSelection; use crate::util::t; -use crate::{Build, Mode, Subcommand}; +use crate::{Build, Compiler, Mode, Subcommand}; #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] pub struct CleanAll {} @@ -40,7 +39,7 @@ macro_rules! clean_crate_tree { ( $( $name:ident, $mode:path, $root_crate:literal);+ $(;)? ) => { $( #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] pub struct $name { - target: TargetSelection, + compiler: Compiler, crates: Interned>, } @@ -54,22 +53,21 @@ macro_rules! clean_crate_tree { fn make_run(run: RunConfig<'_>) { let builder = run.builder; - if builder.top_stage != 0 { - panic!("non-stage-0 clean not supported for individual crates"); - } - builder.ensure(Self { crates: run.cargo_crates_in_set(), target: run.target }); + let compiler = builder.compiler(builder.top_stage, run.target); + builder.ensure(Self { crates: run.cargo_crates_in_set(), compiler }); } fn run(self, builder: &Builder<'_>) -> Self::Output { - let compiler = builder.compiler(0, self.target); - let mut cargo = builder.bare_cargo(compiler, $mode, self.target, "clean"); + let compiler = self.compiler; + let target = compiler.host; + let mut cargo = builder.bare_cargo(compiler, $mode, target, "clean"); for krate in &*self.crates { cargo.arg(krate); } builder.info(&format!( - "Cleaning stage{} {} artifacts ({} -> {})", - compiler.stage, stringify!($name).to_lowercase(), &compiler.host, self.target + "Cleaning stage{} {} artifacts ({} -> {}){}", + compiler.stage, stringify!($name).to_lowercase(), &compiler.host, target, crate_description(self.crates), )); // NOTE: doesn't use `run_cargo` because we don't want to save a stamp file, diff --git a/src/bootstrap/compile.rs b/src/bootstrap/compile.rs index 1030247b890c3..5bf5683f85dea 100644 --- a/src/bootstrap/compile.rs +++ b/src/bootstrap/compile.rs @@ -18,6 +18,7 @@ use std::str; use serde::Deserialize; +use crate::builder::crate_description; use crate::builder::Cargo; use crate::builder::{Builder, Kind, RunConfig, ShouldRun, Step}; use crate::cache::{Interned, INTERNER}; @@ -128,8 +129,11 @@ impl Step for Std { std_cargo(builder, target, compiler.stage, &mut cargo); builder.info(&format!( - "Building stage{} std artifacts ({} -> {})", - compiler.stage, &compiler.host, target + "Building stage{} std artifacts ({} -> {}){}", + compiler.stage, + &compiler.host, + target, + crate_description(self.crates), )); run_cargo( builder, @@ -715,8 +719,11 @@ impl Step for Rustc { } builder.info(&format!( - "Building stage{} compiler artifacts ({} -> {})", - compiler.stage, &compiler.host, target + "Building stage{} compiler artifacts ({} -> {}){}", + compiler.stage, + &compiler.host, + target, + crate_description(self.crates), )); run_cargo( builder, From e15272d8d6319726b22f72ba3ccfc5275e534ca8 Mon Sep 17 00:00:00 2001 From: Nilstrieb <48135649+Nilstrieb@users.noreply.github.com> Date: Wed, 14 Dec 2022 20:22:03 +0100 Subject: [PATCH 05/14] Add tidy check to deny merge commits This will prevent users with the pre-push hook from pushing a merge commit. Exceptions are added for subtree updates. These exceptions are a little hacky and may be non-exhaustive but can be extended in the future. --- src/tools/tidy/src/lib.rs | 1 + src/tools/tidy/src/main.rs | 2 + src/tools/tidy/src/no_merge.rs | 128 +++++++++++++++++++++++++++++++++ 3 files changed, 131 insertions(+) create mode 100644 src/tools/tidy/src/no_merge.rs diff --git a/src/tools/tidy/src/lib.rs b/src/tools/tidy/src/lib.rs index 698e4850bea9b..186cd9bb11b9a 100644 --- a/src/tools/tidy/src/lib.rs +++ b/src/tools/tidy/src/lib.rs @@ -48,6 +48,7 @@ pub mod errors; pub mod extdeps; pub mod features; pub mod mir_opt_tests; +pub mod no_merge; pub mod pal; pub mod primitive_docs; pub mod style; diff --git a/src/tools/tidy/src/main.rs b/src/tools/tidy/src/main.rs index 6714c63ee62a1..65f0fe9743f80 100644 --- a/src/tools/tidy/src/main.rs +++ b/src/tools/tidy/src/main.rs @@ -107,6 +107,8 @@ fn main() { check!(alphabetical, &compiler_path); check!(alphabetical, &library_path); + check!(no_merge, ()); + let collected = { drain_handles(&mut handles); diff --git a/src/tools/tidy/src/no_merge.rs b/src/tools/tidy/src/no_merge.rs new file mode 100644 index 0000000000000..445004e539aae --- /dev/null +++ b/src/tools/tidy/src/no_merge.rs @@ -0,0 +1,128 @@ +//! This check makes sure that no accidental merge commits are introduced to the repository. +//! It forbids all merge commits that are not caused by rollups/bors or subtree syncs. + +use std::process::Command; + +macro_rules! try_unwrap_in_ci { + ($expr:expr) => { + match $expr { + Ok(value) => value, + Err(err) if CiEnv::is_ci() => { + panic!("Encountered error while testing Git status: {:?}", err) + } + Err(_) => return, + } + }; +} + +pub fn check(_: (), bad: &mut bool) { + let remote = try_unwrap_in_ci!(get_rust_lang_rust_remote()); + let merge_commits = try_unwrap_in_ci!(find_merge_commits(&remote)); + + let mut bad_merge_commits = merge_commits.lines().filter(|commit| { + !( + // Bors is the ruler of merge commits. + commit.starts_with("Auto merge of") || commit.starts_with("Rollup merge of") + ) + }); + + if let Some(merge) = bad_merge_commits.next() { + tidy_error!( + bad, + "found a merge commit in the history: `{merge}`. +To resolve the issue, see this: https://rustc-dev-guide.rust-lang.org/git.html#i-made-a-merge-commit-by-accident. +If you're doing a subtree sync, add your tool to the list in the code that emitted this error." + ); + } +} + +/// Finds the remote for rust-lang/rust. +/// For example for these remotes it will return `upstream`. +/// ```text +/// origin https://github.com/Nilstrieb/rust.git (fetch) +/// origin https://github.com/Nilstrieb/rust.git (push) +/// upstream https://github.com/rust-lang/rust (fetch) +/// upstream https://github.com/rust-lang/rust (push) +/// ``` +fn get_rust_lang_rust_remote() -> Result { + let mut git = Command::new("git"); + git.args(["config", "--local", "--get-regex", "remote\\..*\\.url"]); + + let output = git.output().map_err(|err| format!("{err:?}"))?; + if !output.status.success() { + return Err(format!( + "failed to execute git config command: {}", + String::from_utf8_lossy(&output.stderr) + )); + } + + let stdout = String::from_utf8(output.stdout).map_err(|err| format!("{err:?}"))?; + + let rust_lang_remote = stdout + .lines() + .find(|remote| remote.contains("rust-lang")) + .ok_or_else(|| "rust-lang/rust remote not found".to_owned())?; + + let remote_name = + rust_lang_remote.split('.').nth(1).ok_or_else(|| "remote name not found".to_owned())?; + Ok(remote_name.into()) +} + +/// Runs `git log --merges --format=%s $REMOTE/master..HEAD` and returns all commits +fn find_merge_commits(remote: &str) -> Result { + let mut git = Command::new("git"); + git.args([ + "log", + "--merges", + "--format=%s", + &format!("{remote}/master..HEAD"), + // Ignore subtree syncs. Add your new subtrees here. + ":!src/tools/miri", + ":!src/tools/rust-analyzer", + ":!compiler/rustc_smir", + ":!library/portable-simd", + ":!compiler/rustc_codegen_gcc", + ":!src/tools/rustfmt", + ":!compiler/rustc_codegen_cranelift", + ":!src/tools/clippy", + ]); + + let output = git.output().map_err(|err| format!("{err:?}"))?; + if !output.status.success() { + return Err(format!( + "failed to execute git log command: {}", + String::from_utf8_lossy(&output.stderr) + )); + } + + let stdout = String::from_utf8(output.stdout).map_err(|err| format!("{err:?}"))?; + + Ok(stdout) +} + +#[derive(Copy, Clone, PartialEq, Eq, Debug)] +pub enum CiEnv { + /// Not a CI environment. + None, + /// The Azure Pipelines environment, for Linux (including Docker), Windows, and macOS builds. + AzurePipelines, + /// The GitHub Actions environment, for Linux (including Docker), Windows and macOS builds. + GitHubActions, +} + +impl CiEnv { + /// Obtains the current CI environment. + pub fn current() -> CiEnv { + if std::env::var("TF_BUILD").map_or(false, |e| e == "True") { + CiEnv::AzurePipelines + } else if std::env::var("GITHUB_ACTIONS").map_or(false, |e| e == "true") { + CiEnv::GitHubActions + } else { + CiEnv::None + } + } + + pub fn is_ci() -> bool { + Self::current() != CiEnv::None + } +} From ad9806b73ca3f29dba7ab8dff1ed9d38d3ff8117 Mon Sep 17 00:00:00 2001 From: Nilstrieb <48135649+Nilstrieb@users.noreply.github.com> Date: Fri, 23 Dec 2022 14:58:32 +0100 Subject: [PATCH 06/14] Checkout `master` branch in CI --- .github/workflows/ci.yml | 15 +++++++++++++++ src/ci/github-actions/ci.yml | 6 ++++++ 2 files changed, 21 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 23d3e71424b2b..2770bded30aec 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -71,6 +71,11 @@ jobs: uses: actions/checkout@v3 with: fetch-depth: 2 + - name: "checkout the `master` branch for tidy" + uses: actions/checkout@v3 + with: + ref: master + fetch-depth: 1 - name: configure the PR in which the error message will be posted run: "echo \"[CI_PR_NUMBER=$num]\"" env: @@ -485,6 +490,11 @@ jobs: uses: actions/checkout@v3 with: fetch-depth: 2 + - name: "checkout the `master` branch for tidy" + uses: actions/checkout@v3 + with: + ref: master + fetch-depth: 1 - name: configure the PR in which the error message will be posted run: "echo \"[CI_PR_NUMBER=$num]\"" env: @@ -600,6 +610,11 @@ jobs: uses: actions/checkout@v3 with: fetch-depth: 2 + - name: "checkout the `master` branch for tidy" + uses: actions/checkout@v3 + with: + ref: master + fetch-depth: 1 - name: configure the PR in which the error message will be posted run: "echo \"[CI_PR_NUMBER=$num]\"" env: diff --git a/src/ci/github-actions/ci.yml b/src/ci/github-actions/ci.yml index d33396dcc80fe..d3b07419486b2 100644 --- a/src/ci/github-actions/ci.yml +++ b/src/ci/github-actions/ci.yml @@ -103,6 +103,12 @@ x--expand-yaml-anchors--remove: with: fetch-depth: 2 + - name: checkout the `master` branch for tidy + uses: actions/checkout@v3 + with: + ref: master + fetch-depth: 1 + # Rust Log Analyzer can't currently detect the PR number of a GitHub # Actions build on its own, so a hint in the log message is needed to # point it in the right direction. From f9cc01126954e2fde28162fd83b4ef3e447526a6 Mon Sep 17 00:00:00 2001 From: Nilstrieb <48135649+Nilstrieb@users.noreply.github.com> Date: Fri, 30 Dec 2022 12:11:05 +0100 Subject: [PATCH 07/14] Tidy up tidy error codes check --- src/tools/tidy/src/error_codes_check.rs | 37 ++++++++----------------- 1 file changed, 11 insertions(+), 26 deletions(-) diff --git a/src/tools/tidy/src/error_codes_check.rs b/src/tools/tidy/src/error_codes_check.rs index fd870b0997ce0..40a46c630d70a 100644 --- a/src/tools/tidy/src/error_codes_check.rs +++ b/src/tools/tidy/src/error_codes_check.rs @@ -80,15 +80,6 @@ fn check_if_error_code_is_test_in_explanation(f: &str, err_code: &str) -> bool { ignore_found } -macro_rules! some_or_continue { - ($e:expr) => { - match $e { - Some(e) => e, - None => continue, - } - }; -} - fn extract_error_codes( f: &str, error_codes: &mut HashMap, @@ -122,10 +113,16 @@ fn extract_error_codes( Some((file_name, _)) => file_name, }, }; - let path = some_or_continue!(path.parent()) + + let Some(parent) = path.parent() else { + continue; + }; + + let path = parent .join(md_file_name) .canonicalize() .expect("failed to canonicalize error explanation file path"); + match read_to_string(&path) { Ok(content) => { let has_test = check_if_error_code_is_test_in_explanation(&content, &err_code); @@ -215,8 +212,6 @@ pub fn check(paths: &[&Path], bad: &mut bool) { // * #[error = "E0111"] let regex = Regex::new(r#"[(,"\s](E\d{4})[,)"]"#).unwrap(); - println!("Checking which error codes lack tests..."); - for path in paths { walk(path, &mut filter_dirs, &mut |entry, contents| { let file_name = entry.file_name(); @@ -245,20 +240,15 @@ pub fn check(paths: &[&Path], bad: &mut bool) { }); } if found_explanations == 0 { - eprintln!("No error code explanation was tested!"); - *bad = true; + tidy_error!(bad, "No error code explanation was tested!"); } if found_tests == 0 { - eprintln!("No error code was found in compilation errors!"); - *bad = true; + tidy_error!(bad, "No error code was found in compilation errors!"); } if explanations.is_empty() { - eprintln!("No error code explanation was found!"); - *bad = true; + tidy_error!(bad, "No error code explanation was found!"); } if errors.is_empty() { - println!("Found {} error codes", error_codes.len()); - for (err_code, error_status) in &error_codes { if !error_status.has_test && !EXEMPTED_FROM_TEST.contains(&err_code.as_str()) { errors.push(format!("Error code {err_code} needs to have at least one UI test!")); @@ -310,11 +300,6 @@ pub fn check(paths: &[&Path], bad: &mut bool) { } errors.sort(); for err in &errors { - eprintln!("{err}"); - } - println!("Found {} error(s) in error codes", errors.len()); - if !errors.is_empty() { - *bad = true; + tidy_error!(bad, "{err}"); } - println!("Done!"); } From 87bc29d02b65bd21a49136021d701de42b57390b Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Fri, 30 Dec 2022 16:27:56 +0100 Subject: [PATCH 08/14] Extend scraped examples layout GUI test for position of buttons --- src/test/rustdoc-gui/scrape-examples-layout.goml | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/src/test/rustdoc-gui/scrape-examples-layout.goml b/src/test/rustdoc-gui/scrape-examples-layout.goml index fde9a0ab0bc30..95102528ec11d 100644 --- a/src/test/rustdoc-gui/scrape-examples-layout.goml +++ b/src/test/rustdoc-gui/scrape-examples-layout.goml @@ -33,3 +33,17 @@ assert-property: ( ".more-scraped-examples .scraped-example:nth-child(6) .code-wrapper .src-line-numbers", {"clientWidth": |clientWidth|} ) + +// Check that for both mobile and desktop sizes, the buttons in scraped examples are displayed +// correctly. + +store-value: (offset_y, 4) + +// First with desktop +assert-position: (".scraped-example .code-wrapper", {"y": 255}) +assert-position: (".scraped-example .code-wrapper .prev", {"y": 255 + |offset_y|}) + +// Then with mobile +size: (600, 600) +assert-position: (".scraped-example .code-wrapper", {"y": 314}) +assert-position: (".scraped-example .code-wrapper .prev", {"y": 314 + |offset_y|}) From 878af66b53459659b3f53f1c26af1123e82d79a0 Mon Sep 17 00:00:00 2001 From: Nilstrieb <48135649+Nilstrieb@users.noreply.github.com> Date: Fri, 30 Dec 2022 11:11:33 +0100 Subject: [PATCH 09/14] Add `build_helper` crate to share code between tidy and bootstrap --- Cargo.lock | 5 +++ Cargo.toml | 1 + src/bootstrap/Cargo.lock | 5 +++ src/bootstrap/Cargo.toml | 1 + src/bootstrap/format.rs | 30 +-------------- src/bootstrap/lib.rs | 3 +- src/bootstrap/native.rs | 4 +- src/bootstrap/util.rs | 29 --------------- src/tools/build_helper/Cargo.toml | 8 ++++ src/tools/build_helper/src/ci.rs | 40 ++++++++++++++++++++ src/tools/build_helper/src/git.rs | 30 +++++++++++++++ src/tools/build_helper/src/lib.rs | 2 + src/tools/tidy/Cargo.toml | 1 + src/tools/tidy/src/no_merge.rs | 62 ++----------------------------- 14 files changed, 102 insertions(+), 119 deletions(-) create mode 100644 src/tools/build_helper/Cargo.toml create mode 100644 src/tools/build_helper/src/ci.rs create mode 100644 src/tools/build_helper/src/git.rs create mode 100644 src/tools/build_helper/src/lib.rs diff --git a/Cargo.lock b/Cargo.lock index 9581099c210ea..79c70ee31c718 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -259,6 +259,10 @@ dependencies = [ "toml", ] +[[package]] +name = "build_helper" +version = "0.1.0" + [[package]] name = "bump-stage0" version = "0.1.0" @@ -5304,6 +5308,7 @@ dependencies = [ name = "tidy" version = "0.1.0" dependencies = [ + "build_helper", "cargo_metadata 0.14.0", "ignore", "lazy_static", diff --git a/Cargo.toml b/Cargo.toml index 1cec4a84480c4..ce08d4edb567d 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -4,6 +4,7 @@ members = [ "library/std", "library/test", "src/rustdoc-json-types", + "src/tools/build_helper", "src/tools/cargotest", "src/tools/clippy", "src/tools/clippy/clippy_dev", diff --git a/src/bootstrap/Cargo.lock b/src/bootstrap/Cargo.lock index efe8ae3169fc2..4a0ba5925773e 100644 --- a/src/bootstrap/Cargo.lock +++ b/src/bootstrap/Cargo.lock @@ -36,6 +36,7 @@ dependencies = [ name = "bootstrap" version = "0.0.0" dependencies = [ + "build_helper", "cc", "cmake", "fd-lock", @@ -70,6 +71,10 @@ dependencies = [ "regex-automata", ] +[[package]] +name = "build_helper" +version = "0.1.0" + [[package]] name = "cc" version = "1.0.73" diff --git a/src/bootstrap/Cargo.toml b/src/bootstrap/Cargo.toml index fafe82a9c128a..22ceeca941e93 100644 --- a/src/bootstrap/Cargo.toml +++ b/src/bootstrap/Cargo.toml @@ -30,6 +30,7 @@ path = "bin/sccache-plus-cl.rs" test = false [dependencies] +build_helper = { path = "../tools/build_helper" } cmake = "0.1.38" fd-lock = "3.0.8" filetime = "0.2" diff --git a/src/bootstrap/format.rs b/src/bootstrap/format.rs index 1d57c6ecbbb8d..64ae3fe0db914 100644 --- a/src/bootstrap/format.rs +++ b/src/bootstrap/format.rs @@ -2,6 +2,7 @@ use crate::builder::Builder; use crate::util::{output, program_out_of_date, t}; +use build_helper::git::get_rust_lang_rust_remote; use ignore::WalkBuilder; use std::collections::VecDeque; use std::path::{Path, PathBuf}; @@ -100,35 +101,6 @@ fn get_modified_rs_files(build: &Builder<'_>) -> Option> { ) } -/// Finds the remote for rust-lang/rust. -/// For example for these remotes it will return `upstream`. -/// ```text -/// origin https://github.com/Nilstrieb/rust.git (fetch) -/// origin https://github.com/Nilstrieb/rust.git (push) -/// upstream https://github.com/rust-lang/rust (fetch) -/// upstream https://github.com/rust-lang/rust (push) -/// ``` -fn get_rust_lang_rust_remote() -> Result { - let mut git = Command::new("git"); - git.args(["config", "--local", "--get-regex", "remote\\..*\\.url"]); - - let output = git.output().map_err(|err| format!("{err:?}"))?; - if !output.status.success() { - return Err("failed to execute git config command".to_owned()); - } - - let stdout = String::from_utf8(output.stdout).map_err(|err| format!("{err:?}"))?; - - let rust_lang_remote = stdout - .lines() - .find(|remote| remote.contains("rust-lang")) - .ok_or_else(|| "rust-lang/rust remote not found".to_owned())?; - - let remote_name = - rust_lang_remote.split('.').nth(1).ok_or_else(|| "remote name not found".to_owned())?; - Ok(remote_name.into()) -} - #[derive(serde::Deserialize)] struct RustfmtConfig { ignore: Vec, diff --git a/src/bootstrap/lib.rs b/src/bootstrap/lib.rs index 5ea41d10bc817..d44b96cfb991e 100644 --- a/src/bootstrap/lib.rs +++ b/src/bootstrap/lib.rs @@ -113,6 +113,7 @@ use std::path::{Path, PathBuf}; use std::process::Command; use std::str; +use build_helper::ci::CiEnv; use channel::GitInfo; use config::{DryRun, Target}; use filetime::FileTime; @@ -121,7 +122,7 @@ use once_cell::sync::OnceCell; use crate::builder::Kind; use crate::config::{LlvmLibunwind, TargetSelection}; use crate::util::{ - exe, libdir, mtime, output, run, run_suppressed, symlink_dir, try_run_suppressed, CiEnv, + exe, libdir, mtime, output, run, run_suppressed, symlink_dir, try_run_suppressed, }; mod bolt; diff --git a/src/bootstrap/native.rs b/src/bootstrap/native.rs index 4e503dfe864e2..bdc81b07b8d54 100644 --- a/src/bootstrap/native.rs +++ b/src/bootstrap/native.rs @@ -24,6 +24,8 @@ use crate::util::get_clang_cl_resource_dir; use crate::util::{self, exe, output, t, up_to_date}; use crate::{CLang, GitRepo}; +use build_helper::ci::CiEnv; + #[derive(Clone)] pub struct LlvmResult { /// Path to llvm-config binary. @@ -217,7 +219,7 @@ pub(crate) fn is_ci_llvm_available(config: &Config, asserts: bool) -> bool { return false; } - if crate::util::CiEnv::is_ci() { + if CiEnv::is_ci() { // We assume we have access to git, so it's okay to unconditionally pass // `true` here. let llvm_sha = detect_llvm_sha(config, true); diff --git a/src/bootstrap/util.rs b/src/bootstrap/util.rs index 58220783228b2..8ce9a9ce38cc5 100644 --- a/src/bootstrap/util.rs +++ b/src/bootstrap/util.rs @@ -255,35 +255,6 @@ pub enum CiEnv { GitHubActions, } -impl CiEnv { - /// Obtains the current CI environment. - pub fn current() -> CiEnv { - if env::var("TF_BUILD").map_or(false, |e| e == "True") { - CiEnv::AzurePipelines - } else if env::var("GITHUB_ACTIONS").map_or(false, |e| e == "true") { - CiEnv::GitHubActions - } else { - CiEnv::None - } - } - - pub fn is_ci() -> bool { - Self::current() != CiEnv::None - } - - /// If in a CI environment, forces the command to run with colors. - pub fn force_coloring_in_ci(self, cmd: &mut Command) { - if self != CiEnv::None { - // Due to use of stamp/docker, the output stream of rustbuild is not - // a TTY in CI, so coloring is by-default turned off. - // The explicit `TERM=xterm` environment is needed for - // `--color always` to actually work. This env var was lost when - // compiling through the Makefile. Very strange. - cmd.env("TERM", "xterm").args(&["--color", "always"]); - } - } -} - pub fn forcing_clang_based_tests() -> bool { if let Some(var) = env::var_os("RUSTBUILD_FORCE_CLANG_BASED_TESTS") { match &var.to_string_lossy().to_lowercase()[..] { diff --git a/src/tools/build_helper/Cargo.toml b/src/tools/build_helper/Cargo.toml new file mode 100644 index 0000000000000..99f6fea2ecf99 --- /dev/null +++ b/src/tools/build_helper/Cargo.toml @@ -0,0 +1,8 @@ +[package] +name = "build_helper" +version = "0.1.0" +edition = "2021" + +# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html + +[dependencies] diff --git a/src/tools/build_helper/src/ci.rs b/src/tools/build_helper/src/ci.rs new file mode 100644 index 0000000000000..9f113c72b9338 --- /dev/null +++ b/src/tools/build_helper/src/ci.rs @@ -0,0 +1,40 @@ +use std::process::Command; + +#[derive(Copy, Clone, PartialEq, Eq, Debug)] +pub enum CiEnv { + /// Not a CI environment. + None, + /// The Azure Pipelines environment, for Linux (including Docker), Windows, and macOS builds. + AzurePipelines, + /// The GitHub Actions environment, for Linux (including Docker), Windows and macOS builds. + GitHubActions, +} + +impl CiEnv { + /// Obtains the current CI environment. + pub fn current() -> CiEnv { + if std::env::var("TF_BUILD").map_or(false, |e| e == "True") { + CiEnv::AzurePipelines + } else if std::env::var("GITHUB_ACTIONS").map_or(false, |e| e == "true") { + CiEnv::GitHubActions + } else { + CiEnv::None + } + } + + pub fn is_ci() -> bool { + Self::current() != CiEnv::None + } + + /// If in a CI environment, forces the command to run with colors. + pub fn force_coloring_in_ci(self, cmd: &mut Command) { + if self != CiEnv::None { + // Due to use of stamp/docker, the output stream of rustbuild is not + // a TTY in CI, so coloring is by-default turned off. + // The explicit `TERM=xterm` environment is needed for + // `--color always` to actually work. This env var was lost when + // compiling through the Makefile. Very strange. + cmd.env("TERM", "xterm").args(&["--color", "always"]); + } + } +} diff --git a/src/tools/build_helper/src/git.rs b/src/tools/build_helper/src/git.rs new file mode 100644 index 0000000000000..e9638206e6729 --- /dev/null +++ b/src/tools/build_helper/src/git.rs @@ -0,0 +1,30 @@ +use std::process::Command; + +/// Finds the remote for rust-lang/rust. +/// For example for these remotes it will return `upstream`. +/// ```text +/// origin https://github.com/Nilstrieb/rust.git (fetch) +/// origin https://github.com/Nilstrieb/rust.git (push) +/// upstream https://github.com/rust-lang/rust (fetch) +/// upstream https://github.com/rust-lang/rust (push) +/// ``` +pub fn get_rust_lang_rust_remote() -> Result { + let mut git = Command::new("git"); + git.args(["config", "--local", "--get-regex", "remote\\..*\\.url"]); + + let output = git.output().map_err(|err| format!("{err:?}"))?; + if !output.status.success() { + return Err("failed to execute git config command".to_owned()); + } + + let stdout = String::from_utf8(output.stdout).map_err(|err| format!("{err:?}"))?; + + let rust_lang_remote = stdout + .lines() + .find(|remote| remote.contains("rust-lang")) + .ok_or_else(|| "rust-lang/rust remote not found".to_owned())?; + + let remote_name = + rust_lang_remote.split('.').nth(1).ok_or_else(|| "remote name not found".to_owned())?; + Ok(remote_name.into()) +} diff --git a/src/tools/build_helper/src/lib.rs b/src/tools/build_helper/src/lib.rs new file mode 100644 index 0000000000000..d3d2323db853a --- /dev/null +++ b/src/tools/build_helper/src/lib.rs @@ -0,0 +1,2 @@ +pub mod ci; +pub mod git; diff --git a/src/tools/tidy/Cargo.toml b/src/tools/tidy/Cargo.toml index 97d038da702d5..ffedb4d5eb855 100644 --- a/src/tools/tidy/Cargo.toml +++ b/src/tools/tidy/Cargo.toml @@ -5,6 +5,7 @@ edition = "2021" autobins = false [dependencies] +build_helper = { path = "../build_helper" } cargo_metadata = "0.14" regex = "1" miropt-test-tools = { path = "../miropt-test-tools" } diff --git a/src/tools/tidy/src/no_merge.rs b/src/tools/tidy/src/no_merge.rs index 445004e539aae..362d6f6f4d77f 100644 --- a/src/tools/tidy/src/no_merge.rs +++ b/src/tools/tidy/src/no_merge.rs @@ -3,6 +3,9 @@ use std::process::Command; +use build_helper::ci::CiEnv; +use build_helper::git::get_rust_lang_rust_remote; + macro_rules! try_unwrap_in_ci { ($expr:expr) => { match $expr { @@ -36,38 +39,6 @@ If you're doing a subtree sync, add your tool to the list in the code that emitt } } -/// Finds the remote for rust-lang/rust. -/// For example for these remotes it will return `upstream`. -/// ```text -/// origin https://github.com/Nilstrieb/rust.git (fetch) -/// origin https://github.com/Nilstrieb/rust.git (push) -/// upstream https://github.com/rust-lang/rust (fetch) -/// upstream https://github.com/rust-lang/rust (push) -/// ``` -fn get_rust_lang_rust_remote() -> Result { - let mut git = Command::new("git"); - git.args(["config", "--local", "--get-regex", "remote\\..*\\.url"]); - - let output = git.output().map_err(|err| format!("{err:?}"))?; - if !output.status.success() { - return Err(format!( - "failed to execute git config command: {}", - String::from_utf8_lossy(&output.stderr) - )); - } - - let stdout = String::from_utf8(output.stdout).map_err(|err| format!("{err:?}"))?; - - let rust_lang_remote = stdout - .lines() - .find(|remote| remote.contains("rust-lang")) - .ok_or_else(|| "rust-lang/rust remote not found".to_owned())?; - - let remote_name = - rust_lang_remote.split('.').nth(1).ok_or_else(|| "remote name not found".to_owned())?; - Ok(remote_name.into()) -} - /// Runs `git log --merges --format=%s $REMOTE/master..HEAD` and returns all commits fn find_merge_commits(remote: &str) -> Result { let mut git = Command::new("git"); @@ -99,30 +70,3 @@ fn find_merge_commits(remote: &str) -> Result { Ok(stdout) } - -#[derive(Copy, Clone, PartialEq, Eq, Debug)] -pub enum CiEnv { - /// Not a CI environment. - None, - /// The Azure Pipelines environment, for Linux (including Docker), Windows, and macOS builds. - AzurePipelines, - /// The GitHub Actions environment, for Linux (including Docker), Windows and macOS builds. - GitHubActions, -} - -impl CiEnv { - /// Obtains the current CI environment. - pub fn current() -> CiEnv { - if std::env::var("TF_BUILD").map_or(false, |e| e == "True") { - CiEnv::AzurePipelines - } else if std::env::var("GITHUB_ACTIONS").map_or(false, |e| e == "true") { - CiEnv::GitHubActions - } else { - CiEnv::None - } - } - - pub fn is_ci() -> bool { - Self::current() != CiEnv::None - } -} From c8c849ef5c4932d5aec5ba6dbf936d3d18856f71 Mon Sep 17 00:00:00 2001 From: Joshua Nelson Date: Fri, 30 Dec 2022 20:03:24 +0000 Subject: [PATCH 10/14] Use more consistent progress messages in bootstrap Before: ``` Testing ["rustc_interface"] stage0 (aarch64-unknown-linux-gnu -> aarch64-unknown-linux-gnu) ``` After: ``` Testing {rustc_interface} stage0 (aarch64-unknown-linux-gnu -> aarch64-unknown-linux-gnu) ``` Note there is a slight consistency between `build` and `test`: The former doesn't print "compiler artifacts". It would be annoying to fix and doesn't hurt anything, so I left it be. ``` ; x t rustc_interface --stage 0 --dry-run Testing {rustc_interface} stage0 (aarch64-unknown-linux-gnu -> aarch64-unknown-linux-gnu) ; x b rustc_interface --stage 0 --dry-run Building {rustc_interface} stage0 compiler artifacts (aarch64-unknown-linux-gnu -> aarch64-unknown-linux-gnu) ``` --- src/bootstrap/builder.rs | 17 ++++++++--------- src/bootstrap/check.rs | 4 ++-- src/bootstrap/clean.rs | 4 ++-- src/bootstrap/compile.rs | 26 ++++++++++++++++++-------- src/bootstrap/doc.rs | 4 +++- src/bootstrap/test.rs | 9 +++++++-- 6 files changed, 40 insertions(+), 24 deletions(-) diff --git a/src/bootstrap/builder.rs b/src/bootstrap/builder.rs index 97b353b5462f5..66bc0f023b6c9 100644 --- a/src/bootstrap/builder.rs +++ b/src/bootstrap/builder.rs @@ -97,13 +97,13 @@ impl RunConfig<'_> { self.builder.build.build } - /// Return a `-p=x -p=y` string suitable for passing to a cargo invocation. + /// Return a list of crate names selected by `run.paths`. pub fn cargo_crates_in_set(&self) -> Interned> { let mut crates = Vec::new(); for krate in &self.paths { let path = krate.assert_single_path(); let crate_name = self.builder.crate_paths[&path.path]; - crates.push(format!("-p={crate_name}")); + crates.push(crate_name.to_string()); } INTERNER.intern_list(crates) } @@ -112,18 +112,17 @@ impl RunConfig<'_> { /// A description of the crates in this set, suitable for passing to `builder.info`. /// /// `crates` should be generated by [`RunConfig::cargo_crates_in_set`]. -pub fn crate_description(crates: Interned>) -> String { +pub fn crate_description(crates: &[impl AsRef]) -> String { if crates.is_empty() { return "".into(); } - let mut descr = String::from(": {"); - for krate in &*crates { - write!(descr, "{}, ", krate.strip_prefix("-p=").unwrap()).unwrap(); + let mut descr = String::from(" {"); + descr.push_str(crates[0].as_ref()); + for krate in &crates[1..] { + descr.push_str(", "); + descr.push_str(krate.as_ref()); } - - descr.pop(); - descr.pop(); descr.push('}'); descr } diff --git a/src/bootstrap/check.rs b/src/bootstrap/check.rs index 2e1bd8d6d1f6d..188c0a43e50d7 100644 --- a/src/bootstrap/check.rs +++ b/src/bootstrap/check.rs @@ -101,7 +101,7 @@ impl Step for Std { std_cargo(builder, target, compiler.stage, &mut cargo); builder.info(&format!( - "Checking stage{} std artifacts ({} -> {})", + "Checking stage{} library artifacts ({} -> {})", builder.top_stage, &compiler.host, target )); run_cargo( @@ -157,7 +157,7 @@ impl Step for Std { } builder.info(&format!( - "Checking stage{} std test/bench/example targets ({} -> {})", + "Checking stage{} library test/bench/example targets ({} -> {})", builder.top_stage, &compiler.host, target )); run_cargo( diff --git a/src/bootstrap/clean.rs b/src/bootstrap/clean.rs index d887495d633f0..468efc1114c43 100644 --- a/src/bootstrap/clean.rs +++ b/src/bootstrap/clean.rs @@ -66,8 +66,8 @@ macro_rules! clean_crate_tree { } builder.info(&format!( - "Cleaning stage{} {} artifacts ({} -> {}){}", - compiler.stage, stringify!($name).to_lowercase(), &compiler.host, target, crate_description(self.crates), + "Cleaning{} stage{} {} artifacts ({} -> {})", + crate_description(&self.crates), compiler.stage, stringify!($name).to_lowercase(), &compiler.host, target, )); // NOTE: doesn't use `run_cargo` because we don't want to save a stamp file, diff --git a/src/bootstrap/compile.rs b/src/bootstrap/compile.rs index 5bf5683f85dea..35bdfbfa0ed1a 100644 --- a/src/bootstrap/compile.rs +++ b/src/bootstrap/compile.rs @@ -111,7 +111,10 @@ impl Step for Std { let compiler_to_use = builder.compiler_for(compiler.stage, compiler.host, target); if compiler_to_use != compiler { builder.ensure(Std::new(compiler_to_use, target)); - builder.info(&format!("Uplifting stage1 std ({} -> {})", compiler_to_use.host, target)); + builder.info(&format!( + "Uplifting stage1 library ({} -> {})", + compiler_to_use.host, target + )); // Even if we're not building std this stage, the new sysroot must // still contain the third party objects needed by various targets. @@ -127,18 +130,21 @@ impl Step for Std { let mut cargo = builder.cargo(compiler, Mode::Std, SourceType::InTree, target, "build"); std_cargo(builder, target, compiler.stage, &mut cargo); + for krate in &*self.crates { + cargo.arg("-p").arg(krate); + } builder.info(&format!( - "Building stage{} std artifacts ({} -> {}){}", + "Building{} stage{} library artifacts ({} -> {})", + crate_description(&self.crates), compiler.stage, &compiler.host, target, - crate_description(self.crates), )); run_cargo( builder, cargo, - self.crates.to_vec(), + vec![], &libstd_stamp(builder, compiler, target), target_deps, false, @@ -429,7 +435,7 @@ impl Step for StdLink { let target_compiler = self.target_compiler; let target = self.target; builder.info(&format!( - "Copying stage{} std from stage{} ({} -> {} / {})", + "Copying stage{} library from stage{} ({} -> {} / {})", target_compiler.stage, compiler.stage, &compiler.host, target_compiler.host, target )); let libdir = builder.sysroot_libdir(target_compiler, target); @@ -718,17 +724,21 @@ impl Step for Rustc { } } + for krate in &*self.crates { + cargo.arg("-p").arg(krate); + } + builder.info(&format!( - "Building stage{} compiler artifacts ({} -> {}){}", + "Building{} stage{} compiler artifacts ({} -> {})", + crate_description(&self.crates), compiler.stage, &compiler.host, target, - crate_description(self.crates), )); run_cargo( builder, cargo, - self.crates.to_vec(), + vec![], &librustc_stamp(builder, compiler, target), vec![], false, diff --git a/src/bootstrap/doc.rs b/src/bootstrap/doc.rs index 5838049aa5c79..0562c270d54c0 100644 --- a/src/bootstrap/doc.rs +++ b/src/bootstrap/doc.rs @@ -12,6 +12,7 @@ use std::fs; use std::io; use std::path::{Path, PathBuf}; +use crate::builder::crate_description; use crate::builder::{Builder, Compiler, Kind, RunConfig, ShouldRun, Step}; use crate::cache::{Interned, INTERNER}; use crate::compile; @@ -554,7 +555,8 @@ fn doc_std( requested_crates: &[String], ) { builder.info(&format!( - "Documenting stage{} std ({}) in {} format", + "Documenting{} stage{} library ({}) in {} format", + crate_description(requested_crates), stage, target, format.as_str() diff --git a/src/bootstrap/test.rs b/src/bootstrap/test.rs index c8b4134391e5f..d5bec268a4567 100644 --- a/src/bootstrap/test.rs +++ b/src/bootstrap/test.rs @@ -11,6 +11,7 @@ use std::iter; use std::path::{Path, PathBuf}; use std::process::{Command, Stdio}; +use crate::builder::crate_description; use crate::builder::{Builder, Compiler, Kind, RunConfig, ShouldRun, Step}; use crate::cache::Interned; use crate::compile; @@ -2154,8 +2155,12 @@ impl Step for Crate { } builder.info(&format!( - "{} {:?} stage{} ({} -> {})", - test_kind, self.crates, compiler.stage, &compiler.host, target + "{}{} stage{} ({} -> {})", + test_kind, + crate_description(&self.crates), + compiler.stage, + &compiler.host, + target )); let _time = util::timeit(&builder); try_run(builder, &mut cargo.into()); From 9dfe50440e6d48bd2fd40a4b7b3992998e55eace Mon Sep 17 00:00:00 2001 From: Joshua Nelson Date: Fri, 30 Dec 2022 20:36:31 +0000 Subject: [PATCH 11/14] bootstrap: Get rid of `tail_args` in `stream_cargo` --- src/bootstrap/check.rs | 39 ++++++++++++--------------------------- src/bootstrap/compile.rs | 28 ++++------------------------ src/bootstrap/tool.rs | 2 +- 3 files changed, 17 insertions(+), 52 deletions(-) diff --git a/src/bootstrap/check.rs b/src/bootstrap/check.rs index 188c0a43e50d7..32e5d414061ec 100644 --- a/src/bootstrap/check.rs +++ b/src/bootstrap/check.rs @@ -99,19 +99,13 @@ impl Step for Std { cargo_subcommand(builder.kind), ); std_cargo(builder, target, compiler.stage, &mut cargo); + cargo.args(args(builder)); builder.info(&format!( "Checking stage{} library artifacts ({} -> {})", builder.top_stage, &compiler.host, target )); - run_cargo( - builder, - cargo, - args(builder), - &libstd_stamp(builder, compiler, target), - vec![], - true, - ); + run_cargo(builder, cargo, &libstd_stamp(builder, compiler, target), vec![], true); // We skip populating the sysroot in non-zero stage because that'll lead // to rlib/rmeta conflicts if std gets built during this session. @@ -155,19 +149,13 @@ impl Step for Std { for krate in builder.in_tree_crates("test", Some(target)) { cargo.arg("-p").arg(krate.name); } + cargo.args(args(builder)); builder.info(&format!( "Checking stage{} library test/bench/example targets ({} -> {})", builder.top_stage, &compiler.host, target )); - run_cargo( - builder, - cargo, - args(builder), - &libstd_test_stamp(builder, compiler, target), - vec![], - true, - ); + run_cargo(builder, cargo, &libstd_test_stamp(builder, compiler, target), vec![], true); } } @@ -231,19 +219,13 @@ impl Step for Rustc { for krate in builder.in_tree_crates("rustc-main", Some(target)) { cargo.arg("-p").arg(krate.name); } + cargo.args(args(builder)); builder.info(&format!( "Checking stage{} compiler artifacts ({} -> {})", builder.top_stage, &compiler.host, target )); - run_cargo( - builder, - cargo, - args(builder), - &librustc_stamp(builder, compiler, target), - vec![], - true, - ); + run_cargo(builder, cargo, &librustc_stamp(builder, compiler, target), vec![], true); let libdir = builder.sysroot_libdir(compiler, target); let hostdir = builder.sysroot_libdir(compiler, compiler.host); @@ -290,6 +272,7 @@ impl Step for CodegenBackend { .arg("--manifest-path") .arg(builder.src.join(format!("compiler/rustc_codegen_{}/Cargo.toml", backend))); rustc_cargo_env(builder, &mut cargo, target); + cargo.args(args(builder)); builder.info(&format!( "Checking stage{} {} artifacts ({} -> {})", @@ -299,7 +282,6 @@ impl Step for CodegenBackend { run_cargo( builder, cargo, - args(builder), &codegen_backend_stamp(builder, compiler, target, backend), vec![], true, @@ -355,11 +337,13 @@ impl Step for RustAnalyzer { cargo.arg("--benches"); } + cargo.args(args(builder)); + builder.info(&format!( "Checking stage{} {} artifacts ({} -> {})", compiler.stage, "rust-analyzer", &compiler.host.triple, target.triple )); - run_cargo(builder, cargo, args(builder), &stamp(builder, compiler, target), vec![], true); + run_cargo(builder, cargo, &stamp(builder, compiler, target), vec![], true); /// Cargo's output path in a given stage, compiled by a particular /// compiler for the specified target. @@ -413,6 +397,8 @@ macro_rules! tool_check_step { cargo.arg("--all-targets"); } + cargo.args(args(builder)); + // Enable internal lints for clippy and rustdoc // NOTE: this doesn't enable lints for any other tools unless they explicitly add `#![warn(rustc::internal)]` // See https://github.com/rust-lang/rust/pull/80573#issuecomment-754010776 @@ -428,7 +414,6 @@ macro_rules! tool_check_step { run_cargo( builder, cargo, - args(builder), &stamp(builder, compiler, target), vec![], true, diff --git a/src/bootstrap/compile.rs b/src/bootstrap/compile.rs index 35bdfbfa0ed1a..f9a04f2e91dbf 100644 --- a/src/bootstrap/compile.rs +++ b/src/bootstrap/compile.rs @@ -141,14 +141,7 @@ impl Step for Std { &compiler.host, target, )); - run_cargo( - builder, - cargo, - vec![], - &libstd_stamp(builder, compiler, target), - target_deps, - false, - ); + run_cargo(builder, cargo, &libstd_stamp(builder, compiler, target), target_deps, false); builder.ensure(StdLink::from_std( self, @@ -735,14 +728,7 @@ impl Step for Rustc { &compiler.host, target, )); - run_cargo( - builder, - cargo, - vec![], - &librustc_stamp(builder, compiler, target), - vec![], - false, - ); + run_cargo(builder, cargo, &librustc_stamp(builder, compiler, target), vec![], false); builder.ensure(RustcLink::from_rustc( self, @@ -998,7 +984,7 @@ impl Step for CodegenBackend { "Building stage{} codegen backend {} ({} -> {})", compiler.stage, backend, &compiler.host, target )); - let files = run_cargo(builder, cargo, vec![], &tmp_stamp, vec![], false); + let files = run_cargo(builder, cargo, &tmp_stamp, vec![], false); if builder.config.dry_run() { return; } @@ -1422,7 +1408,6 @@ pub fn add_to_sysroot( pub fn run_cargo( builder: &Builder<'_>, cargo: Cargo, - tail_args: Vec, stamp: &Path, additional_target_deps: Vec<(PathBuf, DependencyType)>, is_check: bool, @@ -1448,7 +1433,7 @@ pub fn run_cargo( // files we need to probe for later. let mut deps = Vec::new(); let mut toplevel = Vec::new(); - let ok = stream_cargo(builder, cargo, tail_args, &mut |msg| { + let ok = stream_cargo(builder, cargo, &mut |msg| { let (filenames, crate_types) = match msg { CargoMessage::CompilerArtifact { filenames, @@ -1563,7 +1548,6 @@ pub fn run_cargo( pub fn stream_cargo( builder: &Builder<'_>, cargo: Cargo, - tail_args: Vec, cb: &mut dyn FnMut(CargoMessage<'_>), ) -> bool { let mut cargo = Command::from(cargo); @@ -1583,10 +1567,6 @@ pub fn stream_cargo( } cargo.arg("--message-format").arg(message_format).stdout(Stdio::piped()); - for arg in tail_args { - cargo.arg(arg); - } - builder.verbose(&format!("running: {:?}", cargo)); let mut child = match cargo.spawn() { Ok(child) => child, diff --git a/src/bootstrap/tool.rs b/src/bootstrap/tool.rs index 24b033cc0dc5e..63026bd44d475 100644 --- a/src/bootstrap/tool.rs +++ b/src/bootstrap/tool.rs @@ -72,7 +72,7 @@ impl Step for ToolBuild { builder.info(&format!("Building stage{} tool {} ({})", compiler.stage, tool, target)); let mut duplicates = Vec::new(); - let is_expected = compile::stream_cargo(builder, cargo, vec![], &mut |msg| { + let is_expected = compile::stream_cargo(builder, cargo, &mut |msg| { // Only care about big things like the RLS/Cargo for now match tool { "rls" | "cargo" | "clippy-driver" | "miri" | "rustfmt" => {} From 75b3ee26cbeddcf2244e284d3c822d067cada2e2 Mon Sep 17 00:00:00 2001 From: Nilstrieb <48135649+Nilstrieb@users.noreply.github.com> Date: Fri, 30 Dec 2022 12:23:05 +0100 Subject: [PATCH 12/14] Make tidy errors red This makes it easier to see them (and makes people go owo). --- Cargo.lock | 9 +++++---- src/tools/tidy/Cargo.toml | 1 + src/tools/tidy/src/lib.rs | 31 ++++++++++++++++++++++--------- 3 files changed, 28 insertions(+), 13 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 9581099c210ea..f99e58e59b8e5 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2675,9 +2675,9 @@ dependencies = [ [[package]] name = "owo-colors" -version = "3.4.0" +version = "3.5.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "decf7381921fea4dcb2549c5667eda59b3ec297ab7e2b5fc33eac69d2e7da87b" +checksum = "c1b04fb49957986fdce4d6ee7a65027d55d4b6d2265e5848bbb507b58ccfdb6f" [[package]] name = "packed_simd_2" @@ -5203,9 +5203,9 @@ dependencies = [ [[package]] name = "termcolor" -version = "1.1.2" +version = "1.1.3" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "2dfed899f0eb03f32ee8c6a0aabdb8a7949659e3466561fc0adf54e26d88c5f4" +checksum = "bab24d30b911b2376f3a13cc2cd443142f0c81dda04c118693e35b3835757755" dependencies = [ "winapi-util", ] @@ -5309,6 +5309,7 @@ dependencies = [ "lazy_static", "miropt-test-tools", "regex", + "termcolor", "walkdir", ] diff --git a/src/tools/tidy/Cargo.toml b/src/tools/tidy/Cargo.toml index 97d038da702d5..fff83a1d097b3 100644 --- a/src/tools/tidy/Cargo.toml +++ b/src/tools/tidy/Cargo.toml @@ -11,6 +11,7 @@ miropt-test-tools = { path = "../miropt-test-tools" } lazy_static = "1" walkdir = "2" ignore = "0.4.18" +termcolor = "1.1.3" [[bin]] name = "rust-tidy" diff --git a/src/tools/tidy/src/lib.rs b/src/tools/tidy/src/lib.rs index 698e4850bea9b..ce7e7ac5cd4ca 100644 --- a/src/tools/tidy/src/lib.rs +++ b/src/tools/tidy/src/lib.rs @@ -3,6 +3,10 @@ //! This library contains the tidy lints and exposes it //! to be used by tools. +use std::fmt::Display; + +use termcolor::WriteColor; + /// A helper macro to `unwrap` a result except also print out details like: /// /// * The expression that failed @@ -26,18 +30,27 @@ macro_rules! t { } macro_rules! tidy_error { - ($bad:expr, $fmt:expr) => ({ - *$bad = true; - eprint!("tidy error: "); - eprintln!($fmt); - }); - ($bad:expr, $fmt:expr, $($arg:tt)*) => ({ - *$bad = true; - eprint!("tidy error: "); - eprintln!($fmt, $($arg)*); + ($bad:expr, $($fmt:tt)*) => ({ + $crate::tidy_error($bad, format_args!($($fmt)*)).expect("failed to output error"); }); } +fn tidy_error(bad: &mut bool, args: impl Display) -> std::io::Result<()> { + use std::io::Write; + use termcolor::{Color, ColorChoice, ColorSpec, StandardStream}; + + *bad = true; + + let mut stderr = StandardStream::stdout(ColorChoice::Auto); + stderr.set_color(ColorSpec::new().set_fg(Some(Color::Red)))?; + + write!(&mut stderr, "tidy error")?; + stderr.set_color(&ColorSpec::new())?; + + writeln!(&mut stderr, ": {args}")?; + Ok(()) +} + pub mod alphabetical; pub mod bins; pub mod debug_artifacts; From e2c5999265916399b0700b513cda0221fc8f7ad3 Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Fri, 30 Dec 2022 22:43:54 +0000 Subject: [PATCH 13/14] Dont use `--merge-base` during bootstrap formatting subcommand --- src/bootstrap/format.rs | 23 +++++++++-------------- 1 file changed, 9 insertions(+), 14 deletions(-) diff --git a/src/bootstrap/format.rs b/src/bootstrap/format.rs index 1d57c6ecbbb8d..84e4611895965 100644 --- a/src/bootstrap/format.rs +++ b/src/bootstrap/format.rs @@ -79,24 +79,19 @@ fn update_rustfmt_version(build: &Builder<'_>) { /// /// Returns `None` if all files should be formatted. fn get_modified_rs_files(build: &Builder<'_>) -> Option> { - let Ok(remote) = get_rust_lang_rust_remote() else {return None;}; + let Ok(remote) = get_rust_lang_rust_remote() else { return None; }; if !verify_rustfmt_version(build) { return None; } + + let merge_base = + output(build.config.git().arg("merge-base").arg(&format!("{remote}/master")).arg("HEAD")); Some( - output( - build - .config - .git() - .arg("diff-index") - .arg("--name-only") - .arg("--merge-base") - .arg(&format!("{remote}/master")), - ) - .lines() - .map(|s| s.trim().to_owned()) - .filter(|f| Path::new(f).extension().map_or(false, |ext| ext == "rs")) - .collect(), + output(build.config.git().arg("diff-index").arg("--name-only").arg(merge_base.trim())) + .lines() + .map(|s| s.trim().to_owned()) + .filter(|f| Path::new(f).extension().map_or(false, |ext| ext == "rs")) + .collect(), ) } From 6d2fe52dc5065278c57542487f5d0ab31313d3bf Mon Sep 17 00:00:00 2001 From: Joshua Nelson Date: Sat, 31 Dec 2022 00:53:46 +0000 Subject: [PATCH 14/14] Fix panic on `x build --help` --- src/bootstrap/flags.rs | 44 +++++++++++++++++++++--------------------- 1 file changed, 22 insertions(+), 22 deletions(-) diff --git a/src/bootstrap/flags.rs b/src/bootstrap/flags.rs index 3be7ad4075ed8..2c6d201d18fbe 100644 --- a/src/bootstrap/flags.rs +++ b/src/bootstrap/flags.rs @@ -352,32 +352,32 @@ To learn more about a subcommand, run `./x.py -h`", // fn usage() let usage = |exit_code: i32, opts: &Options, verbose: bool, subcommand_help: &str| -> ! { - // We have an unfortunate situation here: some Steps use `builder.in_tree_crates` to determine their paths. - // To determine those crates, we need to run `cargo metadata`, which means we need all submodules to be checked out. - // That takes a while to run, so only do it when paths were explicitly requested, not on all CLI errors. - // `Build::new` won't load submodules for the `setup` command. - let cmd = if verbose { - println!("note: updating submodules before printing available paths"); - "build" - } else { - "setup" - }; - let config = Config::parse(&[cmd.to_string()]); - let build = Build::new(config); - let paths = Builder::get_help(&build, subcommand); - println!("{}", opts.usage(subcommand_help)); - if let Some(s) = paths { - if verbose { + if verbose { + // We have an unfortunate situation here: some Steps use `builder.in_tree_crates` to determine their paths. + // To determine those crates, we need to run `cargo metadata`, which means we need all submodules to be checked out. + // That takes a while to run, so only do it when paths were explicitly requested, not on all CLI errors. + // `Build::new` won't load submodules for the `setup` command. + let cmd = if verbose { + println!("note: updating submodules before printing available paths"); + "build" + } else { + "setup" + }; + let config = Config::parse(&[cmd.to_string()]); + let build = Build::new(config); + let paths = Builder::get_help(&build, subcommand); + + if let Some(s) = paths { println!("{}", s); } else { - println!( - "Run `./x.py {} -h -v` to see a list of available paths.", - subcommand.as_str() - ); + panic!("No paths available for subcommand `{}`", subcommand.as_str()); } - } else if verbose { - panic!("No paths available for subcommand `{}`", subcommand.as_str()); + } else { + println!( + "Run `./x.py {} -h -v` to see a list of available paths.", + subcommand.as_str() + ); } crate::detail_exit(exit_code); };