Oh, wow, very neat.

I'll give this a proper review soon, but skimming, this looks great. :)

Got a couple of formatting nits on docs regarding readability and such. None of that is blocking, but you'd be doing your users a favor with more white space and a more uniform structure. I won't go into more detail, this is just to point in the general direction. Maybe moving the documentation into the implementation file's doc comment would help contributors not forget to update documentation, but that's just an idle thought.

Maybe moving the documentation into the implementation file's doc comment would help contributors not forget to update documentation, but that's just an idle thought.

Not sure how that works.
Got a reference or something?

Edit: Are you referring to this: https://github.com/NixOS/rfcs/blob/master/rfcs/0145-doc-strings.md ?

Yes, and there are a couple of examples where we include rendered comments from library functions into the manual, it needs a very few lines of wiring it up. But only if you're interested taking the little detour. Having documentation at the level of detail you provide is already awesome.

@fricklerhandwerk show me

Unfortunately I just realized when using nix build --rebuild that the binaries built by deno compile are not built deterministically, which I somehow just assumed so far.

So I'm gonna convert this back to a draft.

Unfortunately I'm not gonna have time to work on this next week, so this might lay around a little while now.

@fricklerhandwerk show me

@aMOPel I checked in a bit more detail and it's not ready for a quick wiring, therefore sorry for the distraction.

If you're generally interested, here's where it happens for lib:

• lib-function-docs.nix invokes nixdoc on the library sections passed as parameters
• package.nix glues the result into the existing markdown
• functions.md is where that result is expected and included into the table of contents

We'd need something similar for other collections of functions as well, in this case for build helpers.

@fricklerhandwerk thank you!

Is the current form of the documentation good, then?

I changed the argument docs to the definition lists.

Yes it's very good as it is.

How does this relate to #326003? Does it supersede it?

Looks like it, along with my PR at #358252. Impressive work (especially the documentation)!

Having trouble with building invidious-companion

