Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

std.Build.Step.WriteFile does not implement caching correctly for directories #20571

Closed
andrewrk opened this issue Jul 10, 2024 · 1 comment
Closed

std.Build.Step.WriteFile does not implement caching correctly for directories #20571

andrewrk opened this issue Jul 10, 2024 · 1 comment
Labels
bug Observed behavior contradicts documented or intended behavior zig build system std.Build, the build runner, `zig build` subcommand, package management
Milestone
0.14.0

Comments

@andrewrk
Copy link
Member

andrewrk commented Jul 10, 2024 •
edited
Loading

Zig Version

0.14.0-dev.208 854e86c56

Steps to Reproduce and Observed Behavior

zig init

Add this code to build.zig:

    const wf = b.addWriteFiles();
    b.installDirectory(.{
        .source_dir = wf.addCopyDirectory(b.path("stuff"), "stuff", .{}),
        .install_dir = .prefix,
        .install_subdir = "",
    });
$ mkdir stuff
$ touch stuff/1
$ zig build
$ ls zig-out/
1  bin  lib
$ touch stuff/2
$ zig build
$ ls zig-out
1  bin  lib

Expected Behavior

When running zig build the second time, it should notice that a second file exists (2) and copy it to the WriteFile output directory, and then copy it to the installation prefix.

Diagnosis

This happens because the implementation only adds the directory name to the cache hash, instead of marking the file and directory itself as part of the cache input.

zig/lib/std/Build/Step/WriteFile.zig

Lines 271 to 276 in b3b923e

for (write_file.directories.items) |dir| {
man.hash.addBytes(dir.source.getPath2(b, step));
man.hash.addBytes(dir.sub_path);
for (dir.options.exclude_extensions) |ext| man.hash.addBytes(ext);
if (dir.options.include_extensions) |incs| for (incs) |inc| man.hash.addBytes(inc);
}

This is insufficient and results in false positive cache hits. False positives in the cache system are not allowed; this must be fixed to work perfectly or the feature reverted.
@andrewrk andrewrk added bug Observed behavior contradicts documented or intended behavior zig build system std.Build, the build runner, `zig build` subcommand, package management labels Jul 10, 2024
@andrewrk andrewrk added this to the 0.14.0 milestone Jul 10, 2024
@andrewrk andrewrk mentioned this issue Jul 10, 2024
Merged
Closed
andrewrk added a commit that referenced this issue Jul 10, 2024
@andrewrk
std.Build.Step.WriteFile: fix handling of directories
fb3a441 
and add file system watching integration.

`addDirectoryWatchInput` now returns a `bool` which helps remind the
caller to
1. call addDirectoryWatchInputFromPath on any derived paths
2. but only if the dependency is not already captured by a step
   dependency edge.

The make function now recursively walks all directories and adds the
found files to the cache hash rather than incorrectly only adding the
directory name to the cache hash.

closes #20571
@bryanjhv
Copy link

I faced this issue today, as addCopyDirectory does not handle changes in files either (as of 0.13.0).

Solving it temporarily by setting options.exclude_extensions to some cache-busting value, like so:

    const wf = b.addWriteFiles();
    b.installDirectory(.{
        .source_dir = wf.addCopyDirectory(b.path("stuff"), "stuff", .{
            .exclude_extensions = &.{
                b.fmt(".tmp.{d}", .{std.time.timestamp()}),
            },
        }),
        .install_dir = .prefix,
        .install_subdir = "",
    });

eric-saintetienne pushed a commit to eric-saintetienne/zig that referenced this issue Jul 16, 2024
@andrewrk @eric-saintetienne
std.Build.Step.WriteFile: fix handling of directories
de15c9f 
and add file system watching integration.

`addDirectoryWatchInput` now returns a `bool` which helps remind the
caller to
1. call addDirectoryWatchInputFromPath on any derived paths
2. but only if the dependency is not already captured by a step
   dependency edge.

The make function now recursively walks all directories and adds the
found files to the cache hash rather than incorrectly only adding the
directory name to the cache hash.

closes ziglang#20571
SammyJames pushed a commit to SammyJames/zig that referenced this issue Aug 7, 2024
@andrewrk @SammyJames
std.Build.Step.WriteFile: fix handling of directories
dd226d8 
and add file system watching integration.

`addDirectoryWatchInput` now returns a `bool` which helps remind the
caller to
1. call addDirectoryWatchInputFromPath on any derived paths
2. but only if the dependency is not already captured by a step
   dependency edge.

The make function now recursively walks all directories and adds the
found files to the cache hash rather than incorrectly only adding the
directory name to the cache hash.

closes ziglang#20571
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Observed behavior contradicts documented or intended behavior zig build system std.Build, the build runner, `zig build` subcommand, package management
Projects
None yet
Milestone
0.14.0
Development

Successfully merging a pull request may close this issue.

Step.WriteFile: Iterate directories when adding data to cache hash der-teufel-programming/zig
2 participants
@andrewrk @bryanjhv