From 6963b376bbb0cff3e9331aaf924e96c1428d3892 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Tue, 10 Sep 2024 20:51:37 -0400 Subject: [PATCH 1/6] Copyedit `create_archive_if_not_on_ci` comments --- tests/tools/src/lib.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/tools/src/lib.rs b/tests/tools/src/lib.rs index 513c06c91bf..8d1a426a5a6 100644 --- a/tests/tools/src/lib.rs +++ b/tests/tools/src/lib.rs @@ -642,12 +642,12 @@ fn is_lfs_pointer_file(path: &Path) -> bool { .map_or(false, |_| buf.starts_with(PREFIX)) } -/// The `script_identity` will be baked into the soon to be created `archive` as it identitifies the script +/// The `script_identity` will be baked into the soon to be created `archive` as it identifies the script /// that created the contents of `source_dir`. fn create_archive_if_not_on_ci(source_dir: &Path, archive: &Path, script_identity: u32) -> std::io::Result<()> { - // on windows, we fail to remove the meta_dir and can't do anything about it, which means tests will see more - // in the directory than they should which makes them fail. It's probably a bad idea to generate archives on windows - // anyway. Either unix is portable OR no archive is created anywhere. This also means that windows users can't create + // On Windows, we fail to remove the meta_dir and can't do anything about it, which means tests will see more + // in the directory than they should which makes them fail. It's probably a bad idea to generate archives on Windows + // anyway. Either Unix is portable OR no archive is created anywhere. This also means that Windows users can't create // archives, but that's not a deal-breaker. if cfg!(windows) || is_ci::cached() || is_excluded(archive) { return Ok(()); From 8b51b3a0cb1edafc7b041f1037b3dfab4cdc3640 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Tue, 10 Sep 2024 20:52:27 -0400 Subject: [PATCH 2/6] Rename create_archive_if_{not_on_ci -> we_should} This renames the private funciton `create_archive_if_not_on_ci` in gix-testtols to `create_archive_if_we_should`, because it already checks for much more than whether we are on CI to decide whether or not to create the archive. This archive is also (already) not created if we are running on Windows, if the archive destination is excluded by being ignored in any `.gitignore`, or if Git LFS is in use and the archive is an LFS pointer file. --- tests/tools/src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/tools/src/lib.rs b/tests/tools/src/lib.rs index 8d1a426a5a6..92b013be842 100644 --- a/tests/tools/src/lib.rs +++ b/tests/tools/src/lib.rs @@ -569,7 +569,7 @@ fn scripted_fixture_read_only_with_args_inner( output.stdout.as_bstr(), output.stderr.as_bstr() ); - create_archive_if_not_on_ci( + create_archive_if_we_should( &script_result_directory, &archive_file_path, script_identity_for_archive, @@ -644,7 +644,7 @@ fn is_lfs_pointer_file(path: &Path) -> bool { /// The `script_identity` will be baked into the soon to be created `archive` as it identifies the script /// that created the contents of `source_dir`. -fn create_archive_if_not_on_ci(source_dir: &Path, archive: &Path, script_identity: u32) -> std::io::Result<()> { +fn create_archive_if_we_should(source_dir: &Path, archive: &Path, script_identity: u32) -> std::io::Result<()> { // On Windows, we fail to remove the meta_dir and can't do anything about it, which means tests will see more // in the directory than they should which makes them fail. It's probably a bad idea to generate archives on Windows // anyway. Either Unix is portable OR no archive is created anywhere. This also means that Windows users can't create From 03d5a6873e1b6d44227f709e91ac447e29abfac3 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Tue, 10 Sep 2024 21:05:43 -0400 Subject: [PATCH 3/6] feat: Recognize `GIX_TEST_CREATE_ARCHIVES_EVEN_ON_CI` When `gix-testtools` runs fixtures, it now recognizes a new environment variable, `GIX_TEST_CREATE_ARCHIVES_EVEN_ON_CI`, specifying that archives should be generated even when on CI. By default, they are still not generated when on CI. It may make sense to enable this: - If automatically testing archive creation, or - As a way to check that all intended generated arhives are committed (which is the motivating use case for this feature), or - If actually using CI to generate archives that will be uploaded as artifacts, or - In unusual non-CI environments that are mis-detected as CI (though that should usually be investigated and fixed, since some software performs destructive operations more readily without interactive checks when CI is detected). The usual reason for not generating archives on CI is that they would not typically be preserved. Thus refraining from generating them on CI remains the default behavior. Like the `GIX_TEST_IGNORE_ARCHIVES` environment variable, the new variable `GIX_TEST_CREATE_ARCHIVES_EVEN_ON_CI` is currently interpreted as "true" based solely on its presence. This is to say that is actual value is currently not examined. --- tests/tools/src/lib.rs | 43 +++++++++++++++++++++++------------------- 1 file changed, 24 insertions(+), 19 deletions(-) diff --git a/tests/tools/src/lib.rs b/tests/tools/src/lib.rs index 92b013be842..6d8f04b3ba3 100644 --- a/tests/tools/src/lib.rs +++ b/tests/tools/src/lib.rs @@ -10,6 +10,7 @@ use std::{ collections::BTreeMap, + env, ffi::OsString, io::Read, path::{Path, PathBuf}, @@ -150,8 +151,8 @@ fn git_version_from_bytes(bytes: &[u8]) -> Result<(u8, u8, u8)> { /// Set the current working dir to `new_cwd` and return a type that returns to the previous working dir on drop. pub fn set_current_dir(new_cwd: impl AsRef) -> std::io::Result { - let cwd = std::env::current_dir()?; - std::env::set_current_dir(new_cwd)?; + let cwd = env::current_dir()?; + env::set_current_dir(new_cwd)?; Ok(AutoRevertToPreviousCWD(cwd)) } @@ -166,7 +167,7 @@ pub struct AutoRevertToPreviousCWD(PathBuf); impl Drop for AutoRevertToPreviousCWD { fn drop(&mut self) { - std::env::set_current_dir(&self.0).unwrap(); + env::set_current_dir(&self.0).unwrap(); } } @@ -461,7 +462,7 @@ fn scripted_fixture_read_only_with_args_inner( crc_digest.update(&std::fs::read(&script_path).unwrap_or_else(|err| { panic!( "file {script_path:?} in CWD {:?} could not be read: {err}", - std::env::current_dir().expect("valid cwd"), + env::current_dir().expect("valid cwd"), ) })); for arg in &args { @@ -548,7 +549,7 @@ fn scripted_fixture_read_only_with_args_inner( script_location.display() ); } - let script_absolute_path = std::env::current_dir()?.join(script_path); + let script_absolute_path = env::current_dir()?.join(script_path); let mut cmd = std::process::Command::new(&script_absolute_path); let output = match configure_command(&mut cmd, &args, &script_result_directory).output() { Ok(out) => out, @@ -593,7 +594,7 @@ fn configure_command<'a>( args: &[String], script_result_directory: &Path, ) -> &'a mut std::process::Command { - let mut msys_for_git_bash_on_windows = std::env::var_os("MSYS").unwrap_or_default(); + let mut msys_for_git_bash_on_windows = env::var_os("MSYS").unwrap_or_default(); msys_for_git_bash_on_windows.push(" winsymlinks:nativestrict"); cmd.args(args) .stdout(std::process::Stdio::piped()) @@ -632,6 +633,14 @@ fn write_failure_marker(failure_marker: &Path) { std::fs::write(failure_marker, []).ok(); } +fn should_skip_all_archive_creation() -> bool { + // On Windows, we fail to remove the meta_dir and can't do anything about it, which means tests will see more + // in the directory than they should which makes them fail. It's probably a bad idea to generate archives on Windows + // anyway. Either Unix is portable OR no archive is created anywhere. This also means that Windows users can't create + // archives, but that's not a deal-breaker. + cfg!(windows) || (is_ci::cached() && !env::var_os("GIX_TEST_CREATE_ARCHIVES_EVEN_ON_CI").is_some()) +} + fn is_lfs_pointer_file(path: &Path) -> bool { const PREFIX: &[u8] = b"version https://gix-lfs"; let mut buf = [0_u8; PREFIX.len()]; @@ -645,11 +654,7 @@ fn is_lfs_pointer_file(path: &Path) -> bool { /// The `script_identity` will be baked into the soon to be created `archive` as it identifies the script /// that created the contents of `source_dir`. fn create_archive_if_we_should(source_dir: &Path, archive: &Path, script_identity: u32) -> std::io::Result<()> { - // On Windows, we fail to remove the meta_dir and can't do anything about it, which means tests will see more - // in the directory than they should which makes them fail. It's probably a bad idea to generate archives on Windows - // anyway. Either Unix is portable OR no archive is created anywhere. This also means that Windows users can't create - // archives, but that's not a deal-breaker. - if cfg!(windows) || is_ci::cached() || is_excluded(archive) { + if should_skip_all_archive_creation() || is_excluded(archive) { return Ok(()); } if is_lfs_pointer_file(archive) { @@ -702,7 +707,7 @@ fn is_excluded(archive: &Path) -> bool { let mut lut = EXCLUDE_LUT.lock(); lut.as_mut() .and_then(|cache| { - let archive = std::env::current_dir().ok()?.join(archive); + let archive = env::current_dir().ok()?.join(archive); let relative_path = archive.strip_prefix(cache.base()).ok()?; cache .at_path( @@ -746,7 +751,7 @@ fn extract_archive( let mut buf = Vec::new(); #[cfg_attr(feature = "xz", allow(unused_mut))] let mut input_archive = std::fs::File::open(archive)?; - if std::env::var_os("GIX_TEST_IGNORE_ARCHIVES").is_some() { + if env::var_os("GIX_TEST_IGNORE_ARCHIVES").is_some() { return Err(std::io::Error::new( std::io::ErrorKind::Other, format!( @@ -848,16 +853,16 @@ impl<'a> Env<'a> { /// Set `var` to `value`. pub fn set(mut self, var: &'a str, value: impl Into) -> Self { - let prev = std::env::var_os(var); - std::env::set_var(var, value.into()); + let prev = env::var_os(var); + env::set_var(var, value.into()); self.altered_vars.push((var, prev)); self } /// Unset `var`. pub fn unset(mut self, var: &'a str) -> Self { - let prev = std::env::var_os(var); - std::env::remove_var(var); + let prev = env::var_os(var); + env::remove_var(var); self.altered_vars.push((var, prev)); self } @@ -867,8 +872,8 @@ impl<'a> Drop for Env<'a> { fn drop(&mut self) { for (var, prev_value) in self.altered_vars.iter().rev() { match prev_value { - Some(value) => std::env::set_var(var, value), - None => std::env::remove_var(var), + Some(value) => env::set_var(var, value), + None => env::remove_var(var), } } } From 94c6d704ae216b12f7132c17876e0526097e86e6 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Tue, 10 Sep 2024 21:33:21 -0400 Subject: [PATCH 4/6] Thanks clippy --- tests/tools/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/tools/src/lib.rs b/tests/tools/src/lib.rs index 6d8f04b3ba3..731bfcb0e05 100644 --- a/tests/tools/src/lib.rs +++ b/tests/tools/src/lib.rs @@ -638,7 +638,7 @@ fn should_skip_all_archive_creation() -> bool { // in the directory than they should which makes them fail. It's probably a bad idea to generate archives on Windows // anyway. Either Unix is portable OR no archive is created anywhere. This also means that Windows users can't create // archives, but that's not a deal-breaker. - cfg!(windows) || (is_ci::cached() && !env::var_os("GIX_TEST_CREATE_ARCHIVES_EVEN_ON_CI").is_some()) + cfg!(windows) || (is_ci::cached() && env::var_os("GIX_TEST_CREATE_ARCHIVES_EVEN_ON_CI").is_none()) } fn is_lfs_pointer_file(path: &Path) -> bool { From de95f2474a4097ca855428d68f782f88cd464ea0 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Tue, 10 Sep 2024 17:03:13 -0400 Subject: [PATCH 5/6] Check for uncommitted unignored generated archives on CI This adds a step to `test-fast` to run `git diff` and fail if there are any changes to any tracked files, and sets the newly recognized `GIX_TEST_CREATE_ARCHIVES_EVEN_ON_CI` environment variable for the `nextest` step in which fixture scripts are run. Any changes would indicate a problem that should be addressed, but the main goal here is to catch when a regenerated archive has not been committed. Because adding archives to `.gitignore` would also cause this to pass but is far less often the appropriate solution, this change includes a comment about what the best solution *usually* is. This is added to the `test-fast` jobs but not to `test` job because the `test` job uses `GIT_TEST_IGNORE_ARCHIVES` to prevent already existing archives from being used, which will nearly always result in at least slightly different archives being generated. So in the `test` job this would, in practice, always give a false-positive failure. Because archive generation is (at least currently) suppressed on Windows, this step should rarely if ever fail on Windows even when the problem it is looking for is present. But if it does fail on Windows, then there is a problem that ought to be investigated, and if the problem it is looking for is present, then it will still fail on the other systems, unless the problem is specific to Windows. Running this step on both Ubuntu and macOS (and Windows), rather than just Ubuntu (and Windows), has the benefit of catching some kinds of skew. It is also somewhat simpler than setting it only for some operating systems. --- .github/workflows/ci.yml | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 7a2ec341d5d..f7c8aff94f9 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -92,9 +92,13 @@ jobs: with: tool: nextest - name: "Test (nextest)" + env: + GIX_TEST_CREATE_ARCHIVES_EVEN_ON_CI: 1 run: cargo nextest run --all --no-fail-fast - name: Doctest run: cargo test --doc + - name: Check that tracked archives are up to date + run: git diff # If this fails, the fix is usually to commit a regenerated archive. test-32bit: runs-on: ubuntu-latest From 4150af45f1265b0f4a6b455d8d84668b66e4dfb3 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Tue, 10 Sep 2024 21:35:27 -0400 Subject: [PATCH 6/6] Fix CI `git diff` command with `--exit-code` So it actually returns a failing exit status if there were changes. --- .github/workflows/ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f7c8aff94f9..6f3546f2aca 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -98,7 +98,7 @@ jobs: - name: Doctest run: cargo test --doc - name: Check that tracked archives are up to date - run: git diff # If this fails, the fix is usually to commit a regenerated archive. + run: git diff --exit-code # If this fails, the fix is usually to commit a regenerated archive. test-32bit: runs-on: ubuntu-latest