Compare user input for multiple dependency build variants

[mattnite]

If I try to build a dependency with different arguments today, only one version of that dependency is built, when it should build every variant. Here's a simplified example, I've imported the echo dependency twice, but in each scenario I've specified the dependency with different optimization modes.

// build.zigconstdep_variant_1=b.dependency("echo", .{
.target=target,
.optimize=.Debug,
});
constdep_variant_2=b.dependency("echo", .{
.target=target,
.optimize=.ReleaseSafe,
});

I'll now write a program to print the value returned by get_optimization() in the echo static library of the echo dependency, and compile it twice, each using one of the dependency variants:

// build.zig continued...constprogram_variant_1=b.addExecutable(.{
.name="i_should_print_debug",
.source_file= .{ .path="src/main.zig" },
});
program_variant_1.addModule(dep_variant_1.module("echo"));
b.installArtifact(program_variant_1);
constprogram_variant_2=b.addExecutable(.{
.name="i_should_print_release_safe",
.source_file= .{ .path="src/main.zig" },
});
program_variant_2.addModule(dep_variant_2.module("echo"));
b.installArtifact(program_variant_2);

However when I run both programs:

mattnite@Matts-MacBook-Pro ~/c/dependency-variants (main)> ./zig-out/bin/i_should_print_debug
Debug
mattnite@Matts-MacBook-Pro ~/c/dependency-variants (main)> ./zig-out/bin/i_should_print_release_safe
Debug

:(

Alright Andrew, I'll bite

walking down the b.dependency() call stack there's a shiny lil todo:

if (b.initialized_deps.get(build_root_string)) |dep| {
// TODO: check args are the samereturndep;
}

To fill in this TODO, the args of different Builds of the same dependency need to be compared. This patch generates a Build's user_input_options here for comparison with existing dependencies. If it's unique then the options are passed down to the newly created Build.

The other piece of work, and related TODO, was generating the hash for the install prefix. Which now looks like this:

varhash=b.cache.hash;
// Random bytes to make unique. Refresh this with new random bytes when// implementation is modified in a non-backwards-compatible way.hash.add(@as(u32, 0xd8cb0055));
hash.addBytes(b.dep_prefix);
hashUserInputOptionsMap(b.allocator, b.user_input_options, &hash);
// TODO additionally update the hash with `args`.constdigest=hash.final();
constinstall_prefix=tryb.cache_root.join(b.allocator, &.{ "i", &digest });
b.resolveInstallPrefix(install_prefix, .{});

AFAIK, it's possible for entries in aUserInputOptionsMap to have different orders depending on order of insertion. hashUserInputOptionsmap orders all values recursively -- TIL there could be maps in the user input option -- and then hashes everything.

Note anyone reviewing please take a look at the hashing of different user values like flag, it seems to me that just hashing the name would be good enough? I'm also not sure if used should be included in hashing as well.

I hope you're happy

Now my programs print:

mattnite@Matts-MacBook-Pro ~/c/dependency-variants (main)> ./zig-out/bin/i_should_print_debug
Debug
mattnite@Matts-MacBook-Pro ~/c/dependency-variants (main)> ./zig-out/bin/i_should_print_release_safe
ReleaseSafe

The repos to reproduce this are dependency-variants and echo.

[andrewrk]

why insertion sort rather than sortUnstable?

[mattnite]

Tbh it was the first one that came up in code completion. I didn't give this choice much thought because I doubt we'll see N approach even 1000.

[andrewrk]

OK please do change it to std.mem.sortUnstable. If you require stable sort, std.mem.sort is the way to go, and the time to use std.sort.insertion is pretty much never.

[mattnite]

done

[andrewrk]

looks like you solved that TODO below :)

[andrewrk]

hmm I think I see a big problem here. This is correctly checking if a value in a hash map can be re-used, but then at the end of the function it will put the new value into the hash map, overwriting the old one. In other words, it does not successfully deduplicate with more complicated set of inputs. If we passed ABA, for example, it would create two A's instead of one A and one B.

I think you already created all the glue code that is needed to solve this; the initialized_deps hash map needs to key on not only the build_root_string but on the user input options as well. That hash map is exclusively used in this function, so you are free to change it to this function's needs.

[mattnite]

Thanks for pointing that out, I completely missed it. I’ve added user input to the key of initialized_deps.

[mitchellh]

Just noting this is blocking my terminal (Ghostty) from adopting the package manager. We build a universal macOS binary as part of the Mac build and this issue means that we build two of the same arches in a row rather than two distinct ones! 😄

Thanks so much @mattnite for fixing this. :)

[ikskuh]

Another use case:
In AshetOS i build a fat library in a way that i can use it for both the host to make an os image as well as embed the library into the OS to actually read that file system.

