From e9e3c38d01669d87d2d14b17d5c7e0b2188991df Mon Sep 17 00:00:00 2001 From: onur-ozkan Date: Mon, 10 Jun 2024 17:07:19 +0300 Subject: [PATCH] tidy: skip submodules if not present for non-CI environments Signed-off-by: onur-ozkan --- Cargo.lock | 1 + src/bootstrap/src/core/build_steps/dist.rs | 2 +- src/bootstrap/src/core/builder.rs | 28 ++-------------------- src/tools/build_helper/src/util.rs | 28 ++++++++++++++++++++++ src/tools/tidy/Cargo.toml | 1 + src/tools/tidy/src/deps.rs | 14 +++++++++++ src/tools/tidy/src/extdeps.rs | 15 +++++++++++- 7 files changed, 61 insertions(+), 28 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 58d95f927bf95..a9fdf8926ee03 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5697,6 +5697,7 @@ dependencies = [ name = "tidy" version = "0.1.0" dependencies = [ + "build_helper", "cargo_metadata 0.15.4", "ignore", "miropt-test-tools", diff --git a/src/bootstrap/src/core/build_steps/dist.rs b/src/bootstrap/src/core/build_steps/dist.rs index 60b1ff3e4413e..7adc8245d8661 100644 --- a/src/bootstrap/src/core/build_steps/dist.rs +++ b/src/bootstrap/src/core/build_steps/dist.rs @@ -1014,7 +1014,7 @@ impl Step for PlainSourceTarball { // perhaps it should be removed in favor of making `dist` perform the `vendor` step? // Ensure we have all submodules from src and other directories checked out. - for submodule in builder.get_all_submodules() { + for submodule in build_helper::util::parse_gitmodules(&builder.src) { builder.update_submodule(Path::new(submodule)); } diff --git a/src/bootstrap/src/core/builder.rs b/src/bootstrap/src/core/builder.rs index 12d2bb18ab7ca..d419a76282e29 100644 --- a/src/bootstrap/src/core/builder.rs +++ b/src/bootstrap/src/core/builder.rs @@ -4,13 +4,11 @@ use std::collections::BTreeSet; use std::env; use std::ffi::{OsStr, OsString}; use std::fmt::{Debug, Write}; -use std::fs::{self, File}; +use std::fs; use std::hash::Hash; -use std::io::{BufRead, BufReader}; use std::ops::Deref; use std::path::{Path, PathBuf}; use std::process::Command; -use std::sync::OnceLock; use std::time::{Duration, Instant}; use crate::core::build_steps::tool::{self, SourceType}; @@ -577,7 +575,7 @@ impl<'a> ShouldRun<'a> { /// /// [`path`]: ShouldRun::path pub fn paths(mut self, paths: &[&str]) -> Self { - let submodules_paths = self.builder.get_all_submodules(); + let submodules_paths = build_helper::util::parse_gitmodules(&self.builder.src); self.paths.insert(PathSet::Set( paths @@ -2238,28 +2236,6 @@ impl<'a> Builder<'a> { out } - /// Return paths of all submodules. - pub fn get_all_submodules(&self) -> &[String] { - static SUBMODULES_PATHS: OnceLock> = OnceLock::new(); - - let init_submodules_paths = |src: &PathBuf| { - let file = File::open(src.join(".gitmodules")).unwrap(); - - let mut submodules_paths = vec![]; - for line in BufReader::new(file).lines().map_while(Result::ok) { - let line = line.trim(); - if line.starts_with("path") { - let actual_path = line.split(' ').last().expect("Couldn't get value of path"); - submodules_paths.push(actual_path.to_owned()); - } - } - - submodules_paths - }; - - SUBMODULES_PATHS.get_or_init(|| init_submodules_paths(&self.src)) - } - /// Ensure that a given step is built *only if it's supposed to be built by default*, returning /// its output. This will cache the step, so it's safe (and good!) to call this as often as /// needed to ensure that all dependencies are build. diff --git a/src/tools/build_helper/src/util.rs b/src/tools/build_helper/src/util.rs index 5801a8648f227..72c05c4c48abf 100644 --- a/src/tools/build_helper/src/util.rs +++ b/src/tools/build_helper/src/util.rs @@ -1,4 +1,8 @@ +use std::fs::File; +use std::io::{BufRead, BufReader}; +use std::path::Path; use std::process::Command; +use std::sync::OnceLock; /// Invokes `build_helper::util::detail_exit` with `cfg!(test)` /// @@ -45,3 +49,27 @@ pub fn try_run(cmd: &mut Command, print_cmd_on_fail: bool) -> Result<(), ()> { Ok(()) } } + +/// Returns the submodule paths from the `.gitmodules` file in the given directory. +pub fn parse_gitmodules(target_dir: &Path) -> &[String] { + static SUBMODULES_PATHS: OnceLock> = OnceLock::new(); + let gitmodules = target_dir.join(".gitmodules"); + assert!(gitmodules.exists(), "'{}' file is missing.", gitmodules.display()); + + let init_submodules_paths = || { + let file = File::open(gitmodules).unwrap(); + + let mut submodules_paths = vec![]; + for line in BufReader::new(file).lines().map_while(Result::ok) { + let line = line.trim(); + if line.starts_with("path") { + let actual_path = line.split(' ').last().expect("Couldn't get value of path"); + submodules_paths.push(actual_path.to_owned()); + } + } + + submodules_paths + }; + + SUBMODULES_PATHS.get_or_init(|| init_submodules_paths()) +} diff --git a/src/tools/tidy/Cargo.toml b/src/tools/tidy/Cargo.toml index 63963b0bd1ced..c5204a9fee728 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.15" regex = "1" miropt-test-tools = { path = "../miropt-test-tools" } diff --git a/src/tools/tidy/src/deps.rs b/src/tools/tidy/src/deps.rs index 2dd3d17f9e3e7..cb47b23b0d05c 100644 --- a/src/tools/tidy/src/deps.rs +++ b/src/tools/tidy/src/deps.rs @@ -1,7 +1,9 @@ //! Checks the licenses of third-party dependencies. +use build_helper::ci::CiEnv; use cargo_metadata::{Metadata, Package, PackageId}; use std::collections::HashSet; +use std::fs::read_dir; use std::path::Path; /// These are licenses that are allowed for all crates, including the runtime, @@ -514,7 +516,19 @@ const PERMITTED_CRANELIFT_DEPENDENCIES: &[&str] = &[ pub fn check(root: &Path, cargo: &Path, bad: &mut bool) { let mut checked_runtime_licenses = false; + let submodules = build_helper::util::parse_gitmodules(root); for &(workspace, exceptions, permitted_deps) in WORKSPACES { + // Skip if it's a submodule, not in a CI environment, and not initialized. + // + // This prevents enforcing developers to fetch submodules for tidy. + if submodules.contains(&workspace.into()) + && !CiEnv::is_ci() + // If the directory is empty, we can consider it as an uninitialized submodule. + && read_dir(root.join(workspace)).unwrap().next().is_none() + { + continue; + } + if !root.join(workspace).join("Cargo.lock").exists() { tidy_error!(bad, "the `{workspace}` workspace doesn't have a Cargo.lock"); continue; diff --git a/src/tools/tidy/src/extdeps.rs b/src/tools/tidy/src/extdeps.rs index 2118de5f20411..8bb80f1171184 100644 --- a/src/tools/tidy/src/extdeps.rs +++ b/src/tools/tidy/src/extdeps.rs @@ -1,6 +1,7 @@ //! Check for external package sources. Allow only vendorable packages. -use std::fs; +use build_helper::ci::CiEnv; +use std::fs::{self, read_dir}; use std::path::Path; /// List of allowed sources for packages. @@ -13,7 +14,19 @@ const ALLOWED_SOURCES: &[&str] = &[ /// Checks for external package sources. `root` is the path to the directory that contains the /// workspace `Cargo.toml`. pub fn check(root: &Path, bad: &mut bool) { + let submodules = build_helper::util::parse_gitmodules(root); for &(workspace, _, _) in crate::deps::WORKSPACES { + // Skip if it's a submodule, not in a CI environment, and not initialized. + // + // This prevents enforcing developers to fetch submodules for tidy. + if submodules.contains(&workspace.into()) + && !CiEnv::is_ci() + // If the directory is empty, we can consider it as an uninitialized submodule. + && read_dir(root.join(workspace)).unwrap().next().is_none() + { + continue; + } + // FIXME check other workspaces too // `Cargo.lock` of rust. let path = root.join(workspace).join("Cargo.lock");