Skip to content

Conversation

Sahnvour
Copy link
Contributor

Closes #14940

Copy link
Member

@andrewrk andrewrk left a comment

Choose a reason for hiding this comment

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

Thanks for working on this!

Don't forget that part of the acceptance criteria is

make the zig build subcommand always supply a random seed argument.

@Sahnvour
Copy link
Contributor Author

Sahnvour commented Aug 14, 2023

Don't forget that part of the acceptance criteria is

make the zig build subcommand always supply a random seed argument.

That's what

zig/lib/std/Build.zig

Lines 271 to 273 in 615372a

.seed = fmt_lib.allocPrint(self.allocator, "{x}", .{
@as(u64, @bitCast(std.time.microTimestamp())),
}) catch @panic("OOM"),

is about, am I missing something ?

@andrewrk
Copy link
Member

andrewrk commented Aug 14, 2023

am I missing something ?

Yeah- specifically the Zig CLI should choose the seed, so that when the build fails, the command to reproduce the issue includes the --seed argument.

In src/main.zig, in the cmdBuild function.

std.debug.print("Expected u32 after {s}\n\n", .{arg});
usageAndErr(builder, false, stderr_stream);
};
seed = std.fmt.parseUnsigned(u32, next_arg, 10) catch |err| {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
seed = std.fmt.parseUnsigned(u32, next_arg, 10) catch |err| {
seed = std.fmt.parseUnsigned(u32, next_arg, 0) catch |err| {

This will allow the user to use hexadecimal notation and a few others supported by parseUnsigned.

};
seed = std.fmt.parseUnsigned(u32, next_arg, 10) catch |err| {
std.debug.print("unable to parse seed '{s}' as u32: {s}", .{ next_arg, @errorName(err) });
process.exit(1);
Copy link
Member

Choose a reason for hiding this comment

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

Please follow the pattern of error handling above and below for failure to parse user arguments. If you want to change all of them to not print the usage, please do that in a separate PR.

src/main.zig Outdated
Comment on lines 4371 to 4372
const micros: u32 = @truncate(@as(u64, @bitCast(std.time.microTimestamp())));
var seed: []const u8 = try std.fmt.allocPrint(arena, "{}", .{micros});
Copy link
Member

@andrewrk andrewrk Aug 17, 2023

Choose a reason for hiding this comment

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

Please use std.crypto.random.int(u32).

Sahnvour and others added 2 commits October 19, 2023 14:24
help detect possibly hidden dependencies on the running order of steps,
especially in -j1 mode
* support 0x prefixed hex code for CLI seed arguments
* don't change the build summary; the printed CLI on build runner
  failure is sufficient
* use `std.crypto.random` instead of system time for entropy
@andrewrk andrewrk merged commit 7de893c into ziglang:master Oct 20, 2023
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.

accept a seed option to the build runner and shuffle the dependency order using it
3 participants