The os isnt using the package manager yet, so i didnt encounter that problem

[mattnite]

If I try to build a dependency with different arguments today, only one version of that dependency is built, when it should build every variant. Here's a simplified example, I've imported the echo dependency twice, but in each scenario I've specified the dependency with different optimization modes.

// build.zigconstdep_variant_1=b.dependency("echo", .{
.target=target,
.optimize=.Debug,
});
constdep_variant_2=b.dependency("echo", .{
.target=target,
.optimize=.ReleaseSafe,
});

I'll now write a program to print the value returned by get_optimization() in the echo static library of the echo dependency, and compile it twice, each using one of the dependency variants:

// build.zig continued...constprogram_variant_1=b.addExecutable(.{
.name="i_should_print_debug",
.source_file= .{ .path="src/main.zig" },
});
program_variant_1.addModule(dep_variant_1.module("echo"));
b.installArtifact(program_variant_1);
constprogram_variant_2=b.addExecutable(.{
.name="i_should_print_release_safe",
.source_file= .{ .path="src/main.zig" },
});
program_variant_2.addModule(dep_variant_2.module("echo"));
b.installArtifact(program_variant_2);

However when I run both programs:

mattnite@Matts-MacBook-Pro ~/c/dependency-variants (main)> ./zig-out/bin/i_should_print_debug
Debug
mattnite@Matts-MacBook-Pro ~/c/dependency-variants (main)> ./zig-out/bin/i_should_print_release_safe
Debug

:(

Alright Andrew, I'll bite

walking down the b.dependency() call stack there's a shiny lil todo:

if (b.initialized_deps.get(build_root_string)) |dep| {
// TODO: check args are the samereturndep;
}

To fill in this TODO, the args of different Builds of the same dependency need to be compared. This patch generates a Build's user_input_options here for comparison with existing dependencies. If it's unique then the options are passed down to the newly created Build.

The other piece of work, and related TODO, was generating the hash for the install prefix. Which now looks like this:

varhash=b.cache.hash;
// Random bytes to make unique. Refresh this with new random bytes when// implementation is modified in a non-backwards-compatible way.hash.add(@as(u32, 0xd8cb0055));
hash.addBytes(b.dep_prefix);
hashUserInputOptionsMap(b.allocator, b.user_input_options, &hash);
// TODO additionally update the hash with `args`.constdigest=hash.final();
constinstall_prefix=tryb.cache_root.join(b.allocator, &.{ "i", &digest });
b.resolveInstallPrefix(install_prefix, .{});

AFAIK, it's possible for entries in aUserInputOptionsMap to have different orders depending on order of insertion. hashUserInputOptionsmap orders all values recursively -- TIL there could be maps in the user input option -- and then hashes everything.

Note anyone reviewing please take a look at the hashing of different user values like flag, it seems to me that just hashing the name would be good enough? I'm also not sure if used should be included in hashing as well.

I hope you're happy

Now my programs print:

mattnite@Matts-MacBook-Pro ~/c/dependency-variants (main)> ./zig-out/bin/i_should_print_debug
Debug
mattnite@Matts-MacBook-Pro ~/c/dependency-variants (main)> ./zig-out/bin/i_should_print_release_safe
ReleaseSafe

The repos to reproduce this are dependency-variants and echo.

[andrewrk]

why insertion sort rather than sortUnstable?

[mattnite]

Tbh it was the first one that came up in code completion. I didn't give this choice much thought because I doubt we'll see N approach even 1000.

[andrewrk]

OK please do change it to std.mem.sortUnstable. If you require stable sort, std.mem.sort is the way to go, and the time to use std.sort.insertion is pretty much never.

[mattnite]

done

[andrewrk]

looks like you solved that TODO below :)

[andrewrk]

hmm I think I see a big problem here. This is correctly checking if a value in a hash map can be re-used, but then at the end of the function it will put the new value into the hash map, overwriting the old one. In other words, it does not successfully deduplicate with more complicated set of inputs. If we passed ABA, for example, it would create two A's instead of one A and one B.

I think you already created all the glue code that is needed to solve this; the initialized_deps hash map needs to key on not only the build_root_string but on the user input options as well. That hash map is exclusively used in this function, so you are free to change it to this function's needs.

[mattnite]

Thanks for pointing that out, I completely missed it. I’ve added user input to the key of initialized_deps.

[mitchellh]

Just noting this is blocking my terminal (Ghostty) from adopting the package manager. We build a universal macOS binary as part of the Mac build and this issue means that we build two of the same arches in a row rather than two distinct ones! 😄

Thanks so much @mattnite for fixing this. :)

[ikskuh]

Another use case:
In AshetOS i build a fat library in a way that i can use it for both the host to make an os image as well as embed the library into the OS to actually read that file system.

The os isnt using the package manager yet, so i didnt encounter that problem

