From bb536d2fe3046e25378fff7781f3f419595b3c0f Mon Sep 17 00:00:00 2001 From: Greg Soltis Date: Thu, 7 Dec 2023 09:50:17 -0800 Subject: [PATCH 1/3] Integration test for global deps folder hashing --- .../fixtures/global_deps/.gitignore | 3 +++ .../global_deps/apps/my-app/.env.local | 0 .../global_deps/apps/my-app/package.json | 10 +++++++ .../fixtures/global_deps/global_deps/bar.txt | 1 + .../fixtures/global_deps/global_deps/foo.txt | 1 + .../fixtures/global_deps/package.json | 10 +++++++ .../global_deps/packages/another/package.json | 4 +++ .../global_deps/packages/util/package.json | 7 +++++ .../fixtures/global_deps/turbo.json | 21 +++++++++++++++ .../integration/tests/global-deps.t | 26 +++++++++++++++++++ 10 files changed, 83 insertions(+) create mode 100644 turborepo-tests/integration/fixtures/global_deps/.gitignore create mode 100644 turborepo-tests/integration/fixtures/global_deps/apps/my-app/.env.local create mode 100644 turborepo-tests/integration/fixtures/global_deps/apps/my-app/package.json create mode 100644 turborepo-tests/integration/fixtures/global_deps/global_deps/bar.txt create mode 100644 turborepo-tests/integration/fixtures/global_deps/global_deps/foo.txt create mode 100644 turborepo-tests/integration/fixtures/global_deps/package.json create mode 100644 turborepo-tests/integration/fixtures/global_deps/packages/another/package.json create mode 100644 turborepo-tests/integration/fixtures/global_deps/packages/util/package.json create mode 100644 turborepo-tests/integration/fixtures/global_deps/turbo.json create mode 100644 turborepo-tests/integration/tests/global-deps.t diff --git a/turborepo-tests/integration/fixtures/global_deps/.gitignore b/turborepo-tests/integration/fixtures/global_deps/.gitignore new file mode 100644 index 0000000000000..77af9fc60321d --- /dev/null +++ b/turborepo-tests/integration/fixtures/global_deps/.gitignore @@ -0,0 +1,3 @@ +node_modules/ +.turbo +.npmrc diff --git a/turborepo-tests/integration/fixtures/global_deps/apps/my-app/.env.local b/turborepo-tests/integration/fixtures/global_deps/apps/my-app/.env.local new file mode 100644 index 0000000000000..e69de29bb2d1d diff --git a/turborepo-tests/integration/fixtures/global_deps/apps/my-app/package.json b/turborepo-tests/integration/fixtures/global_deps/apps/my-app/package.json new file mode 100644 index 0000000000000..1746e0db23610 --- /dev/null +++ b/turborepo-tests/integration/fixtures/global_deps/apps/my-app/package.json @@ -0,0 +1,10 @@ +{ + "name": "my-app", + "scripts": { + "build": "echo building", + "maybefails": "exit 4" + }, + "dependencies": { + "util": "*" + } +} diff --git a/turborepo-tests/integration/fixtures/global_deps/global_deps/bar.txt b/turborepo-tests/integration/fixtures/global_deps/global_deps/bar.txt new file mode 100644 index 0000000000000..5e849f85df5d1 --- /dev/null +++ b/turborepo-tests/integration/fixtures/global_deps/global_deps/bar.txt @@ -0,0 +1 @@ +other file, not a global dependency diff --git a/turborepo-tests/integration/fixtures/global_deps/global_deps/foo.txt b/turborepo-tests/integration/fixtures/global_deps/global_deps/foo.txt new file mode 100644 index 0000000000000..eebae5f3ca7b5 --- /dev/null +++ b/turborepo-tests/integration/fixtures/global_deps/global_deps/foo.txt @@ -0,0 +1 @@ +global dep! all tasks depend on this content! diff --git a/turborepo-tests/integration/fixtures/global_deps/package.json b/turborepo-tests/integration/fixtures/global_deps/package.json new file mode 100644 index 0000000000000..b3601d4b13a2c --- /dev/null +++ b/turborepo-tests/integration/fixtures/global_deps/package.json @@ -0,0 +1,10 @@ +{ + "name": "monorepo", + "scripts": { + "something": "turbo run build" + }, + "workspaces": [ + "apps/**", + "packages/**" + ] +} diff --git a/turborepo-tests/integration/fixtures/global_deps/packages/another/package.json b/turborepo-tests/integration/fixtures/global_deps/packages/another/package.json new file mode 100644 index 0000000000000..e9e34ea52c154 --- /dev/null +++ b/turborepo-tests/integration/fixtures/global_deps/packages/another/package.json @@ -0,0 +1,4 @@ +{ + "name": "another", + "scripts": {} +} diff --git a/turborepo-tests/integration/fixtures/global_deps/packages/util/package.json b/turborepo-tests/integration/fixtures/global_deps/packages/util/package.json new file mode 100644 index 0000000000000..e755064fd7893 --- /dev/null +++ b/turborepo-tests/integration/fixtures/global_deps/packages/util/package.json @@ -0,0 +1,7 @@ +{ + "name": "util", + "scripts": { + "build": "echo building", + "maybefails": "echo didnotfail" + } +} diff --git a/turborepo-tests/integration/fixtures/global_deps/turbo.json b/turborepo-tests/integration/fixtures/global_deps/turbo.json new file mode 100644 index 0000000000000..390c719bdf582 --- /dev/null +++ b/turborepo-tests/integration/fixtures/global_deps/turbo.json @@ -0,0 +1,21 @@ +{ + "$schema": "https://turbo.build/schema.json", + "globalDependencies": ["global_deps/**"], + "globalEnv": ["SOME_ENV_VAR"], + "pipeline": { + "build": { + "env": ["NODE_ENV"], + "outputs": [] + }, + // this comment verifies that turbo can read .json files with comments + "my-app#build": { + "outputs": ["banana.txt", "apple.json"], + "dotEnv": [".env.local"] + }, + + "something": {}, + "//#something": {}, + + "maybefails": {} + } +} diff --git a/turborepo-tests/integration/tests/global-deps.t b/turborepo-tests/integration/tests/global-deps.t new file mode 100644 index 0000000000000..6b01cf8d9a93d --- /dev/null +++ b/turborepo-tests/integration/tests/global-deps.t @@ -0,0 +1,26 @@ +Setup + $ . ${TESTDIR}/../../helpers/setup_integration_test.sh global_deps + +Run a build + $ ${TURBO} build -F my-app --output-logs=hash-only + \xe2\x80\xa2 Packages in scope: my-app (esc) + \xe2\x80\xa2 Running build in 1 packages (esc) + \xe2\x80\xa2 Remote caching disabled (esc) + my-app:build: cache miss, executing 81165ceb4ed0e31f + + Tasks: 1 successful, 1 total + Cached: 0 cached, 1 total + Time:\s*[\.0-9]+m?s (re) + + + $ echo "new text" > global_deps/foo.txt + $ ${TURBO} build -F my-app --output-logs=hash-only + \xe2\x80\xa2 Packages in scope: my-app (esc) + \xe2\x80\xa2 Running build in 1 packages (esc) + \xe2\x80\xa2 Remote caching disabled (esc) + my-app:build: cache miss, executing 7bc88ba3e84628fd + + Tasks: 1 successful, 1 total + Cached: 0 cached, 1 total + Time:\s*[\.0-9]+m?s (re) + From b549c2fa6a6eaee34e6f00ea25370682f8759de7 Mon Sep 17 00:00:00 2001 From: Alexander Lyon Date: Thu, 7 Dec 2023 18:37:31 +0000 Subject: [PATCH 2/3] refactor: pull out walking code into new function --- crates/turborepo-lib/src/run/global_hash.rs | 91 +++++++++++---------- 1 file changed, 49 insertions(+), 42 deletions(-) diff --git a/crates/turborepo-lib/src/run/global_hash.rs b/crates/turborepo-lib/src/run/global_hash.rs index 4e229591ed8b4..79c943bc012ce 100644 --- a/crates/turborepo-lib/src/run/global_hash.rs +++ b/crates/turborepo-lib/src/run/global_hash.rs @@ -3,7 +3,7 @@ use std::collections::{HashMap, HashSet}; use globwalk::WalkType; use thiserror::Error; use tracing::debug; -use turbopath::{AbsoluteSystemPath, RelativeUnixPathBuf}; +use turbopath::{AbsoluteSystemPath, AbsoluteSystemPathBuf, RelativeUnixPathBuf}; use turborepo_env::{get_global_hashable_env_vars, DetailedMap, EnvironmentVariableMap}; use turborepo_lockfiles::Lockfile; use turborepo_repository::package_manager::{self, PackageManager}; @@ -68,47 +68,8 @@ pub fn get_global_hash_inputs<'a, L: ?Sized + Lockfile>( global_hashable_env_vars.all.names() ); - let mut global_deps = HashSet::new(); - - if !global_file_dependencies.is_empty() { - let exclusions = match package_manager.get_workspace_globs(root_path) { - Ok(globs) => globs.raw_exclusions, - // If we hit a missing workspaces error, we could be in single package mode - // so we should just use the default globs - Err(package_manager::Error::Workspace(_)) => { - package_manager.get_default_exclusions().collect() - } - Err(err) => { - debug!("no workspace globs found"); - return Err(err.into()); - } - }; - - // This is a bit of a hack to ensure that we don't crash - // when given an absolute path on Windows. We don't support - // absolute paths, but the ':' from the drive letter will also - // fail to compile to a glob. We already know we aren't going to - // get anything good, and we've already logged a warning, but we - // can modify the glob so it compiles. This is similar to the old - // behavior, which tacked it on to the end of the base path unmodified, - // and then would produce no files. - #[cfg(windows)] - let windows_global_file_dependencies: Vec = global_file_dependencies - .iter() - .map(|s| s.replace(":", "")) - .collect(); - let files = globwalk::globwalk( - root_path, - #[cfg(not(windows))] - global_file_dependencies, - #[cfg(windows)] - windows_global_file_dependencies.as_slice(), - &exclusions, - WalkType::All, - )?; - - global_deps.extend(files); - } + let mut global_deps = + collect_global_deps(package_manager, root_path, global_file_dependencies)?; if lockfile.is_none() { global_deps.insert(root_path.join_component("package.json")); @@ -158,6 +119,52 @@ pub fn get_global_hash_inputs<'a, L: ?Sized + Lockfile>( }) } +fn collect_global_deps( + package_manager: &PackageManager, + root_path: &AbsoluteSystemPath, + global_file_dependencies: &[String], +) -> Result, Error> { + if global_file_dependencies.is_empty() { + return Ok(HashSet::new()); + } + let exclusions = match package_manager.get_workspace_globs(root_path) { + Ok(globs) => globs.raw_exclusions, + // If we hit a missing workspaces error, we could be in single package mode + // so we should just use the default globs + Err(package_manager::Error::Workspace(_)) => { + package_manager.get_default_exclusions().collect() + } + Err(err) => { + debug!("no workspace globs found"); + return Err(err.into()); + } + }; + + // This is a bit of a hack to ensure that we don't crash + // when given an absolute path on Windows. We don't support + // absolute paths, but the ':' from the drive letter will also + // fail to compile to a glob. We already know we aren't going to + // get anything good, and we've already logged a warning, but we + // can modify the glob so it compiles. This is similar to the old + // behavior, which tacked it on to the end of the base path unmodified, + // and then would produce no files. + #[cfg(windows)] + let windows_global_file_dependencies: Vec = global_file_dependencies + .iter() + .map(|s| s.replace(":", "")) + .collect(); + + Ok(globwalk::globwalk( + root_path, + #[cfg(not(windows))] + global_file_dependencies, + #[cfg(windows)] + windows_global_file_dependencies.as_slice(), + &exclusions, + WalkType::All, + )?) +} + impl<'a> GlobalHashableInputs<'a> { pub fn calculate_global_hash_from_inputs(&mut self) -> String { match self.env_mode { From e3c7c41d2cbec2e07f1107c91dccb92496ff3242 Mon Sep 17 00:00:00 2001 From: Alexander Lyon Date: Thu, 7 Dec 2023 18:39:21 +0000 Subject: [PATCH 3/3] fix: ensure that global hash walking only yields files --- crates/turborepo-lib/src/run/global_hash.rs | 43 ++++++++++++++++++++- 1 file changed, 41 insertions(+), 2 deletions(-) diff --git a/crates/turborepo-lib/src/run/global_hash.rs b/crates/turborepo-lib/src/run/global_hash.rs index 79c943bc012ce..954a79e9dce7c 100644 --- a/crates/turborepo-lib/src/run/global_hash.rs +++ b/crates/turborepo-lib/src/run/global_hash.rs @@ -161,7 +161,7 @@ fn collect_global_deps( #[cfg(windows)] windows_global_file_dependencies.as_slice(), &exclusions, - WalkType::All, + WalkType::Files, )?) } @@ -213,7 +213,7 @@ mod tests { use turborepo_repository::package_manager::PackageManager; use super::get_global_hash_inputs; - use crate::cli::EnvMode; + use crate::{cli::EnvMode, run::global_hash::collect_global_deps}; #[test] fn test_absolute_path() { @@ -251,4 +251,43 @@ mod tests { ); assert!(result.is_ok()); } + + /// get_global_hash_inputs should not yield any folders when walking since + /// turbo does not consider changes to folders when evaluating hashes, + /// only to files + #[test] + fn test_collect_only_yields_files() { + let tmp = tempfile::tempdir().unwrap(); + + // add some files + // - package.json + // - src/index.js + // - src/index.test.js + // - empty-folder/ + + let root = AbsoluteSystemPathBuf::try_from(tmp.path()).unwrap(); + let src = root.join_component("src"); + + root.join_component("package.json") + .create_with_contents("{}") + .unwrap(); + root.join_component("empty-folder") + .create_dir_all() + .unwrap(); + + src.create_dir_all().unwrap(); + src.join_component("index.js") + .create_with_contents("console.log('hello world');") + .unwrap(); + src.join_component("index.test.js") + .create_with_contents("") + .unwrap(); + + let global_file_dependencies = vec!["**".to_string()]; + let results = + collect_global_deps(&PackageManager::Berry, &root, &global_file_dependencies).unwrap(); + + // should not yield the root folder itself, src, or empty-folder + assert_eq!(results.len(), 3, "{:?}", results); + } }