-
-
Notifications
You must be signed in to change notification settings - Fork 367
Cache #7631
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
Cache #7631
Conversation
smores56
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good start, excited to see caching of build artifacts for the first time since Roc was created!
src/cache.zig
Outdated
| return try getBaseRocCacheFolder(allocator, xdg_cache_home, sub_path); | ||
| } | ||
|
|
||
| fn getXdgCacheHome(allocator: Allocator) ?[]u8 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On Unix, this should be used first when looking for the cache root, then $HOME/.cache/roc after that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made this a separate function because I didn't want the tests to break because someone set XDG_CACHE_HOME. I wanted to mock it out, but it's probably making the code too confusing so I'll refactor.
I'm sure we can figure out something better to solve the problem.
src/cache.zig
Outdated
| const roc_cache_folder = std.fs.path.dirname(file_cache_path); | ||
| if (roc_cache_folder) |x| { | ||
| cwd.makePath(x) catch { | ||
| return SaveCacheFileError.FailedToMakeCacheFolder; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very minor nitpick: I think generally Zig uses error.NameOfError when creating an error variant from a custom error union.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added it so that I can pass a callback function for saving to the file system in saveToCacheInternal. I wanted to add the callback so that I can mock it out during testing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think I communicated well here, I meant I more see error.FailedToMakeCacheFolder instead of SaveCacheFileError.FailedToMakeCacheFolder
src/cache.zig
Outdated
| packages_build, | ||
| }; | ||
|
|
||
| pub const RocCacheFile = union(RocCacheFileTag) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two naming things:
- Roc is implied throughout this whole compiler, so no need to prefix names with it except in cases of ambiguity
- Zig prefers to nest related "has-a" data like the above
CompilerCacheFile: https://ziggit.dev/t/nesting-struct-types-or-not/6750
An example of how I might nest it would be:
pub const CacheFile = union(enum) {
compiler: Compiler,
build,
packages_src,
packages_build,
pub const Compiler = struct {
version_name: []const u8,
binary: []const u8,
};
};|
@jfkonecn can you interactive rebase to either squash the commits or at least give them minorly meaningful names? A bunch of commits just with the name |
|
Yeah, I was going to do that when I'm ready for an official review, but if you prefer I do that now I can do that as well. |
Doesn't matter when. Just was noting in general. |
|
@smores56 this is a nitpick, but for |
|
You're right, I was trying to write "Roc" code in spite of Zig standard patterns. That's been corrected in #7657 |
12420b2 to
5f0391b
Compare
|
Addressed with #7759 |
closes #7517
Still a work in progress