[mattnite]

If I try to build a dependency with different arguments today, only one version of that dependency is built, when it should build every variant. Here's a simplified example, I've imported the echo dependency twice, but in each scenario I've specified the dependency with different optimization modes.

// build.zigconstdep_variant_1=b.dependency("echo", .{
.target=target,
.optimize=.Debug,
});
constdep_variant_2=b.dependency("echo", .{
.target=target,
.optimize=.ReleaseSafe,
});

I'll now write a program to print the value returned by get_optimization() in the echo static library of the echo dependency, and compile it twice, each using one of the dependency variants:

// build.zig continued...constprogram_variant_1=b.addExecutable(.{
.name="i_should_print_debug",
.source_file= .{ .path="src/main.zig" },
});
program_variant_1.addModule(dep_variant_1.module("echo"));
b.installArtifact(program_variant_1);
constprogram_variant_2=b.addExecutable(.{
.name="i_should_print_release_safe",
.source_file= .{ .path="src/main.zig" },
});
program_variant_2.addModule(dep_variant_2.module("echo"));
b.installArtifact(program_variant_2);

However when I run both programs:

mattnite@Matts-MacBook-Pro ~/c/dependency-variants (main)> ./zig-out/bin/i_should_print_debug
Debug
mattnite@Matts-MacBook-Pro ~/c/dependency-variants (main)> ./zig-out/bin/i_should_print_release_safe
Debug

:(

Alright Andrew, I'll bite

walking down the b.dependency() call stack there's a shiny lil todo:

if (b.initialized_deps.get(build_root_string)) |dep| {
// TODO: check args are the samereturndep;
}

To fill in this TODO, the args of different Builds of the same dependency need to be compared. This patch generates a Build's user_input_options here for comparison with existing dependencies. If it's unique then the options are passed down to the newly created Build.

The other piece of work, and related TODO, was generating the hash for the install prefix. Which now looks like this:

varhash=b.cache.hash;
// Random bytes to make unique. Refresh this with new random bytes when// implementation is modified in a non-backwards-compatible way.hash.add(@as(u32, 0xd8cb0055));
hash.addBytes(b.dep_prefix);
hashUserInputOptionsMap(b.allocator, b.user_input_options, &hash);
// TODO additionally update the hash with `args`.constdigest=hash.final();
constinstall_prefix=tryb.cache_root.join(b.allocator, &.{ "i", &digest });
b.resolveInstallPrefix(install_prefix, .{});

AFAIK, it's possible for entries in aUserInputOptionsMap to have different orders depending on order of insertion. hashUserInputOptionsmap orders all values recursively -- TIL there could be maps in the user input option -- and then hashes everything.

Note anyone reviewing please take a look at the hashing of different user values like flag, it seems to me that just hashing the name would be good enough? I'm also not sure if used should be included in hashing as well.

I hope you're happy

Now my programs print:

mattnite@Matts-MacBook-Pro ~/c/dependency-variants (main)> ./zig-out/bin/i_should_print_debug
Debug
mattnite@Matts-MacBook-Pro ~/c/dependency-variants (main)> ./zig-out/bin/i_should_print_release_safe
ReleaseSafe

The repos to reproduce this are dependency-variants and echo.

[andrewrk]

why insertion sort rather than sortUnstable?

[mattnite]

Tbh it was the first one that came up in code completion. I didn't give this choice much thought because I doubt we'll see N approach even 1000.

[andrewrk]

OK please do change it to std.mem.sortUnstable. If you require stable sort, std.mem.sort is the way to go, and the time to use std.sort.insertion is pretty much never.

[mattnite]

done

[andrewrk]

looks like you solved that TODO below :)

[andrewrk]

hmm I think I see a big problem here. This is correctly checking if a value in a hash map can be re-used, but then at the end of the function it will put the new value into the hash map, overwriting the old one. In other words, it does not successfully deduplicate with more complicated set of inputs. If we passed ABA, for example, it would create two A's instead of one A and one B.

I think you already created all the glue code that is needed to solve this; the initialized_deps hash map needs to key on not only the build_root_string but on the user input options as well. That hash map is exclusively used in this function, so you are free to change it to this function's needs.

[mattnite]

Thanks for pointing that out, I completely missed it. I’ve added user input to the key of initialized_deps.

[mitchellh]

Just noting this is blocking my terminal (Ghostty) from adopting the package manager. We build a universal macOS binary as part of the Mac build and this issue means that we build two of the same arches in a row rather than two distinct ones! 😄

Thanks so much @mattnite for fixing this. :)

[ikskuh]

Another use case:
In AshetOS i build a fat library in a way that i can use it for both the host to make an os image as well as embed the library into the OS to actually read that file system.

The os isnt using the package manager yet, so i didnt encounter that problem

