fix: moving the preserve mtime test logic to Go for portability (#908)

* Fix: Moving the preserve mtiem test logic to Go for portability

* add tags to disable remote caching, execution and force the test to always re-run

* include docs

* use the runfiles library for windows compatability

* mark the test as manual

* remove duplicate word in comment

* chore: reduce duplication of the long caveats text

---------

Co-authored-by: Alex Eagle <alex@aspect.dev>
This commit is contained in:
Justin Pinkul 2024-09-17 18:25:26 -06:00 committed by GitHub
parent 2f65c8c0c7
commit ad48c0d855
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
9 changed files with 143 additions and 50 deletions

17
docs/copy_directory.md generated
View File

@ -4,6 +4,19 @@ A rule that copies a directory to another place.
The rule uses a precompiled binary to perform the copy, so no shell is required.
## Preserving modification times
`copy_directory` and `copy_to_directory` have a `preserve_mtime` attribute, however
there are two caveats to consider when using this feature:
1. Remote Execution / Caching: These layers will reset the modify time and are
incompatible with this feature. To avoid these failures the [no-remote tag](https://bazel.build/reference/be/common-definitions)
can be added.
2. Caching: Changes to only the modified time will not re-trigger cached actions. This can
be worked around by using a clean build when these types of changes occur. For tests the
[external tag](https://bazel.build/reference/be/common-definitions) can be used but this
will result in tests never being cached.
<a id="copy_directory"></a>
## copy_directory
@ -62,7 +75,7 @@ within other rule implementations.
| <a id="copy_directory_bin_action-dst"></a>dst | The directory to copy to. Must be a TreeArtifact. | none |
| <a id="copy_directory_bin_action-copy_directory_bin"></a>copy_directory_bin | Copy to directory tool binary. | none |
| <a id="copy_directory_bin_action-hardlink"></a>hardlink | Controls when to use hardlinks to files instead of making copies.<br><br>See copy_directory rule documentation for more details. | `"auto"` |
| <a id="copy_directory_bin_action-verbose"></a>verbose | If true, prints out verbose logs to stdout | `False` |
| <a id="copy_directory_bin_action-preserve_mtime"></a>preserve_mtime | If true, preserve the modified time from the source. | `False` |
| <a id="copy_directory_bin_action-verbose"></a>verbose | print verbose logs to stdout | `False` |
| <a id="copy_directory_bin_action-preserve_mtime"></a>preserve_mtime | preserve the modified time from the source. See the caveats above about interactions with remote execution and caching. | `False` |

View File

@ -57,7 +57,7 @@ for more information on supported globbing patterns.
| <a id="copy_to_directory-include_external_repositories"></a>include_external_repositories | List of external repository names (with glob support) to include in the output directory.<br><br>Files from external repositories are only copied into the output directory if the external repository they come from matches one of the external repository patterns specified or if they are in the same external repository as this target.<br><br>When copied from an external repository, the file path in the output directory defaults to the file's path within the external repository. The external repository name is _not_ included in that path.<br><br>For example, the following copies `@external_repo//path/to:file` to `path/to/file` within the output directory.<br><br><pre><code>copy_to_directory(&#10; name = "dir",&#10; include_external_repositories = ["external_*"],&#10; srcs = ["@external_repo//path/to:file"],&#10;)</code></pre><br><br>Files that come from matching external are subject to subsequent filters and transformations to determine if they are copied and what their path in the output directory will be. The external repository name of the file from an external repository is not included in the output directory path and is considered in subsequent filters and transformations.<br><br>Globs are supported (see rule docstring above). | List of strings | optional | `[]` |
| <a id="copy_to_directory-include_srcs_packages"></a>include_srcs_packages | List of Bazel packages (with glob support) to include in output directory.<br><br>Files in srcs are only copied to the output directory if the Bazel package of the file matches one of the patterns specified.<br><br>Forward slashes (`/`) should be used as path separators. A first character of `"."` will be replaced by the target's package path.<br><br>Defaults to `["**"]` which includes sources from all packages.<br><br>Files that have matching Bazel packages are subject to subsequent filters and transformations to determine if they are copied and what their path in the output directory will be.<br><br>Globs are supported (see rule docstring above). | List of strings | optional | `["**"]` |
| <a id="copy_to_directory-include_srcs_patterns"></a>include_srcs_patterns | List of paths (with glob support) to include in output directory.<br><br>Files in srcs are only copied to the output directory if their output directory path, after applying `root_paths`, matches one of the patterns specified.<br><br>Forward slashes (`/`) should be used as path separators.<br><br>Defaults to `["**"]` which includes all sources.<br><br>Files that have matching output directory paths are subject to subsequent filters and transformations to determine if they are copied and what their path in the output directory will be.<br><br>Globs are supported (see rule docstring above). | List of strings | optional | `["**"]` |
| <a id="copy_to_directory-preserve_mtime"></a>preserve_mtime | If True, the last modified time of copied files is preserved. | Boolean | optional | `False` |
| <a id="copy_to_directory-preserve_mtime"></a>preserve_mtime | If True, the last modified time of copied files is preserved. See the [caveats on copy_directory](/docs/copy_directory.md#preserving-modification-times) about interactions with remote execution and caching. | Boolean | optional | `False` |
| <a id="copy_to_directory-replace_prefixes"></a>replace_prefixes | Map of paths prefixes (with glob support) to replace in the output directory path when copying files.<br><br>If the output directory path for a file starts with or fully matches a a key in the dict then the matching portion of the output directory path is replaced with the dict value for that key. The final path segment matched can be a partial match of that segment and only the matching portion will be replaced. If there are multiple keys that match, the longest match wins.<br><br>Forward slashes (`/`) should be used as path separators.<br><br>Replace prefix transformation are the final step in the list of filters and transformations. The final output path of a file being copied into the output directory is determined at this step.<br><br>Globs are supported (see rule docstring above). | <a href="https://bazel.build/rules/lib/dict">Dictionary: String -> String</a> | optional | `{}` |
| <a id="copy_to_directory-root_paths"></a>root_paths | List of paths (with glob support) that are roots in the output directory.<br><br>If any parent directory of a file being copied matches one of the root paths patterns specified, the output directory path will be the path relative to the root path instead of the path relative to the file's workspace. If there are multiple root paths that match, the longest match wins.<br><br>Matching is done on the parent directory of the output file path so a trailing '**' glob patterm will match only up to the last path segment of the dirname and will not include the basename. Only complete path segments are matched. Partial matches on the last segment of the root path are ignored.<br><br>Forward slashes (`/`) should be used as path separators.<br><br>A `"."` value expands to the target's package path (`ctx.label.package`).<br><br>Defaults to `["."]` which results in the output directory path of files in the target's package and and sub-packages are relative to the target's package and files outside of that retain their full workspace relative paths.<br><br>Globs are supported (see rule docstring above). | List of strings | optional | `["."]` |
| <a id="copy_to_directory-verbose"></a>verbose | If true, prints out verbose logs to stdout | Boolean | optional | `False` |

View File

@ -6,6 +6,7 @@ load("@aspect_bazel_lib//lib:expand_template.bzl", "expand_template")
load("@aspect_bazel_lib//lib:jq.bzl", "jq")
load("@aspect_bazel_lib//lib:tar.bzl", "tar")
load("@aspect_bazel_lib//lib:yq.bzl", "yq")
load("@io_bazel_rules_go//go:def.bzl", "go_test")
# Validate that JQ works and resolves its toolchain
jq(
@ -105,6 +106,7 @@ copy_to_directory(
srcs = ["d"],
out = "copy_to_directory_mtime_out",
preserve_mtime = True,
tags = ["no-remote"],
)
copy_directory(
@ -112,26 +114,26 @@ copy_directory(
src = "d",
out = "copy_directory_mtime_out",
preserve_mtime = True,
tags = ["no-remote"],
)
sh_test(
go_test(
name = "test_preserve_mtime",
size = "small",
srcs = ["test_preserve_mtime.sh"],
srcs = ["test_preserve_mtime.go"],
deps = [
"@io_bazel_rules_go//go/tools/bazel:go_default_library",
],
data = [
"d",
":copy_directory_mtime_case",
":copy_to_directory_mtime_case",
],
# FIXME(#898)
# On macos, fails with
# stat: illegal option -- -
# usage: stat [-FLnq] [-f format | -l | -r | -s | -x] [-t timefmt] [file ...]
# On windows, fails with
# stat: cannot stat 'd/1': No such file or directory
# On Linux, it's helplessly flaky, often failing in 2/2 attempts like
# Preserve mtime test failed. Modify times do not match for d/1 and copy_to_directory_mtime_out/d/1
# Original modify time: 2024-08-14 18:23:01.413642851 +0000
# Copied modify time: 2024-08-14 18:23:31.501819589 +0000
tags = ["manual"],
# Note: This test is marked as manual as it is not reliable in the CI environment. The reason for is that changes
# to only the modify time do not invalidate the Bazel cache. These failures can be reproduced by:
# bazel clean --expunge
# bazel test :test_preserve_mtime # This test should pass
# touch d/1 # Update the mtime, in the CI environment this is done with the SCM integration.
# bazel test :test_preserve_mtime # This test now fails
tags = ["no-remote", "external", "manual"],
)

View File

@ -5,3 +5,4 @@ local_path_override(
)
bazel_dep(name = "bazel_skylib", version = "1.7.1", dev_dependency = True)
bazel_dep(name = "rules_go", version = "0.46.0", repo_name = "io_bazel_rules_go", dev_dependency = True)

View File

@ -0,0 +1,90 @@
package smoketest
import (
"os"
"path/filepath"
"testing"
"time"
"github.com/bazelbuild/rules_go/go/tools/bazel"
)
type runfilePath struct {
// runfileDir is the directory that was provided as the bazel data dep.
runfileDir string
// subPaths to look for inside of the bazel tracked directory.
subPaths []string
}
func mtime(path string) (time.Time, error) {
info, err := os.Stat(path)
if err != nil {
return time.Time{}, err
}
return info.ModTime(), nil
}
func (r runfilePath) osPath() (string, error) {
dirPath, err := bazel.Runfile(r.runfileDir)
if err != nil{
return "", err
}
parts := append([]string{dirPath}, r.subPaths...)
return filepath.Join(parts...), nil
}
func TestPreserveMTime(t *testing.T) {
cases := map[string]struct{
original runfilePath
copied runfilePath
}{
"copy_directory": {
original: runfilePath{
runfileDir: "d",
subPaths: []string{"1"},
},
copied: runfilePath{
runfileDir: "copy_directory_mtime_out",
subPaths: []string{"1"},
},
},
"copy_to_directory": {
original: runfilePath{
runfileDir: "d",
subPaths: []string{"1"},
},
copied: runfilePath{
runfileDir: "copy_to_directory_mtime_out",
subPaths: []string{"d", "1"},
},
},
}
for name, test := range cases {
t.Run(name, func(t *testing.T) {
originalPath, err := test.original.osPath()
if err != nil {
t.Fatal(err.Error())
}
originalMTime, err := mtime(originalPath)
if err != nil {
t.Fatal(err.Error())
}
copiedPath, err := test.copied.osPath()
if err != nil {
t.Fatal(err.Error())
}
copiedMTime, err := mtime(copiedPath)
if err != nil {
t.Fatal(err.Error())
}
if originalMTime != copiedMTime {
t.Fatalf(`Modify times do not match for %s and %s:
Original modify time: %s
Copied modify time: %s`, originalPath, copiedPath, originalMTime, copiedMTime)
}
})
}
}

View File

@ -1,30 +0,0 @@
#!/usr/bin/env bash
set -euo pipefail
function main {
compareMTimes d/1 copy_to_directory_mtime_out/d/1
compareMTimes d/1 copy_directory_mtime_out/1
}
function compareMTimes {
local originalFile="$1"
local copiedFile="$2"
local mtimeOriginal
mtimeOriginal="$(stat --dereference --format=%y "$originalFile")"
local mtimeCopy
mtimeCopy="$(stat --dereference --format=%y "$copiedFile")"
if [[ "$mtimeOriginal" != "$mtimeCopy" ]]; then
echo "Preserve mtime test failed. Modify times do not match for $originalFile and $copiedFile"
echo " Original modify time: $mtimeOriginal"
echo " Copied modify time: $mtimeCopy"
return 1
fi
echo "Preserve mtime test passed for $originalFile and $copiedFile"
}
main "$@"

View File

@ -1,6 +1,19 @@
"""A rule that copies a directory to another place.
The rule uses a precompiled binary to perform the copy, so no shell is required.
## Preserving modification times
`copy_directory` and `copy_to_directory` have a `preserve_mtime` attribute, however
there are two caveats to consider when using this feature:
1. Remote Execution / Caching: These layers will reset the modify time and are
incompatible with this feature. To avoid these failures the [no-remote tag](https://bazel.build/reference/be/common-definitions)
can be added.
2. Caching: Changes to only the modified time will not re-trigger cached actions. This can
be worked around by using a clean build when these types of changes occur. For tests the
[external tag](https://bazel.build/reference/be/common-definitions) can be used but this
will result in tests never being cached.
"""
load(

View File

@ -34,9 +34,11 @@ def copy_directory_bin_action(
See copy_directory rule documentation for more details.
verbose: If true, prints out verbose logs to stdout
verbose: print verbose logs to stdout
preserve_mtime: preserve the modified time from the source.
See the caveats above about interactions with remote execution and caching.
preserve_mtime: If true, preserve the modified time from the source.
"""
args = [
src.path,
@ -100,7 +102,7 @@ _copy_directory = rule(
),
"verbose": attr.bool(),
"preserve_mtime": attr.bool(
doc = "If True, the last modified time of copied files is preserved.",
doc = """If True, the last modified time of copied files is preserved. Note the caveats on copy_directory.""",
default = False,
),
# use '_tool' attribute for development only; do not commit with this attribute active since it

View File

@ -205,7 +205,9 @@ removed from sources files.
- `off`: all files are copied
- `on`: hardlinks are used for all files (not recommended)
""",
"preserve_mtime": """If True, the last modified time of copied files is preserved.""",
"preserve_mtime": """If True, the last modified time of copied files is preserved.
See the [caveats on copy_directory](/docs/copy_directory.md#preserving-modification-times)
about interactions with remote execution and caching.""",
# verbose
"verbose": """If true, prints out verbose logs to stdout""",
}