Skip to content

package manager: prevent confusing copy+paste error with package .hash entries #16679

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

Open
emidoots opened this issue Aug 4, 2023 · 7 comments
Labels
enhancement Solving this issue will likely involve adding new logic or components to the codebase. use case Describes a real use case that is difficult or impossible, but does not propose a solution. zig build system std.Build, the build runner, `zig build` subcommand, package management
Milestone

Comments

@emidoots
Copy link

emidoots commented Aug 4, 2023

Zig Version

0.11.0

Steps to Reproduce and Observed Behavior

  1. Start with any existing Zig project that uses the package manager (you must have ran zig build previously, as in normal development)
  2. Go to add a new dependency, and copy+paste a section of build.zig.zon like this:
        .mach_earcut = .{
            .url = "https://pkg.machengine.org/mach-earcut/fa2d4962181d812960838435f95916c95f7c9132.tar.gz",
            .hash = "1220da97fa7b8ffb9576aa3e2346b5e82d93f3d1fa67f30cc5a5afe4b401008d40c7",
        },

to create your new entry:

+        .mach_newdep = .{
+            .url = "https://pkg.machengine.org/mach-newdep/ed8129635f9592d4962181d16c95f7c913208384.tar.gz",
             .hash = "1220da97fa7b8ffb9576aa3e2346b5e82d93f3d1fa67f30cc5a5afe4b401008d40c7",
+        },

If you forgot to delete the .hash, or otherwise invalidate it, then zig build will not complain about the hash being wrong, but rather you'll get very confusing errors because .mach_newdep will actually be pointing to .mach_earcut (the hash you copied) in your ~/.cache/zig/p folder.

This seems like a footgun that many users will encounter, I've personally had ~5 people run into this. Because the hashes in the compiler errors obfuscate which dependency actually lives inside that cache directory, it can lead to some very confusing error messages (e.g. b.dependency reporting an artifact doesn't exist.

Expected Behavior

Some way to avoid the copy+paste footgun here.

@emidoots emidoots added the bug Observed behavior contradicts documented or intended behavior label Aug 4, 2023
@andrewrk andrewrk added this to the 0.12.0 milestone Aug 4, 2023
@andrewrk andrewrk added enhancement Solving this issue will likely involve adding new logic or components to the codebase. zig build system std.Build, the build runner, `zig build` subcommand, package management use case Describes a real use case that is difficult or impossible, but does not propose a solution. and removed bug Observed behavior contradicts documented or intended behavior labels Aug 4, 2023
@mitchellh
Copy link
Contributor

I've also experienced this. I completely understand why this is happening, especially as a Nix user I really get it. But I agree that it does lead to some head scratching periodically before saying "oh yeah, duh."

@rohlem
Copy link
Contributor

rohlem commented Aug 4, 2023

Might not be worth it, but a crazy idea to fix this would be to invert the hierarchy to mirror our actual caching strategy (so the top-level entry becomes the hash instead of the name):

 .@"1220da97fa7b8ffb9576aa3e2346b5e82d93f3d1fa67f30cc5a5afe4b401008d40c7" = .{
    .name = "mach_earcut",
    .url = "https://pkg.machengine.org/mach-earcut/fa2d4962181d812960838435f95916c95f7c9132.tar.gz",
},

Then the .zon parser can complain on duplicate keys (I assume it does? it definitely should), and the error and fix would be relatively understandable/obvious to users.
(I guess generally speaking my idea was that exposing the underlying structure to the users would help them understand/track down unexpected behavior. As-is it's not obvious to me that a field "hash" is essentially used as sole identification criterion, for both identity+version of a package.)

@mitchellh
Copy link
Contributor

Then the .zon parser can complain on duplicate keys (I assume it does? it definitely should), and the error and fix would be blatantly obvious to users.

The parser could just as easily complain about duplicate hashes in a single zon file without it being the keys

@rohlem
Copy link
Contributor

rohlem commented Aug 4, 2023

The parser could just as easily complain about duplicate hashes

I assume that would be after parsing, but the build system could complain, that's true.

@drujensen
Copy link

I'm new to zig and I'm a bit sad the packages are not installed in the project like a libs or deps directory instead of the ~/.cache/zig directory.

This would remove the need for hashes and allow you to quickly view the dependency code in your editor instead of trying to find the code in a cached hash directory. It also makes it dead simple to cleanup and pull down the latest dependencies by removing this local directory. It also prevents collisions with other zig projects since the deps are locally cached instead of using a shared cache.

Crystal language package manager does this and it makes development so much easier. It also supports pulling from github using tags or branches. You can add the libs or deps directory in a .gitignore to avoid conflicts with git.

@marler8997
Copy link
Contributor

marler8997 commented Aug 28, 2023

The solution I propose is to maintain a set of "verified url/hash pairings" somewhere inside zig's global cache. Zig doesn't use any url hash until after it's been verified locally. When zig encounters a new URL/hash pair, it verifies the pairing is correct before adding it to the "trusted store". This allows zig to detect erroneous mismatches and report them to the user. Breaking zig's URL/hash correctness would now require mucking with Zig's internal trusted store instead of making an innocent mistake modifying build.zig.zon. This maintains the benefit we get from relying on "trusted hashes" (no need for network access to verify URL contents) and avoids the footgun.

Beyley added a commit to LittleBigRefresh/FreshPresence that referenced this issue Sep 1, 2023
Beyley added a commit to LittleBigRefresh/FreshPresence that referenced this issue Sep 1, 2023
@andrewrk andrewrk modified the milestones: 0.12.0, 0.13.0 Jan 9, 2024
@karlseguin
Copy link
Contributor

A couple examples of users running into this issue:

karlseguin/http.zig#29

karlseguin/pg.zig#4 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Solving this issue will likely involve adding new logic or components to the codebase. use case Describes a real use case that is difficult or impossible, but does not propose a solution. zig build system std.Build, the build runner, `zig build` subcommand, package management
Projects
None yet
Development

No branches or pull requests

7 participants