Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -662,7 +662,11 @@ fn main() -> Result<(), String> {
let intrinsics = parse_header(&header_content);
println!("Parsed {} scalar intrinsics", intrinsics.len());

let hexagon_dir = crate_dir.join("../core_arch/src/hexagon");
let hexagon_dir = std::env::args()
.nth(1)
.map(std::path::PathBuf::from)
.unwrap_or_else(|| crate_dir.join("../core_arch/src/hexagon"));
std::fs::create_dir_all(&hexagon_dir).map_err(|e| e.to_string())?;
let scalar_path = hexagon_dir.join("scalar.rs");

generate_scalar_file(&intrinsics, &scalar_path)?;
Expand Down
6 changes: 5 additions & 1 deletion library/stdarch/crates/stdarch-gen-hexagon/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1691,7 +1691,11 @@ fn main() -> Result<(), String> {
}

// Generate output files
let hexagon_dir = crate_dir.join("../core_arch/src/hexagon");
let hexagon_dir = std::env::args()
.nth(1)
.map(std::path::PathBuf::from)
.unwrap_or_else(|| crate_dir.join("../core_arch/src/hexagon"));
std::fs::create_dir_all(&hexagon_dir).map_err(|e| e.to_string())?;

// Generate v64.rs (64-byte vector mode)
let v64_path = hexagon_dir.join("v64.rs");
Expand Down
151 changes: 151 additions & 0 deletions src/bootstrap/src/core/build_steps/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -904,6 +904,157 @@ NOTE: if you're sure you want to do this, please open an issue as to why. In the
}
}

#[derive(Debug, Clone, PartialEq, Eq, Hash)]
pub struct StdarchGenCheck {
host: TargetSelection,
}

