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
12 changes: 11 additions & 1 deletion .github/workflows/main.yml
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,16 @@ env:
RUSTFLAGS: "-Dwarnings"

jobs:
todo_check:
runs-on: ubuntu-latest
timeout-minutes: 10

steps:
- uses: actions/checkout@v4

- name: Check todo
run: ./y.sh check-todo

rustfmt:
runs-on: ubuntu-latest
timeout-minutes: 10
Expand Down Expand Up @@ -232,7 +242,7 @@ jobs:
runs-on: ubuntu-latest
timeout-minutes: 10
if: ${{ github.ref == 'refs/heads/main' }}
needs: [rustfmt, test, bench, dist]
needs: [todo_check, rustfmt, test, bench, dist]

permissions:
contents: write # for creating the dev tag and release
Expand Down
13 changes: 12 additions & 1 deletion build_system/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ mod prepare;
mod rustc_info;
mod shared_utils;
mod tests;
mod todo;
mod utils;

fn usage() {
Expand All @@ -38,6 +39,7 @@ enum Command {
Test,
AbiCafe,
Bench,
CheckTodo,
}

#[derive(Copy, Clone, Debug)]
Expand Down Expand Up @@ -66,6 +68,7 @@ fn main() {
Some("test") => Command::Test,
Some("abi-cafe") => Command::AbiCafe,
Some("bench") => Command::Bench,
Some("check-todo") => Command::CheckTodo,
Some(flag) if flag.starts_with('-') => arg_error!("Expected command found flag {}", flag),
Some(command) => arg_error!("Unknown command {}", command),
None => {
Expand Down Expand Up @@ -139,6 +142,14 @@ fn main() {
process::exit(0);
}

if command == Command::CheckTodo {
if let Err(err) = todo::run() {
eprintln!("{err}");
process::exit(1);
}
process::exit(0);
Comment on lines +146 to +150
Copy link
Member

Choose a reason for hiding this comment

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

Maybe make todo::run() return ! and move the process::exit() into it?

}

let rustup_toolchain_name = match (env::var("CARGO"), env::var("RUSTC"), env::var("RUSTDOC")) {
(Ok(_), Ok(_), Ok(_)) => None,
(_, Err(_), Err(_)) => Some(rustc_info::get_toolchain_name()),
Expand Down Expand Up @@ -202,7 +213,7 @@ fn main() {
))
};
match command {
Command::Prepare => {
Command::Prepare | Command::CheckTodo => {
// Handled above
}
Command::Test => {
Expand Down
87 changes: 87 additions & 0 deletions build_system/todo.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
use std::ffi::OsStr;
use std::fs;
use std::path::{Path, PathBuf};
use std::process::Command;

const EXTENSIONS: &[&str] =
&["rs", "py", "js", "sh", "c", "cpp", "h", "md", "css", "ftl", "toml", "yml", "yaml"];
const SKIP_FILES: &[&str] = &["build_system/todo.rs", ".github/workflows/main.yml"];

fn has_supported_extension(path: &Path) -> bool {
path.extension().is_some_and(|ext| EXTENSIONS.iter().any(|e| ext == OsStr::new(e)))
}

fn is_editor_temp(path: &Path) -> bool {
path.file_name().is_some_and(|name| name.to_string_lossy().starts_with(".#"))
}

fn list_tracked_files() -> Result<Vec<PathBuf>, String> {
let output = Command::new("git")
.args(["ls-files", "-z"])
.output()
.map_err(|e| format!("Failed to run `git ls-files`: {e}"))?;

if !output.status.success() {
let stderr = String::from_utf8_lossy(&output.stderr);
return Err(format!("`git ls-files` failed: {stderr}"));
}

let mut files = Vec::new();
for entry in output.stdout.split(|b| *b == 0) {
if entry.is_empty() {
continue;
}
let path = std::str::from_utf8(entry)
.map_err(|_| "Non-utf8 path returned by `git ls-files`".to_string())?;
Copy link
Member

Choose a reason for hiding this comment

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

These can also use unwrap.

files.push(PathBuf::from(path));
}

Ok(files)
}

pub(crate) fn run() -> Result<(), String> {
let files = list_tracked_files()?;
let mut errors = Vec::new();
// Avoid embedding the task marker in source so greps only find real occurrences.
let todo_marker = "todo".to_ascii_uppercase();
Copy link
Member

Choose a reason for hiding this comment

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

You are both excluding this file from the todo check and also avoid the literal string TODO. One of the two seems redundant.


for file in files {
if is_editor_temp(&file) {
continue;
}
Comment on lines +49 to +51
Copy link
Member

Choose a reason for hiding this comment

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

This is already excluded by list_tracked_files, right?

if SKIP_FILES.iter().any(|skip| file.ends_with(Path::new(skip))) {
continue;
}
if !has_supported_extension(&file) {
continue;
}

let bytes =
fs::read(&file).map_err(|e| format!("Failed to read `{}`: {e}", file.display()))?;
Copy link
Member

Choose a reason for hiding this comment

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

No need to do error handling. Just unwrap. The build system doesn't have to be robust against hardware/OS issues. It isn't robust in other places either.

let Ok(contents) = std::str::from_utf8(&bytes) else {
continue;
};
Copy link
Member

Choose a reason for hiding this comment

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

All source files should be valid UTF-8, so unwrap here is fine.


for (i, line) in contents.split('\n').enumerate() {
let trimmed = line.trim();
if trimmed.contains(&todo_marker) {
errors.push(format!(
Copy link
Member

Choose a reason for hiding this comment

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

This can immediately print the error and increment an error counter.

"{}:{}: {} is used for tasks that should be done before merging a PR; if you want to leave a message in the codebase use FIXME",
file.display(),
i + 1,
todo_marker
));
}
}
}

if errors.is_empty() {
return Ok(());
}

for err in &errors {
eprintln!("{err}");
}

Err(format!("found {} {}(s)", errors.len(), todo_marker))
}
1 change: 1 addition & 0 deletions build_system/usage.txt
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ USAGE:
./y.sh test [--sysroot none|clif|llvm] [--out-dir DIR] [--download-dir DIR] [--no-unstable-features] [--frozen] [--skip-test TESTNAME]
./y.sh abi-cafe [--sysroot none|clif|llvm] [--out-dir DIR] [--download-dir DIR] [--no-unstable-features] [--frozen]
./y.sh bench [--sysroot none|clif|llvm] [--out-dir DIR] [--download-dir DIR] [--no-unstable-features] [--frozen]
./y.sh check-todo

OPTIONS:
--sysroot none|clif|llvm
Expand Down
Loading