[mattnite]

If I try to build a dependency with different arguments today, only one version of that dependency is built, when it should build every variant. Here's a simplified example, I've imported the echo dependency twice, but in each scenario I've specified the dependency with different optimization modes.

// build.zigconstdep_variant_1=b.dependency("echo", .{
.target=target,
.optimize=.Debug,
});
constdep_variant_2=b.dependency("echo", .{
.target=target,
.optimize=.ReleaseSafe,
});

I'll now write a program to print the value returned by get_optimization() in the echo static library of the echo dependency, and compile it twice, each using one of the dependency variants:

// build.zig continued...constprogram_variant_1=b.addExecutable(.{
.name="i_should_print_debug",
.source_file= .{ .path="src/main.zig" },
});
program_variant_1.addModule(dep_variant_1.module("echo"));
b.installArtifact(program_variant_1);
constprogram_variant_2=b.addExecutable(.{
.name="i_should_print_release_safe",
.source_file= .{ .path="src/main.zig" },
});
program_variant_2.addModule(dep_variant_2.module("echo"));
b.installArtifact(program_variant_2);

However when I run both programs:

mattnite@Matts-MacBook-Pro ~/c/dependency-variants (main)> ./zig-out/bin/i_should_print_debug
Debug
mattnite@Matts-MacBook-Pro ~/c/dependency-variants (main)> ./zig-out/bin/i_should_print_release_safe
Debug

:(

Alright Andrew, I'll bite

walking down the b.dependency() call stack there's a shiny lil todo:

if (b.initialized_deps.get(build_root_string)) |dep| {
// TODO: check args are the samereturndep;
}

To fill in this TODO, the args of different Builds of the same dependency need to be compared. This patch generates a Build's user_input_options here for comparison with existing dependencies. If it's unique then the options are passed down to the newly created Build.

The other piece of work, and related TODO, was generating the hash for the install prefix. Which now looks like this:

varhash=b.cache.hash;
// Random bytes to make unique. Refresh this with new random bytes when// implementation is modified in a non-backwards-compatible way.hash.add(@as(u32, 0xd8cb0055));
hash.addBytes(b.dep_prefix);
hashUserInputOptionsMap(b.allocator, b.user_input_options, &hash);
// TODO additionally update the hash with `args`.constdigest=hash.final();
constinstall_prefix=tryb.cache_root.join(b.allocator, &.{ "i", &digest });
b.resolveInstallPrefix(install_prefix, .{});

AFAIK, it's possible for entries in aUserInputOptionsMap to have different orders depending on order of insertion. hashUserInputOptionsmap orders all values recursively -- TIL there could be maps in the user input option -- and then hashes everything.

Note anyone reviewing please take a look at the hashing of different user values like flag, it seems to me that just hashing the name would be good enough? I'm also not sure if used should be included in hashing as well.

I hope you're happy

Now my programs print:

mattnite@Matts-MacBook-Pro ~/c/dependency-variants (main)> ./zig-out/bin/i_should_print_debug
Debug
mattnite@Matts-MacBook-Pro ~/c/dependency-variants (main)> ./zig-out/bin/i_should_print_release_safe
ReleaseSafe

The repos to reproduce this are dependency-variants and echo.

[andrewrk]

why insertion sort rather than sortUnstable?

[mattnite]

Tbh it was the first one that came up in code completion. I didn't give this choice much thought because I doubt we'll see N approach even 1000.

[andrewrk]

OK please do change it to std.mem.sortUnstable. If you require stable sort, std.mem.sort is the way to go, and the time to use std.sort.insertion is pretty much never.

[mattnite]

done

[andrewrk]

looks like you solved that TODO below :)

[andrewrk]

hmm I think I see a big problem here. This is correctly checking if a value in a hash map can be re-used, but then at the end of the function it will put the new value into the hash map, overwriting the old one. In other words, it does not successfully deduplicate with more complicated set of inputs. If we passed ABA, for example, it would create two A's instead of one A and one B.

I think you already created all the glue code that is needed to solve this; the initialized_deps hash map needs to key on not only the build_root_string but on the user input options as well. That hash map is exclusively used in this function, so you are free to change it to this function's needs.

[mattnite]

Thanks for pointing that out, I completely missed it. I’ve added user input to the key of initialized_deps.

[mitchellh]

Just noting this is blocking my terminal (Ghostty) from adopting the package manager. We build a universal macOS binary as part of the Mac build and this issue means that we build two of the same arches in a row rather than two distinct ones! 😄

Thanks so much @mattnite for fixing this. :)

[ikskuh]

Another use case:
In AshetOS i build a fat library in a way that i can use it for both the host to make an os image as well as embed the library into the OS to actually read that file system.

The os isnt using the package manager yet, so i didnt encounter that problem