impl Step for StdarchGenCheck {
type Output = ();
const IS_HOST: bool = true;

fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> {
run.alias("stdarch-gen-check")
.path("library/stdarch/crates/stdarch-gen-arm")
.path("library/stdarch/crates/stdarch-gen-loongarch")
.path("library/stdarch/crates/stdarch-gen-hexagon")
.path("library/stdarch/crates/stdarch-gen-hexagon-scalar")
}

fn is_default_step(_builder: &Builder<'_>) -> bool {
true
}

fn make_run(run: RunConfig<'_>) {
run.builder.ensure(StdarchGenCheck { host: run.target });
}

fn run(self, builder: &Builder<'_>) {
let stdarch_root = builder.src.join("library/stdarch");
let bless = builder.config.cmd.bless();

// The generators shell out to rustfmt to format their output so we need
// it discoverable on PATH. Bootstrap downloads one and exposes it here.
let rustfmt_path = builder.config.initial_rustfmt.clone().unwrap_or_else(|| {
eprintln!(
"stdarch-gen-check: a rustfmt binary is required but none was found.\n\
Set `rustfmt = true` in `bootstrap.toml` so bootstrap downloads one."
);
crate::exit!(1);
});

let mut path_dirs: Vec<PathBuf> = Vec::new();
if let Some(cargo_dir) = builder.initial_cargo.parent() {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why does cargo need to be in PATH?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Because cargo_dir is providing rustc . As cargo run build looks up rustc on PATH .

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In that case we should set RUSTC to point to the rustc that we want to invoke directly, I think.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Okay makes sense can do.

path_dirs.push(cargo_dir.to_path_buf());
}
if let Some(rustfmt_dir) = rustfmt_path.parent() {
path_dirs.push(rustfmt_dir.to_path_buf());
}
let old_path = env::var_os("PATH").unwrap_or_default();
let new_path = env::join_paths(path_dirs.into_iter().chain(env::split_paths(&old_path)))
.expect("could not build PATH for stdarch-gen-check");

let work_dir = builder.out.join("stdarch-gen-check");
let cargo_target_dir = work_dir.join("target");

// Writable copy of `crates/core_arch` that the generators write into.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
// Writable copy of `crates/core_arch` that the generators write into.
// Writable copy of `crates/core_arch` that the generators write into when blessing.

let src_core_arch = stdarch_root.join("crates/core_arch");
let out_core_arch = work_dir.join("core_arch");
if out_core_arch.exists() {
t!(fs::remove_dir_all(&out_core_arch));
}
cp_writable_r(&src_core_arch, &out_core_arch);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why do we need to copy things over? My mental model of how this should work is:

  1. When running the test, we generate the output code into some temporary directory on disk
  2. We then check if the generated files are the same as the files committed in the repo.
  3. If not, and we are not blessing, the test fails.
  4. If not, and we are blessing, the generated files override the committed files (we also need to ensure that any leftover files are deleted, so the committed dir should probably be emptied first).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Your mental model is right it's just the copy is mainly there for the arm generator. It writes its output files directly into existing folders under core_arch/src but doesn't create those folders itself so they have to already exist. Copying core_arch over gives it those folders. Having the full copy also let run a single git diff over the whole folder instead of listing every generated file by hand. It's the same thing stdarch's own check-stdarch-gen CI does.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'd rather edit the generator to create those directories and make it so that it doesn't write to the source dirs, than do hacks with cp_writable_r.

In fact, to ensure that we run the same logic, it might be better if the check + bless logic actually lived within the generators themselves, and maybe we could also unify them under a single crate, it looks like they are separate right now? I wonder if there's an opportunity to do some refactoring there to make the whole setup less hacky.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I approached with cp_writable_r to keep the changes minimal and strictly around bootstrap step only but if you want then I think we can edit the generator and can get rid of cp_writable_r . Yes they are separate right now and yeah by unifying it will make the whole setup less hacky for sure .

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah I'd rather also do targeted refactorings to stdarch to make the overall testing infra better, rather than to just try hacking bootstrap around whatever stdarch is currently doing.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Such changes are welcome. You can experiment here, and once you're happy we can see how to sync the changes back into stdarch

let out_core_arch_src = out_core_arch.join("src");

// Mirrors the check-stdarch-gen CI job with its output redirected
// into `out_core_arch` args follow `--` and `out_dir` when set becomes OUT_DIR.
let run_gen = |selector: &str, pkg: &str, args: &[&OsStr], out_dir: Option<&Path>| {
let mut cmd = command(&builder.initial_cargo);
cmd.current_dir(&stdarch_root);
cmd.arg("run").arg(selector).arg(pkg).arg("--release").arg("--").args(args);
// RUSTC_BOOTSTRAP=1 allow nightly features when building tools against stage0.
cmd.env("RUSTC_BOOTSTRAP", "1");
cmd.env("PATH", &new_path);
cmd.env("CARGO_TARGET_DIR", &cargo_target_dir);
if let Some(out_dir) = out_dir {
cmd.env("OUT_DIR", out_dir);
}
cmd.run(builder);
};

let hexagon_out = out_core_arch_src.join("hexagon");
run_gen(
"--bin",
"stdarch-gen-arm",
&[OsStr::new("crates/stdarch-gen-arm/spec"), out_core_arch_src.as_os_str()],
None,
);
run_gen(
"--bin",
"stdarch-gen-loongarch",
&[OsStr::new("crates/stdarch-gen-loongarch/lsx.spec")],
Some(&out_core_arch),
);
run_gen(
"--bin",
"stdarch-gen-loongarch",
&[OsStr::new("crates/stdarch-gen-loongarch/lasx.spec")],
Some(&out_core_arch),
);
run_gen("-p", "stdarch-gen-hexagon", &[hexagon_out.as_os_str()], None);
run_gen("-p", "stdarch-gen-hexagon-scalar", &[hexagon_out.as_os_str()], None);

let mut git = helpers::git(Some(&stdarch_root));
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hmm, I was kinda thinking that we would check the files using Rust code, but I checked bootstrap and it doesn't seem like we already do any diffing at the moment, so git diff seems fine.

git.arg("diff")
.arg("--no-index")
.arg("--exit-code")
.arg(&src_core_arch)
.arg(&out_core_arch);
let clean = git.allow_failure().run(builder);

if !clean {
if bless {
builder.info(
"stdarch-gen-check: updating generated files in the source tree. \
Review the diff and commit.",
);
cp_writable_r(&out_core_arch, &src_core_arch);
} else {
eprintln!(
"stdarch-gen-check: generated files are out of date.\n\
Run `./x test stdarch-gen-check --bless` to update them, then commit."
);
crate::exit!(1);
}
}
}
}

/// Recursively copy src to dst clearing the read-only bit so the destination can be
/// regenerated even when src lives on a read-only filesystem.
fn cp_writable_r(src: &Path, dst: &Path) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This shouldn't be needed. We shouldn't ever bless on CI, nor copy any files on CI, only locally.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The read-only handling isn't for bless it only runs locally never on CI. It's there because we copy core_arch into the work folder at the start of every run and on CI the original files are read-only so the copies come out read-only too.