myPackage> Running phase: buildPhase
myPackage> Executing denoBuildHook
myPackage> Creating binary
myPackage> Download https://deno.land/x/crypto@v0.11.0/aes.ts
myPackage> Download https://deno.land/x/crypto@v0.11.0/block-modes.ts
myPackage> error: Import 'https://deno.land/x/crypto@v0.11.0/aes.ts' failed.
myPackage>     0: error sending request for url (https://deno.land/x/crypto@v0.11.0/aes.ts): client error (Connect): dns error: failed to lookup address information: Temporary failure in name resolution: failed to lookup address information: Temporary failure in name resolution
myPackage>     1: client error (Connect)
myPackage>     2: dns error: failed to lookup address information: Temporary failure in name resolution
myPackage>     3: failed to lookup address information: Temporary failure in name resolution
myPackage>     at file:///build/source/lib/helpers/verifyRequest.ts:2:21
myPackage> build for myPackage-0.1.0 failed in buildPhase with exit code 1

For some reason the deno compile in the build hook is trying to download deps and doesn't pull from the deps nix provides in the FOD.

@winterqt @emanueljg

I resolved the outstanding issues from my perspective.

Please review (again), if you have the time.

On NPM_TOKEN: as you wrote yourself, it compromises the integrity of the nix store, since the system closure now contains a world-readable secret. How about passing such a secret through nix.settings.extra-sandbox-paths? That way, we allow the user to pass it through with sops-nix or any similar secret management software (or simply writing to it yourself).

{ config, ... }: {
  sops.secrets.npm-token = { };
  nix.settings-extra-sandbox-paths = [ config.sops.secrets.npm-token.path ];
}

Though I'm not aware of how well-used this pattern is in the Nix ecosystem.

Interesting. I personally didn't use sops-nix/agenix and other secret management solutions yet, so I couldn't tell you.

When I wrote this build helper I looked at the code from the npm build helper and used as much of that as possible.

I noticed 3 ways to pass the secrets, which I outlined in the documentation. One of them I did not implement, since it was beyond my scope.

I didn't see anything about nix.settings-extra-sandbox-paths. That doesn't mean it wouldn't be a good idea.

I don't plan to add any more features to this PR, since this PR seems to me like a coherent and working unit, which improves nixpkgs.

But surely the integration of the nix.settings-extra-sandbox-paths can be done in a follow up PR.

Completely fair. LGTM

Heck yeah @aMOPel! Nix contributor!

This had a bunch of commits that didn't follow contributing guidelines and should have been cleaned up better before merging.

Oh, that is very true. I still wanted to squash these commits. My bad.

Can we correct this somehow?

Can we correct this somehow?

No, this is a "for next time" thing for you and @hsjobeki as the nixpkgs committer.

Can we correct this somehow?

No, this is a "for next time" thing for you and @hsjobeki as the nixpkgs committer.

Oh i am very sorry, i thought to squash the commits 🙂‍↕️ Not sure how to fix this afterwards. Any ideas?

EDIT: seems we really cannot fix it afterwards. I can only appologize seems i didnt pay full attention here

Oh i am very sorry, i thought to squash the commits 🙂‍↕️ Not sure how to fix this afterwards. Any ideas?

I recommend talking to your mentor @infinisil on this and making a "merge checklist" before clicking merge. It helped me when I made similar errors.

@hsjobeki Generally we prefer to ask contributors to squash commits as appropriate before merging, since we prefer to use merge commits for PRs over squashes and rebases. In this case, the PR wouldn’t be suitable as a single commit, so it must not be squash merged and the usual thing is to ask the contributor to fix up the history. As a committer you can usually force‐push to PR branches (though contributors can opt to disable this), but generally we tend to ask people to do it themselves instead (it’s better for a contributor to do it themselves if use commit signing, and it will help them with future contributions).

• Deno's internal logic for managing packages and their cache is not officially documented and not meant to be used as a stable api. So file formats and directory structures that this solution assumes can keep changing without notice. This means this solution will occasionally need fixing, when pkgs.deno is bumped and breaking changes were introduced by deno upstream.

Does this mean that the resulting FOD hashes could depend on upstream behaviour that is not guaranteed to be reproducible?

FOD hashes breaking because of upstream changes are hard to detect and can lead to mass breakage in and out of the tree. This is why we switched to @TomaSajt’s deterministic Cargo fetcher (merged a few months before a Rust update broke all our existing FOD hashes) and merged @yuyuyureka’s fetchYarnBerryDeps rather than using Yarn directly.

If this is just about the implementation and the FODs themselves should be easy to keep reproducible (i.e., containing only the minimum externally‐fetched data in the simplest format possible, with any complex logic that might change handled in non‐FOD builders) then no worries! I couldn’t tell from skimming the diff.

@hsjobeki Generally we prefer to ask contributors to squash commits as appropriate before merging, since we prefer to use merge commits for PRs over squashes and rebases. In this case, the PR wouldn’t be suitable as a single commit, so it must not be squash merged and the usual thing is to ask the contributor to fix up the history.

Noted.

• Deno's internal logic for managing packages and their cache is not officially documented and not meant to be used as a stable api. So file formats and directory structures that this solution assumes
  If this is just about the implementation and the FODs themselves should be easy to keep reproducible (i.e., containing only the minimum externally‐fetched data in the simplest format possible, with any complex logic that might change handled in non‐FOD builders) then no worries! I couldn’t tell from skimming the diff.

Regarding the FOD hashes. There is of course the risk of breaking FODs if deno changes its dependency fetching and storing behavior in a breaking manner. if whe where to implement a custom fetcher that wouldnt really reduce the risk by much and cost a lot of effort. This implementation used denos native fetching, letting deno produce its desired output by deno itself.
We clean up that data as much as possible to reduce the potential breaking surface and influence of impurities. I hope this mechanism will proove itself as stable. Any objections to this approach?

Regarding the FOD hashes. There is of course the risk of breaking FODs if deno changes its dependency fetching and storing behavior in a breaking manner. if whe where to implement a custom fetcher that wouldnt really reduce the risk by much and cost a lot of effort. This implementation used denos native fetching, letting deno produce its desired output by deno itself. We clean up that data as much as possible to reduce the potential breaking surface and influence of impurities. I hope this mechanism will proove itself as stable. Any objections to this approach?

Yes, I’m afraid I object and think this probably shouldn’t have been merged, in that case. It’s more important for FODs to be stable than other derivations, because existing FODs in the store that match the hash will be silently reused even if the implementation later breaks the hashes. That can cause silent reproducibility issues that can persist indefinitely, especially since cache.nixos.org will mean that even the vast majority of external users won’t notice such an issue if it’s due to a use in Nixpkgs.

Why do you say that a custom fetcher doesn’t reduce the risk significantly? It is usually relatively simple to write one given a reasonable lock file format (yarn-berry being an exception that complicates things by deliberately omitting hashes from lock files, but I doubt Deno does that). fetchCargoVendor is a good example here: the FOD consumes the lock file and produces the simplest possible output of just a bare tree of downloaded packages (not even unpacking tarballs downloaded from crates.io), so that we have no reason to break the output in the future. That can then be processed in the actual build into the form that Cargo expects, so that we never need to break FOD hashes because of changes in Cargo. (The FOD itself is simpler than it appears there, since the same script handles both stages.)

Deno’s ability to use NPM packages might complicate this approach, but note that we do already have a deterministic NPM fetcher in the tree whose logic could be reused for that. There’s a reason we keep end up writing these :)