[mattnite]

If I try to build a dependency with different arguments today, only one version of that dependency is built, when it should build every variant. Here's a simplified example, I've imported the echo dependency twice, but in each scenario I've specified the dependency with different optimization modes.

// build.zigconstdep_variant_1=b.dependency("echo", .{
.target=target,
.optimize=.Debug,
});
constdep_variant_2=b.dependency("echo", .{
.target=target,
.optimize=.ReleaseSafe,
});

I'll now write a program to print the value returned by get_optimization() in the echo static library of the echo dependency, and compile it twice, each using one of the dependency variants:

// build.zig continued...constprogram_variant_1=b.addExecutable(.{
.name="i_should_print_debug",
.source_file= .{ .path="src/main.zig" },
});
program_variant_1.addModule(dep_variant_1.module("echo"));
b.installArtifact(program_variant_1);
constprogram_variant_2=b.addExecutable(.{
.name="i_should_print_release_safe",
.source_file= .{ .path="src/main.zig" },
});
program_variant_2.addModule(dep_variant_2.module("echo"));
b.installArtifact(program_variant_2);

However when I run both programs:

mattnite@Matts-MacBook-Pro ~/c/dependency-variants (main)> ./zig-out/bin/i_should_print_debug
Debug
mattnite@Matts-MacBook-Pro ~/c/dependency-variants (main)> ./zig-out/bin/i_should_print_release_safe
Debug

:(

Alright Andrew, I'll bite

walking down the b.dependency() call stack there's a shiny lil todo:

if (b.initialized_deps.get(build_root_string)) |dep| {
// TODO: check args are the samereturndep;
}

To fill in this TODO, the args of different Builds of the same dependency need to be compared. This patch generates a Build's user_input_options here for comparison with existing dependencies. If it's unique then the options are passed down to the newly created Build.

The other piece of work, and related TODO, was generating the hash for the install prefix. Which now looks like this:

varhash=b.cache.hash;
// Random bytes to make unique. Refresh this with new random bytes when// implementation is modified in a non-backwards-compatible way.hash.add(@as(u32, 0xd8cb0055));
hash.addBytes(b.dep_prefix);
hashUserInputOptionsMap(b.allocator, b.user_input_options, &hash);
// TODO additionally update the hash with `args`.constdigest=hash.final();
constinstall_prefix=tryb.cache_root.join(b.allocator, &.{ "i", &digest });
b.resolveInstallPrefix(install_prefix, .{});

AFAIK, it's possible for entries in aUserInputOptionsMap to have different orders depending on order of insertion. hashUserInputOptionsmap orders all values recursively -- TIL there could be maps in the user input option -- and then hashes everything.

Note anyone reviewing please take a look at the hashing of different user values like flag, it seems to me that just hashing the name would be good enough? I'm also not sure if used should be included in hashing as well.

I hope you're happy

Now my programs print:

mattnite@Matts-MacBook-Pro ~/c/dependency-variants (main)> ./zig-out/bin/i_should_print_debug
Debug
mattnite@Matts-MacBook-Pro ~/c/dependency-variants (main)> ./zig-out/bin/i_should_print_release_safe
ReleaseSafe

The repos to reproduce this are dependency-variants and echo.

[andrewrk]

why insertion sort rather than sortUnstable?

[mattnite]

Tbh it was the first one that came up in code completion. I didn't give this choice much thought because I doubt we'll see N approach even 1000.

[andrewrk]

OK please do change it to std.mem.sortUnstable. If you require stable sort, std.mem.sort is the way to go, and the time to use std.sort.insertion is pretty much never.

[mattnite]

done

[andrewrk]

looks like you solved that TODO below :)

[andrewrk]

hmm I think I see a big problem here. This is correctly checking if a value in a hash map can be re-used, but then at the end of the function it will put the new value into the hash map, overwriting the old one. In other words, it does not successfully deduplicate with more complicated set of inputs. If we passed ABA, for example, it would create two A's instead of one A and one B.

I think you already created all the glue code that is needed to solve this; the initialized_deps hash map needs to key on not only the build_root_string but on the user input options as well. That hash map is exclusively used in this function, so you are free to change it to this function's needs.

[mattnite]

Thanks for pointing that out, I completely missed it. I’ve added user input to the key of initialized_deps.

[mitchellh]

Just noting this is blocking my terminal (Ghostty) from adopting the package manager. We build a universal macOS binary as part of the Mac build and this issue means that we build two of the same arches in a row rather than two distinct ones! 😄

Thanks so much @mattnite for fixing this. :)

[ikskuh]

Another use case:
In AshetOS i build a fat library in a way that i can use it for both the host to make an os image as well as embed the library into the OS to actually read that file system.

The os isnt using the package manager yet, so i didnt encounter that problem

