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.

Sign up for GitHub

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

Jump to bottom

migrate thumb-none-cortex-m to rmake #128636

Merged
merged 2 commits into from
Aug 8, 2024
Merged

migrate thumb-none-cortex-m to rmake #128636

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f71a627
migrate `thumb-none-cortex-m` to rmake
folkertdev Aug 4, 2024
201ca3f
changes after review
folkertdev Aug 6, 2024
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
1 change: 0 additions & 1 deletion src/tools/tidy/src/allowed_run_make_makefiles.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,6 @@ run-make/stable-symbol-names/Makefile
run-make/staticlib-dylib-linkage/Makefile
run-make/symbol-mangling-hashed/Makefile
run-make/sysroot-crates-are-unstable/Makefile
run-make/thumb-none-cortex-m/Makefile
run-make/thumb-none-qemu/Makefile
run-make/translation/Makefile
run-make/x86_64-fortanix-unknown-sgx-lvi/Makefile
38 changes: 0 additions & 38 deletions tests/run-make/thumb-none-cortex-m/Makefile
View file
Open in desktop

This file was deleted.

58 changes: 58 additions & 0 deletions tests/run-make/thumb-none-cortex-m/rmake.rs
View file
Open in desktop
folkertdev marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
//! How to run this
//! $ ./x.py clean
//! $ ./x.py test --target thumbv6m-none-eabi,thumbv7m-none-eabi tests/run-make
//!
//! Supported targets:
//! - thumbv6m-none-eabi (Bare Cortex-M0, M0+, M1)
//! - thumbv7em-none-eabi (Bare Cortex-M4, M7)
//! - thumbv7em-none-eabihf (Bare Cortex-M4F, M7F, FPU, hardfloat)
//! - thumbv7m-none-eabi (Bare Cortex-M3)
Comment on lines +5 to +13
Copy link
Member

Choose a reason for hiding this comment

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

Question: do we know if these instructions and support targets are up-to-date?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is correct, but today probably should also include

  • thumbv8m.base-none-eabi
  • thumbv8m.main-none-eabi
  • thumbv8m.main-none-eabihf

I'm not sure whether CI even runs those though?

Copy link
Contributor Author

@folkertdev folkertdev Aug 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

based on https://hackmd.io/@thejpster/H1iTNJSRT the comment can be extended by

//! - thumbv8m.base-none-eabi (Bare Cortex-M23)
//! - thumbv8m.main-none-eabi (Bare Cortex-M33, M55, M85)
//! - thumbv8m.main-none-eabihf (Bare Cortex-M33, M55, M85, FPU, hardfloat)

this appears to work fine locally, but I think is only useful if CI actually runs these targets

Copy link
Member

Choose a reason for hiding this comment

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

it is correct, but today probably should also include

* thumbv8m.base-none-eabi
* thumbv8m.main-none-eabi
* thumbv8m.main-none-eabihf

I'm not sure whether CI even runs those though?

cc @rust-lang/infra dear infra people, do we know if any of our CI jobs exercise these targets?


//@ only-thumb

use std::path::PathBuf;

use run_make_support::rfs::create_dir;
use run_make_support::{cmd, env_var};

const CRATE: &str = "cortex-m";
const CRATE_URL: &str = "https://github.com/rust-embedded/cortex-m";
const CRATE_SHA1: &str = "a448e9156e2cb1e556e5441fd65426952ef4b927"; // v0.5.0
folkertdev marked this conversation as resolved.
Show resolved Hide resolved

fn main() {
// See below link for git usage:
// https://stackoverflow.com/questions/3489173#14091182
cmd("git").args(["clone", CRATE_URL, CRATE]).run();
std::env::set_current_dir(CRATE).unwrap();
cmd("git").args(["reset", "--hard", CRATE_SHA1]).run();
folkertdev marked this conversation as resolved.
Show resolved Hide resolved

let target_dir = PathBuf::from("target");
let target = env_var("TARGET");
folkertdev marked this conversation as resolved.
Show resolved Hide resolved

let manifest_path = PathBuf::from("Cargo.toml");

let path = env_var("PATH");
let rustc = env_var("RUSTC");
let bootstrap_cargo = env_var("BOOTSTRAP_CARGO");
let mut cmd = cmd(bootstrap_cargo);
cmd.args(&[
"build",
"--manifest-path",
manifest_path.to_str().unwrap(),
"-Zbuild-std=core",
"--target",
&target,
])
.env("PATH", path)
.env("RUSTC", rustc)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

should this be extracted into some cargo helper? it occurs a couple of times and is kind of inscrutable with all the env variables.

Copy link
Member

Choose a reason for hiding this comment

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

I would actually prefer if we keep the current bootstrap cargo incantations because there's another test compiler_builtins which does something similar but also subtly different. I want to investigate them together in a follow-up PR, so can you please leave me a FIXME pointing to #128734?

// Don't make lints fatal, but they need to at least warn
// or they break Cargo's target info parsing.
.env("RUSTFLAGS", "-Copt-level=0 -Cdebug-assertions=yes --cap-lints=warn")
.env("CARGO_TARGET_DIR", &target_dir)
.env("RUSTC_BOOTSTRAP", "1")
// Visual Studio 2022 requires that the LIB env var be set so it can
// find the Windows SDK.
.env("LIB", std::env::var("LIB").unwrap_or_default());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

is this required for these embedded targets? I guess it was in the makefile for a reason?

Copy link
Member

Choose a reason for hiding this comment

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

We can always try to run try-jobs and see if they pass without this env var. But I don't know if the embedded targets need the LIB env var.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well the windows SDK just does not seem relevant. let's try removing it.


cmd.run();
}
Loading