Repository navigation
Reproducible builds on Linux #3043
Description
Activity
@joyeecheung do you have any thoughts/suggestions on the snapshot front?
@sxa I assume that it should be possible to update
https://github.com/nodejs/node/blob/main/deps/v8/tools/gen-postmortem-metadata.pyso that it generates data in a deterministic manner (maybe sorting them at some point) and it's really just a matter of doing that versus there being some fundamental reason whey it might not be possible?That would be my assumption yes. The previous issues and PRs suggest this was resolved, so we'd just need to understand what happened and potentially reimplement a fix for it.
Pinging @warpfork for this thread. He's currently heads-down on building Warpforge and has a passion for reproducible builds and I was telling him that there's interest in making progress on this front for Node.js so there might be some potential for collaboration and the production of some tooling to both generate reproducible builds and also document & provide mechanisms for people to do it for themselves without all the pain that usually comes from doing it manually.
So @sxa, meet the most excellent and clever @warpfork; maybe you two should chat.
Thanks @rvagg - I've worked on reproducible build environments for another project, and the Node.js build is close other than those two issues above, so it should just require a little bit of knowledge about the Node processes involved in producing those two things to be able to resolve it, then we can possibly put some more robust things in place for making our builds reproducible by default, which I think should be the aim here.
@warpfork DO you have much experience on non-Linux reproducible build work (I've so far only looked at Linux for Node.js)
1. The production of `debug-support.cc` is nondeterministic - the definitions are not always in the same order within the file. It is generated by https://github.com/nodejs/node/blob/main/deps/v8/tools/gen-postmortem-metadata.py - If I copy in a consistent one the builds can be reproducible.One source of indeterminism is here:
Instead of a set a dict can be used here (with
Nonevalues), as since Python 3.7 dict preserves order. Then usefor line in dict.fromkeys(lines):to output the lines in order.Is there a todo on this issue or can it be closed?
Definitely still work to do and I still plan to look at it.
Reacted by Christian ClaussJust tried four consecutive builds with the latest code and the
debug-support.cccame out the same each time. There have been four V8 version bumps since I last tested (10.1.124.6 ->11.3.244.4) so either I'm lucky today or something in there has made things better, but potentially the fix in the earlier comment will no longer required.This was tested with
./configure --without-node-snapshotand settingSOURCE_DATE_EPOCH=0in the environment.With snapshots enabled the builds are still non-reproducible. due to
node_snapshot.cc(Note thatnode_code_cache.ccreferred to in nodejs/node#29108 is no longer a problem on the Linux system being used.Looks like the break was between these two commits, so nodejs/node@1faf6f459f likely made it non-reproducible again.
* 1faf6f459f 2019-04-21 | src: snapshot Environment upon instantiation (HEAD, refs/bisect/bad) [Joyee Cheung] * f04538761f 2020-04-30 | tools: enable Node.js command line flags in node_mksnapshot (refs/bisect/good-f04538761f5bb3c334d3c8d16d093ac0916ff3bc) [Joyee Cheung]FYI @joyeecheung (PR) @bnoordhuis (PR)
I don't think I have enough knowledge of this code to be able to propose a safe solution here so looking for advice.
@joyeecheung Would you be able to assist with the reproducibility of the snapshots now? I'm not sure who else might have good knowledge of the snapshot support in Node so anyone else might have to start from scratch.
Thanks for the ping, not sure how I missed the earlier one. I diff'ed the generated snapshot locally and found ~20 lines of differences in the snapshot.cc generated (with
--predictable, which we should also set for mksnapshot), most notably the binding data are initialized in different order, not sure why but they are supposed to be initialized in the same order. I'll look into why that happens.With this patch the snapshot.cc difference is down to 13 lines. Still need to figure out the differences in the blob though.
see diff
diff --git a/src/cleanup_queue-inl.h b/src/cleanup_queue-inl.h index 5d9a56e6b0..d1fbd8241d 100644 --- a/src/cleanup_queue-inl.h +++ b/src/cleanup_queue-inl.h @@ -39,7 +39,9 @@ void CleanupQueue::Remove(Callback cb, void* arg) { template <typename T> void CleanupQueue::ForEachBaseObject(T&& iterator) const { - for (const auto& hook : cleanup_hooks_) { + std::vector<CleanupHookCallback> callbacks = GetOrdered(); + + for (const auto& hook : callbacks) { BaseObject* obj = GetBaseObject(hook); if (obj != nullptr) iterator(obj); } diff --git a/src/cleanup_queue.cc b/src/cleanup_queue.cc index 6290b6796c..c0fcda2fac 100644 --- a/src/cleanup_queue.cc +++ b/src/cleanup_queue.cc @@ -5,7 +5,7 @@ namespace node { -void CleanupQueue::Drain() { +std::vector<CleanupQueue::CleanupHookCallback> CleanupQueue::GetOrdered() const { // Copy into a vector, since we can't sort an unordered_set in-place. std::vector<CleanupHookCallback> callbacks(cleanup_hooks_.begin(), cleanup_hooks_.end()); @@ -20,6 +20,12 @@ void CleanupQueue::Drain() { return a.insertion_order_counter_ > b.insertion_order_counter_; }); + return callbacks; +} + +void CleanupQueue::Drain() { + std::vector<CleanupHookCallback> callbacks = GetOrdered(); + for (const CleanupHookCallback& cb : callbacks) { if (cleanup_hooks_.count(cb) == 0) { // This hook was removed from the `cleanup_hooks_` set during another diff --git a/src/cleanup_queue.h b/src/cleanup_queue.h index 64e04e1856..2ca333aca8 100644 --- a/src/cleanup_queue.h +++ b/src/cleanup_queue.h @@ -6,6 +6,7 @@ #include <cstddef> #include <cstdint> #include <unordered_set> +#include <vector> #include "memory_tracker.h" @@ -66,6 +67,7 @@ class CleanupQueue : public MemoryRetainer { uint64_t insertion_order_counter_; }; + std::vector<CleanupHookCallback> GetOrdered() const; inline BaseObject* GetBaseObject(const CleanupHookCallback& callback) const; // Use an unordered_set, so that we have efficient insertion and removal. diff --git a/tools/snapshot/node_mksnapshot.cc b/tools/snapshot/node_mksnapshot.cc index ecc295acdb..2ba6878a28 100644 --- a/tools/snapshot/node_mksnapshot.cc +++ b/tools/snapshot/node_mksnapshot.cc @@ -52,6 +52,7 @@ int main(int argc, char* argv[]) { #endif // _WIN32 v8::V8::SetFlagsFromString("--random_seed=42"); + v8::V8::SetFlagsFromString("--predictable"); v8::V8::SetFlagsFromString("--harmony-import-assertions"); return BuildSnapshot(argc, argv); }
Reacted by Richard LauReacted by Richard LauSounds like good progress - thanks @joyeecheung!
Checking the snapshots again, another source of indeterminism comes from performance data (milestones, time origin etc.), the easiest way to fix it is probably discarding it before snapshot generation. But I'll need to check if/how they should be synchronized.
With nodejs/node#48702 and nodejs/node#48708 and --predictable the differences are down to 8 places:
- 7 of which coming from binding data's embedder data slot
- 1 of which coming from the context embedder data slot
We probably need some V8 patches to make it deterministic (my guess is, v8 doesn't actually need to copy the exact values of those slots into the snapshot, they are only there as place holders, so v8 should probably just copy the same amount of 0s for those - still working locally to see if this is correct)EDIT: we can just return some non-empty data for both BaseObject slots to fix 1. 2 probably need a V8 patch for us to customize how the slots should be serialized. Trying to work out a prototype.
Reacted by Stewart X Addison60 remaining items
- added a commit that references this issue
on Jun 20, 2024 Are the reproducibility tests only comparing doing a build twice on identical hardware?
Node 22.4.0 is not reproducible in our testing on stagex, when building on two different ryzen machines with different specs.
See my comment here: #3043 (comment)
I tried
CFLAGS="-march=x86-64 -mtune=generic -O2"to try to avoid any cpu-specific optimizations but it seems this was not enough.Hmm, I am pretty sure it is related to the CPU features hash V8 adds to the code cache, which is encoded as a bit field including the following properties: https://chromium.googlesource.com/v8/v8/+/7c7c6b475c6edcd7ef863e1f668be3df55c56307/src/codegen/cpu-features.h#15 - so it's not strictly required that the hardware must be identical, but the differences need to be out of the probed set.
We also add the cached version tag (which includes this bitfield) to the custom snapshot blob, but for the built-in snapshot at least we can remove it since we don't check it for built-in snapshot anyway. But the ones in the code cache would be harder to get rid of as V8 has some concerns about skipping the CPU feature test (see the discussions in https://chromium-review.googlesource.com/c/v8/v8/+/4905290). I wonder if we can convince the upstream to change the bits set in the code cache to be a combination of "CPU features required by this code cache" instead of "CPU features of the platform that compiles this code cache" (the two aren't necessarily the same and at least for now, the former is basically "none" because V8 doesn't include any optimized code in the code cache, though they want to reserve the ability to make it CPU-specific. But at least making it more specific about the actual requirement instead of doing a blind equality check would help our use case).
Oh wait you are already using
--without-node-snapshot, then I am not quite sure what else is varying - though according to #3043 (comment) other than libnode it comes fromlibnghttp2,libicuucxandlibucii18nandlibicutools- added a commit that references this issue
on Aug 23, 2024 - added a commit that references this issue
on Aug 25, 2024 Confirmed I am finally able to build nodejs 22.7.0 reproducibly across multiple different systems/cpus:
https://codeberg.org/stagex/stagex/pulls/95/files
--without-node-snapshot is no longer an option in this release so it builds with it enabled, however the issue (as suspected earlier) was nodejs currently does not build some deps (icu, openssl, brotli, libev, nghttp2, c-ares) deterministically.
I had to package all those myself separately and get those deterministic, then have node build against the system versions instead of in-tree versions, but it works!
Are the reproducibility tests only comparing doing a build twice on identical hardware?
Yes. And it's done on aarch64 (chosen due to the fact we have machines with very high number of cores there which means it's feasible to run the test without ccache without them taking up too much machine time!)
nodejs currently does not build some deps (icu, openssl, brotli, libev, nghttp2, c-ares) deterministically.
That's useful to know - thank you. Although it's interesting that it's only showing up when building on different machines. Do you know if it's definitely a problem for all of those that you mentioned?
- added a commit that references this issue
on Aug 30, 2024 CI build started failing with 4 Feb build. The previous 28 Jan build passed.
The failures are due to ada changes that don't work with gcc 10 (which the job was using). We bumped the minimum supported gcc compiler to 12 for Node.js 23 -- I've edited the reproducibility-test job to swap
gcc-10/g++-10togcc-12/g++-12and am running a test build: https://ci.nodejs.org/job/reproducibility-test/102Reacted by Stewart X AddisonLGTM and those jobs are running green consistently. I feel it's time we closed this one :-)
ANy subsequent problems can be managed as separate issues now. Thanks everyone, especially @joyeecheung and for the two blog posts on the topic:
In the dim and distant past before global lockdowns there was some previous work to make the build process binary reproducible.
I had a play recently with the process (on Linux/aarch64 because they are the quickest machines I have access to) and I hit two issues:
debug-support.ccis nondeterministic - the definitions are not always in the same order within the file. It is generated by https://github.com/nodejs/node/blob/main/deps/v8/tools/gen-postmortem-metadata.py - If I copy in a consistent one the builds can be reproducible.configure --without-node-snapshot), which suggests that the changes made in node_code_cache.cc and node_snapshot.cc generation is unreproducible node#29108 are no longer valid for the current release.Tagging @ChALkeR who put the original PR in (albeit three years ago!)