[mattnite]

If I try to build a dependency with different arguments today, only one version of that dependency is built, when it should build every variant. Here's a simplified example, I've imported the echo dependency twice, but in each scenario I've specified the dependency with different optimization modes.

// build.zigconstdep_variant_1=b.dependency("echo", .{
.target=target,
.optimize=.Debug,
});
constdep_variant_2=b.dependency("echo", .{
.target=target,
.optimize=.ReleaseSafe,
});

I'll now write a program to print the value returned by get_optimization() in the echo static library of the echo dependency, and compile it twice, each using one of the dependency variants:

// build.zig continued...constprogram_variant_1=b.addExecutable(.{
.name="i_should_print_debug",
.source_file= .{ .path="src/main.zig" },
});
program_variant_1.addModule(dep_variant_1.module("echo"));
b.installArtifact(program_variant_1);
constprogram_variant_2=b.addExecutable(.{
.name="i_should_print_release_safe",
.source_file= .{ .path="src/main.zig" },
});
program_variant_2.addModule(dep_variant_2.module("echo"));
b.installArtifact(program_variant_2);

However when I run both programs:

mattnite@Matts-MacBook-Pro ~/c/dependency-variants (main)> ./zig-out/bin/i_should_print_debug
Debug
mattnite@Matts-MacBook-Pro ~/c/dependency-variants (main)> ./zig-out/bin/i_should_print_release_safe
Debug

:(

Alright Andrew, I'll bite

walking down the b.dependency() call stack there's a shiny lil todo:

if (b.initialized_deps.get(build_root_string)) |dep| {
// TODO: check args are the samereturndep;
}

To fill in this TODO, the args of different Builds of the same dependency need to be compared. This patch generates a Build's user_input_options here for comparison with existing dependencies. If it's unique then the options are passed down to the newly created Build.

The other piece of work, and related TODO, was generating the hash for the install prefix. Which now looks like this:

varhash=b.cache.hash;
// Random bytes to make unique. Refresh this with new random bytes when// implementation is modified in a non-backwards-compatible way.hash.add(@as(u32, 0xd8cb0055));
hash.addBytes(b.dep_prefix);
hashUserInputOptionsMap(b.allocator, b.user_input_options, &hash);
// TODO additionally update the hash with `args`.constdigest=hash.final();
constinstall_prefix=tryb.cache_root.join(b.allocator, &.{ "i", &digest });
b.resolveInstallPrefix(install_prefix, .{});

AFAIK, it's possible for entries in aUserInputOptionsMap to have different orders depending on order of insertion. hashUserInputOptionsmap orders all values recursively -- TIL there could be maps in the user input option -- and then hashes everything.

Note anyone reviewing please take a look at the hashing of different user values like flag, it seems to me that just hashing the name would be good enough? I'm also not sure if used should be included in hashing as well.

I hope you're happy

Now my programs print:

mattnite@Matts-MacBook-Pro ~/c/dependency-variants (main)> ./zig-out/bin/i_should_print_debug
Debug
mattnite@Matts-MacBook-Pro ~/c/dependency-variants (main)> ./zig-out/bin/i_should_print_release_safe
ReleaseSafe

The repos to reproduce this are dependency-variants and echo.

[andrewrk]

why insertion sort rather than sortUnstable?

[mattnite]

Tbh it was the first one that came up in code completion. I didn't give this choice much thought because I doubt we'll see N approach even 1000.

[andrewrk]

OK please do change it to std.mem.sortUnstable. If you require stable sort, std.mem.sort is the way to go, and the time to use std.sort.insertion is pretty much never.

[mattnite]

done

[andrewrk]

looks like you solved that TODO below :)

[andrewrk]

hmm I think I see a big problem here. This is correctly checking if a value in a hash map can be re-used, but then at the end of the function it will put the new value into the hash map, overwriting the old one. In other words, it does not successfully deduplicate with more complicated set of inputs. If we passed ABA, for example, it would create two A's instead of one A and one B.

I think you already created all the glue code that is needed to solve this; the initialized_deps hash map needs to key on not only the build_root_string but on the user input options as well. That hash map is exclusively used in this function, so you are free to change it to this function's needs.

[mattnite]

Thanks for pointing that out, I completely missed it. I’ve added user input to the key of initialized_deps.

[mitchellh]

Just noting this is blocking my terminal (Ghostty) from adopting the package manager. We build a universal macOS binary as part of the Mac build and this issue means that we build two of the same arches in a row rather than two distinct ones! 😄

Thanks so much @mattnite for fixing this. :)

[ikskuh]

Another use case:
In AshetOS i build a fat library in a way that i can use it for both the host to make an os image as well as embed the library into the OS to actually read that file system.

The os isnt using the package manager yet, so i didnt encounter that problem