It may be that a filtering approach is sufficient here, but past experience with other package managers in Nixpkgs makes me wary. The worst case is having to choose between undetectable mass breakage and keeping around an insecure EOL version of a language runtime forever. When Rust broke years of accumulated FOD hashes in a single release, we were thankfully able to switch over the implementation so that users at least got FOD mismatches, but that’s not always possible. 300 lines of script is a pretty small cost in comparison to that – especially since the implementation here is that long already.

What does the output format of the FOD used here look like? If it’s sufficiently simple to derive from the lock file, then hopefully we will be able to maintain the format even if Deno changes. But the risk of updates to Deno causing changes in the output that we don’t even notice is real, and something we need to have proactive tests against if we’re going to use its dependency fetching logic in the implementation.

(None of this means I don’t appreciate @aMOPel’s work on this and hope that we can ensure it works in a reliable manner, of course! Just a process failure here – we should probably explicitly write down expectations for FOD fetchers.)

I agree there needs to be a plan for how to deal with deno breaking the FOD hashes, as this has been very painful with Cargo in the past (where we also thought it was sane to use Cargo's builtin vendoring mechanism).

I agree with the concerns proposed by @emilazy. Is it feasible to revert this PR and put it in a draft state until a custom minimal-assumptions FOD fetcher has been added? Or is it something that should be done in a follow-up PR? Regardless, we should raise an issue discussing the future contribution process of new FOD fetchers.

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/fodwatch-automation-for-detecting-reproducibility-issues-in-fixed-output-derivations/28671/7

Just finding this due to build failures in #416177

I'll skip over some of the other points like cleaning up the commits which were merged since they're already covered.

As the listed deno maintainers please in future CC @ofalvai and myself. Especially when we're put down to maintain a new package.

Deno already builds the denort binary but it's removed. The correct solution for us would probably have been to add an extra output & keep denort.

@hsjobeki Are you amenable to reverting for now? Seems there are multiple concerns that could use addressing before we land this.

@aMOPel Sorry about this, I know it must be a disheartening experience and I wish I’d seen this sooner! Thanks for working on it and I hope we can get it landed in revised form soon.

@06kellyjac very good point. Sorry for not pinging you first on this.

@aMOPel It seems we needed to revert this. Could you reopen a new PR and set as draft for now?
@06kellyjac offered some help with resolving the remaining issue that where discovered post-merge. I think they might be better help on that topic than me.

@emilazy it seems you are a good person to talk to about FOD fetchers (Do you happen to know if we have some guidelines for @aMOPel adding FOD fetchers?)

EDIT: so far i think the premise is that we must implement a custom fetcher, that is decoupled from deno entirely. We could formalize this requirement. It somehow slipped through my mind because i am mostly doing lib stuff lately ^^. Would be happy to add that to our commit guidelines to prevent errors like this in the future.

@emilazy

So I saw the custom fetcher solution for npmBuildPackage, but I didn't see the need when I wrote this contribution.
You make a very good point that I wasn't aware of, though.

My analysis of the problem I tried to solve was:

Deno's package management as far as I'm aware is not really a documented and stable interface.

I see 3 parts to this Interface

1. The lock file has a version, currently version: 5. So the lock file is "versioned".

2. The format of how the directory tree for the dependencies looks like, that deno expects, is not stable or documented.

3. On top of that deno injects many stateful files into that directory tree, to cache work.

   3.1. Some files can simply be deleted and will be regenerated by the deno cli.
   3.2. Some files can't be deleted (registry.json and meta.json files), since deno won't recognise the dependencies, if they are missing.

The work I did now was basically reverse engineering the files in 3. and clean them up.
3.2. especially needs information from the lock file. So some compatibility to 1. Is also needed, but it is limited.
In this solution we don't have to worry about compatibility with 2. at all, since the deno CLI is inherently compatible with it.

I understand the problem you describe like this:

Scenario:
A. In nixpkgs we bump the deno version
B. Deno upstream changed point 2. And/or 3.2. From above

Then the FOD of the deps-build and thus its hash would change, since the deno cli now produces a new output.

However, since the deps-build is in the nix cache and the consumer of the deps-build didn't change the expected hash, the cached version will be used in the build, even though, a rebuild would produce a different FOD hash.

Did I fully understand it?

When we build our custom fetcher, then we have to ensure that it is always up to date with points 1., 2. And 3.2. From above. It's currently hard for me to gauge how frequent that breakage would happen. I will inquire that from the deno maintainers.