let metadata = t!(fs::symlink_metadata(src));
if metadata.file_type().is_symlink() {
return;
}
if metadata.is_dir() {
t!(fs::create_dir_all(dst));
for entry in t!(fs::read_dir(src)) {
let entry = t!(entry);
cp_writable_r(&entry.path(), &dst.join(entry.file_name()));
}
} else {
if let Some(parent) = dst.parent() {
t!(fs::create_dir_all(parent));
}
t!(fs::copy(src, dst));
let mut perms = t!(fs::metadata(dst)).permissions();
#[allow(clippy::permissions_set_readonly_false)]
perms.set_readonly(false);
t!(fs::set_permissions(dst, perms));
}
}

#[derive(Debug, Clone, PartialEq, Eq, Hash)]
pub struct Clippy {
compilers: RustcPrivateCompilers,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,13 @@ expression: test
[Test] test::RustcBook
targets: [x86_64-unknown-linux-gnu]
- Set({test::src/doc/rustc})
[Test] test::StdarchGenCheck
targets: [x86_64-unknown-linux-gnu]
- Set({test::library/stdarch/crates/stdarch-gen-arm})
- Set({test::library/stdarch/crates/stdarch-gen-hexagon})
- Set({test::library/stdarch/crates/stdarch-gen-hexagon-scalar})
- Set({test::library/stdarch/crates/stdarch-gen-loongarch})
- Set({test::stdarch-gen-check})
[Test] test::RustdocJSStd
targets: [x86_64-unknown-linux-gnu]
- Suite(test::tests/rustdoc-js-std)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,3 +18,9 @@ expression: test library
- Set({test::library/sysroot})
- Set({test::library/test})
- Set({test::library/unwind})
[Test] test::StdarchGenCheck
targets: [x86_64-unknown-linux-gnu]
- Set({test::library/stdarch/crates/stdarch-gen-arm})
- Set({test::library/stdarch/crates/stdarch-gen-hexagon})
- Set({test::library/stdarch/crates/stdarch-gen-hexagon-scalar})
- Set({test::library/stdarch/crates/stdarch-gen-loongarch})
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,13 @@ expression: test --skip=coverage
[Test] test::RustcBook
targets: [x86_64-unknown-linux-gnu]
- Set({test::src/doc/rustc})
[Test] test::StdarchGenCheck
targets: [x86_64-unknown-linux-gnu]
- Set({test::library/stdarch/crates/stdarch-gen-arm})
- Set({test::library/stdarch/crates/stdarch-gen-hexagon})
- Set({test::library/stdarch/crates/stdarch-gen-hexagon-scalar})
- Set({test::library/stdarch/crates/stdarch-gen-loongarch})
- Set({test::stdarch-gen-check})
[Test] test::RustdocJSStd
targets: [x86_64-unknown-linux-gnu]
- Suite(test::tests/rustdoc-js-std)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,13 @@ expression: test --skip=tests
[Test] test::RustcBook
targets: [x86_64-unknown-linux-gnu]
- Set({test::src/doc/rustc})
[Test] test::StdarchGenCheck
targets: [x86_64-unknown-linux-gnu]
- Set({test::library/stdarch/crates/stdarch-gen-arm})
- Set({test::library/stdarch/crates/stdarch-gen-hexagon})
- Set({test::library/stdarch/crates/stdarch-gen-hexagon-scalar})
- Set({test::library/stdarch/crates/stdarch-gen-loongarch})
- Set({test::stdarch-gen-check})
[Test] test::RustdocTheme
targets: [x86_64-unknown-linux-gnu]
- Set({test::src/tools/rustdoc-themes})
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,9 @@ expression: test --skip=tests --skip=coverage-map --skip=coverage-run --skip=lib
[Test] test::RustcBook
targets: [x86_64-unknown-linux-gnu]
- Set({test::src/doc/rustc})
[Test] test::StdarchGenCheck
targets: [x86_64-unknown-linux-gnu]
- Set({test::stdarch-gen-check})
[Test] test::RustdocTheme
targets: [x86_64-unknown-linux-gnu]
- Set({test::src/tools/rustdoc-themes})
Expand Down
1 change: 1 addition & 0 deletions src/bootstrap/src/core/builder/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -910,6 +910,7 @@ impl<'a> Builder<'a> {
test::CargoMiri,
test::Clippy,
test::CompiletestTest,
test::StdarchGenCheck,
test::CrateRunMakeSupport,
test::CrateBuildHelper,
test::RustdocJSStd,
Expand Down
Loading