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.

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

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

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

Comments

@andrewrk
Copy link
Member

andrewrk commented Jul 10, 2024

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.

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 added a commit that referenced this issue Jul 10, 2024
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
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
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
igor84 pushed a commit to igor84/zig that referenced this issue Aug 11, 2024
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
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
Development

Successfully merging a pull request may close this issue.

2 participants