Zig 0.16.0#18
Conversation
the main breakage here is caused by writergate. there's some weirdness going on to do with file paths and the build system too though, so that should be dealt with before merging. also the new writer and reader don't provide any error specificity, so we don't know why a read or a write failed now. this should also be considered: perhaps creating a custom interface is warranted?
we set the cwd of dimmer to the script directory such that the run step passes the correct relative paths. this caused weirdness in zig's dependency file handling - zig expects relative paths to be relative to the project root instead of the cwd of the run step. changing paths to be absolute is a suitable workaround.
|
windows compilation failure is due to https://codeberg.org/ziglang/translate-c/issues/327 |
|
Hi! Really happy with this, thank you for your work on it, and sorry that I didn't make further progress on my PR :) My only little nitpick is when you've continued from my PR, you ended up I think re-breaking the GPT partitioning? If you have a config which doesn't specify the starting LBA, then you get: This is an unfortunate off-by-one error that was introduced in #12. I've been upgrading my kernel to 0.16, so that's how I've just encountered it. As I was looking into it, I realised that I encountered this before and subsequently forgotten that I fixed it in my fork: It'd be really great if you could re-include that line in your PR :) Or do you think actually it'd be best just to have that in a separate PR? Thanks again ^_^ |
|
it would be good to add that yeah. i havent had time or energy to work on this for a while though, so no ETA unless a maintainer wants to add that change in |
|
Since I'm unsure weather will be doing a 0.16 version or jumping straight to 0.17 I don't know if the fix will be added here or if I'll use those two branches as a base for a big 0.17 update, especially since 0.17-dev has some translate-c bugs fixed that are the reason for windows compilation failure |
|
I think it'd be nice at least to have a working version for 0.16 merged if thats ok? Then work for 0.17.0/master can be built on this in a separate branch, and other users will have an easier time finding a working version. I'm happy to work on @Khitiara's branch if fae would be happy to give me push access, or alternatively I could open my own PR :) |
|
sent an invite to my fork, should be able to push to this pr when accepted @bnuuydev |
|
@ikskuh this is ready to merge pending your review |
ikskuh
left a comment
ikskuh
left a comment
There was a problem hiding this comment.
I already have change requests, will do the BuildInterface review after these issues are addressed
| std.log.warn("cannot copy file {!s}: {s} is not a supported file type!", .{ | ||
| entry.dir.realpath(entry.path, &realpath_buffer), | ||
| std.log.warn("cannot copy file {s}: {s} is not a supported file type!", .{ | ||
| if (entry.dir.realPathFile(io, entry.path, &realpath_buffer)) |l| realpath_buffer[0..l] else |e| @errorName(e), |
There was a problem hiding this comment.
Instead of printing errorName(), just print entry.path as this is a user information. This information isn't helpful when we print "NotADir" instead of "./foo/bar/bam.h". The relative path is still more valuable than a raw error description
| } | ||
|
|
||
| fn disk_read(intf: *fatfs.Disk, buff: [*]u8, sector: fatfs.LBA, count: c_uint) fatfs.Disk.Error!void { | ||
| fn disk_read(intf: *fatfs.Disk, io: std.Io, buff: [*]u8, sector: fatfs.LBA, count: c_uint) fatfs.Disk.Error!void { |
There was a problem hiding this comment.
This looks wrong. Why does fatfs.Disk require std.Io?
The fatfs implementation must not use std.Io, as we can store the Io in the BinaryStreamDisk object.
| b.release_mode = .fast; | ||
| const optimize = b.standardOptimizeOption(.{}); |
There was a problem hiding this comment.
this correctly sets the default optimize mode in a way that still allows overriding - using fast instead of safe to work around the translate-c bug in 0.16.0
| partitions: []Partition, | ||
|
|
||
| pub fn parse(ctx: dim.Context) !dim.Content { | ||
| pub fn parse(ctx: dim.Context, stdio: std.Io) !dim.Content { |
There was a problem hiding this comment.
Instead of passing std.Io through a parameter, we can also pass it through the dim.Context, which is already a fusion of all parameters typically passed around.
It makes the code less dependend and infective of std.Io, and makes it easier to review
There was a problem hiding this comment.
The whole diff should be way slimmer then, as the io isn't required in a lot of places, but is currently dragged through every place
| try stream.write(io, block_size * 2 + pe_ofs, &pe_block); | ||
| try stream.write(io, block_size * secondary_pe_array_lba + pe_ofs, &pe_block); |
There was a problem hiding this comment.
The BinaryStream should also encapsulate std.Io, to keep the core of dimmer free of std.Io assumptions (it is intentionally built in a way that it does not drag std as the I/O layer into it too much)
4 participants
This PR disables fuzz tests temporarily because I dont understand the api well enough to update them.