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

Step.WriteFile: Iterate directories when adding data to cache hash #20573

Conversation

der-teufel-programming
Copy link
Contributor

Fixes #20571

Comment on lines +286 to +295
for (dir.options.exclude_extensions) |ext| {
if (std.mem.endsWith(u8, entry.path, ext)) continue :next_entry;
}
if (dir.options.include_extensions) |incs| {
for (incs) |inc| {
if (std.mem.endsWith(u8, entry.path, inc)) break;
} else {
continue :next_entry;
}
}
Copy link
Contributor

@rohlem rohlem Jul 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic seems duplicated in fn make.
We could have a shared function Directory.Options.isFilePathExcluded(options: Directory.Options, file_path: []const u8) bool -> if(dir.options.isFilePathExcluded(entry)) continue; to make sure they stay in-sync.

@rohlem
Copy link
Contributor

rohlem commented Jul 10, 2024

Not directly related to this change, but I was surprised that we simply use mem.endsWith for the include-/exclude_extensions, so f.e. *.doc would also match extension c.
The doc comments correctly describe them as suffixes instead of extensions; I think it would be clearer to rename the fields to include-/exclude_suffixes.
(I can open a separate issue for discussion, or the maintainers can just directly accept/reject that idea here.)

@andrewrk
Copy link
Member

andrewrk commented Jul 10, 2024

@rohlem I agree that you have identified a better name, but I also think that the endsWith behavior is a more useful abstraction, so I think your proposed rename is the best solution.

edit: changed my mind. I think the word "extension" is perfectly adequate.

@andrewrk andrewrk self-requested a review July 10, 2024 21:24
@andrewrk
Copy link
Member

Thank you, it was a good effort but ultimately an incorrect implementation, and I solved it myself in fb3a441 in the "watch" branch, while adding file system watching integration at the same time.

@andrewrk andrewrk closed this Jul 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

std.Build.Step.WriteFile does not implement caching correctly for directories
3 participants