[mattnite]

If I try to build a dependency with different arguments today, only one version of that dependency is built, when it should build every variant. Here's a simplified example, I've imported the echo dependency twice, but in each scenario I've specified the dependency with different optimization modes.

// build.zigconstdep_variant_1=b.dependency("echo", .{
.target=target,
.optimize=.Debug,
});
constdep_variant_2=b.dependency("echo", .{
.target=target,
.optimize=.ReleaseSafe,
});

I'll now write a program to print the value returned by get_optimization() in the echo static library of the echo dependency, and compile it twice, each using one of the dependency variants:

// build.zig continued...constprogram_variant_1=b.addExecutable(.{
.name="i_should_print_debug",
.source_file= .{ .path="src/main.zig" },
});
program_variant_1.addModule(dep_variant_1.module("echo"));
b.installArtifact(program_variant_1);
constprogram_variant_2=b.addExecutable(.{
.name="i_should_print_release_safe",
.source_file= .{ .path="src/main.zig" },
});
program_variant_2.addModule(dep_variant_2.module("echo"));
b.installArtifact(program_variant_2);

However when I run both programs:

mattnite@Matts-MacBook-Pro ~/c/dependency-variants (main)> ./zig-out/bin/i_should_print_debug
Debug
mattnite@Matts-MacBook-Pro ~/c/dependency-variants (main)> ./zig-out/bin/i_should_print_release_safe
Debug

:(

Alright Andrew, I'll bite

walking down the b.dependency() call stack there's a shiny lil todo:

if (b.initialized_deps.get(build_root_string)) |dep| {
// TODO: check args are the samereturndep;
}

To fill in this TODO, the args of different Builds of the same dependency need to be compared. This patch generates a Build's user_input_options here for comparison with existing dependencies. If it's unique then the options are passed down to the newly created Build.

The other piece of work, and related TODO, was generating the hash for the install prefix. Which now looks like this:

varhash=b.cache.hash;
// Random bytes to make unique. Refresh this with new random bytes when// implementation is modified in a non-backwards-compatible way.hash.add(@as(u32, 0xd8cb0055));
hash.addBytes(b.dep_prefix);
hashUserInputOptionsMap(b.allocator, b.user_input_options, &hash);
// TODO additionally update the hash with `args`.constdigest=hash.final();
constinstall_prefix=tryb.cache_root.join(b.allocator, &.{ "i", &digest });
b.resolveInstallPrefix(install_prefix, .{});

AFAIK, it's possible for entries in aUserInputOptionsMap to have different orders depending on order of insertion. hashUserInputOptionsmap orders all values recursively -- TIL there could be maps in the user input option -- and then hashes everything.

Note anyone reviewing please take a look at the hashing of different user values like flag, it seems to me that just hashing the name would be good enough? I'm also not sure if used should be included in hashing as well.

I hope you're happy

Now my programs print:

mattnite@Matts-MacBook-Pro ~/c/dependency-variants (main)> ./zig-out/bin/i_should_print_debug
Debug
mattnite@Matts-MacBook-Pro ~/c/dependency-variants (main)> ./zig-out/bin/i_should_print_release_safe
ReleaseSafe

The repos to reproduce this are dependency-variants and echo.

[andrewrk]

why insertion sort rather than sortUnstable?

[mattnite]

Tbh it was the first one that came up in code completion. I didn't give this choice much thought because I doubt we'll see N approach even 1000.

[andrewrk]

OK please do change it to std.mem.sortUnstable. If you require stable sort, std.mem.sort is the way to go, and the time to use std.sort.insertion is pretty much never.

[mattnite]

done

[andrewrk]

looks like you solved that TODO below :)

[andrewrk]

hmm I think I see a big problem here. This is correctly checking if a value in a hash map can be re-used, but then at the end of the function it will put the new value into the hash map, overwriting the old one. In other words, it does not successfully deduplicate with more complicated set of inputs. If we passed ABA, for example, it would create two A's instead of one A and one B.

I think you already created all the glue code that is needed to solve this; the initialized_deps hash map needs to key on not only the build_root_string but on the user input options as well. That hash map is exclusively used in this function, so you are free to change it to this function's needs.

[mattnite]

Thanks for pointing that out, I completely missed it. I’ve added user input to the key of initialized_deps.

[mitchellh]

Just noting this is blocking my terminal (Ghostty) from adopting the package manager. We build a universal macOS binary as part of the Mac build and this issue means that we build two of the same arches in a row rather than two distinct ones! 😄

Thanks so much @mattnite for fixing this. :)

[ikskuh]

Another use case:
In AshetOS i build a fat library in a way that i can use it for both the host to make an os image as well as embed the library into the OS to actually read that file system.

The os isnt using the package manager yet, so i didnt encounter that problem

[mattnite]

If I try to build a dependency with different arguments today, only one version of that dependency is built, when it should build every variant. Here's a simplified example, I've imported the echo dependency twice, but in each scenario I've specified the dependency with different optimization modes.

// build.zigconstdep_variant_1=b.dependency("echo", .{
.target=target,
.optimize=.Debug,
});
constdep_variant_2=b.dependency("echo", .{
.target=target,
.optimize=.ReleaseSafe,
});

I'll now write a program to print the value returned by get_optimization() in the echo static library of the echo dependency, and compile it twice, each using one of the dependency variants:

// build.zig continued...constprogram_variant_1=b.addExecutable(.{
.name="i_should_print_debug",
.source_file= .{ .path="src/main.zig" },
});
program_variant_1.addModule(dep_variant_1.module("echo"));
b.installArtifact(program_variant_1);
constprogram_variant_2=b.addExecutable(.{
.name="i_should_print_release_safe",
.source_file= .{ .path="src/main.zig" },
});
program_variant_2.addModule(dep_variant_2.module("echo"));
b.installArtifact(program_variant_2);

However when I run both programs:

mattnite@Matts-MacBook-Pro ~/c/dependency-variants (main)> ./zig-out/bin/i_should_print_debug
Debug
mattnite@Matts-MacBook-Pro ~/c/dependency-variants (main)> ./zig-out/bin/i_should_print_release_safe
Debug

:(

Alright Andrew, I'll bite

walking down the b.dependency() call stack there's a shiny lil todo:

if (b.initialized_deps.get(build_root_string)) |dep| {
// TODO: check args are the samereturndep;
}

To fill in this TODO, the args of different Builds of the same dependency need to be compared. This patch generates a Build's user_input_options here for comparison with existing dependencies. If it's unique then the options are passed down to the newly created Build.

The other piece of work, and related TODO, was generating the hash for the install prefix. Which now looks like this:

varhash=b.cache.hash;
// Random bytes to make unique. Refresh this with new random bytes when// implementation is modified in a non-backwards-compatible way.hash.add(@as(u32, 0xd8cb0055));
hash.addBytes(b.dep_prefix);
hashUserInputOptionsMap(b.allocator, b.user_input_options, &hash);
// TODO additionally update the hash with `args`.constdigest=hash.final();
constinstall_prefix=tryb.cache_root.join(b.allocator, &.{ "i", &digest });
b.resolveInstallPrefix(install_prefix, .{});

AFAIK, it's possible for entries in aUserInputOptionsMap to have different orders depending on order of insertion. hashUserInputOptionsmap orders all values recursively -- TIL there could be maps in the user input option -- and then hashes everything.

Note anyone reviewing please take a look at the hashing of different user values like flag, it seems to me that just hashing the name would be good enough? I'm also not sure if used should be included in hashing as well.

I hope you're happy

Now my programs print:

mattnite@Matts-MacBook-Pro ~/c/dependency-variants (main)> ./zig-out/bin/i_should_print_debug
Debug
mattnite@Matts-MacBook-Pro ~/c/dependency-variants (main)> ./zig-out/bin/i_should_print_release_safe
ReleaseSafe

The repos to reproduce this are dependency-variants and echo.

[andrewrk]

why insertion sort rather than sortUnstable?

[mattnite]

Tbh it was the first one that came up in code completion. I didn't give this choice much thought because I doubt we'll see N approach even 1000.

[andrewrk]

OK please do change it to std.mem.sortUnstable. If you require stable sort, std.mem.sort is the way to go, and the time to use std.sort.insertion is pretty much never.

[mattnite]

done

[andrewrk]

looks like you solved that TODO below :)

[andrewrk]

hmm I think I see a big problem here. This is correctly checking if a value in a hash map can be re-used, but then at the end of the function it will put the new value into the hash map, overwriting the old one. In other words, it does not successfully deduplicate with more complicated set of inputs. If we passed ABA, for example, it would create two A's instead of one A and one B.

I think you already created all the glue code that is needed to solve this; the initialized_deps hash map needs to key on not only the build_root_string but on the user input options as well. That hash map is exclusively used in this function, so you are free to change it to this function's needs.

[mattnite]

Thanks for pointing that out, I completely missed it. I’ve added user input to the key of initialized_deps.

[mitchellh]

Just noting this is blocking my terminal (Ghostty) from adopting the package manager. We build a universal macOS binary as part of the Mac build and this issue means that we build two of the same arches in a row rather than two distinct ones! 😄

Thanks so much @mattnite for fixing this. :)

[ikskuh]

Another use case:
In AshetOS i build a fat library in a way that i can use it for both the host to make an os image as well as embed the library into the OS to actually read that file system.

The os isnt using the package manager yet, so i didnt encounter that problem