Repository navigation
build: rebuild test addons conditionally - #335
bnoordhuis wants to merge 1 commit into
Conversation
Before this commit, `make test-addons` and the targets that depend on it (test-ci, test-all) would always do a full rebuild of the files in test/addons and the addons scraped from doc/api/addons.markdown. This commit introduces a proper dependency chain so that files are only rebuilt when changed, shaving off about 10-20 seconds from each run.
There was a problem hiding this comment.
it doesn't look like addon-verify is run, so those doc tests won't be built?
There was a problem hiding this comment.
(Which I'm fine with for now, by the by, I'm not a huge fan of doctests.)
There was a problem hiding this comment.
Sorry, perhaps I made it too obtuse for the sake of DRY. In the line below, $< is replaced with tools/doc/addon-verify.js.
There was a problem hiding this comment.
This still breaks on OSX for me on first run -- running ./$(NODE_EXE) addon-verify.js in a separate target to create the test/addon/doc-* dirs that the test/addons/.stamp target depends on should fix this.
There was a problem hiding this comment.
Oh! That's the problem -- it evaluates and expands $(foreach dir... before executing addon-verify.js -- and thus it can't see those new directories, and the test fails because node-gyp hasn't run in those dirs.
There was a problem hiding this comment.
I wonder if this is a make issue... I see what you are saying but I can only reproduce it on OS X with make 3.81. On Linux with make 4.0, it always builds things in the right order, no matter how many jobs I throw at it. Will investigate.
There was a problem hiding this comment.
I can't reproduce it on OS X anymore either, I wonder what changed... @chrisdickinson Can you give it one more try? If it still fails for you, can you tell me how to reproduce?
There was a problem hiding this comment.
To reproduce you have to rm -rf test/addons/doc-* between test runs.
On Jan 16, 2015, at 4:04 AM, Ben Noordhuis notifications@github.com wrote:
In Makefile:
- at-exit \
- hello-world-function-export \
- hello-world \
- repl-domain-abort \
+# Note: don't depend on $(NODE_EXE) here, that unconditionally forces a rebuild.
+test/addons/%/build/Release/binding.node: \
test/addons/%/binding.cc test/addons/%/binding.gyp- ./$(NODE_EXE) deps/npm/node_modules/node-gyp/bin/node-gyp rebuild \
--nodedir="$(PWD)" --directory="$(dir $<)"+build-static-addons: \
$(NODE_EXE) $ (ADDONS_TESTS:%=test/addons/%/build/Release/binding.node)
+# Note: don't depend on $(NODE_EXE) here, that unconditionally forces a rebuild.
+test/addons/.stamp: tools/doc/addon-verify.js doc/api/addons.markdown
I can't reproduce it on OS X anymore either, I wonder what changed... @chrisdickinson Can you give it one more try? If it still fails for you, can you tell me how to reproduce?—
Reply to this email directly or view it on GitHub.
|
To recap: the issue is that this expands before any targets are run. The directories that expansion looks for are created by the previous step. On clean builds, this fails because the generated directories aren't present. On subsequent re-runs, the directories are there and the tests pass. |
|
I just tried this on my mac as well (10.10 should that matter, gnu make 3.81) - suspecting that is was an parallelism issue (gmake has .NOTPARALLEL if needed). Can't reproduce your error @chrisdickinson. I'd really like to sort this out to be able to get jenkins running |
|
Closing, stale. |
Bring the port in line with the latest replayio/chromium-v8#334 and nodejs#335 (up to 609b71dddca): - Rename the added fields and runtime functions with the record_replay / RecordReplay prefix, and pass Heap::RecordReplayTracking to the dirty registry helpers and the cleanup task. Heap::DequeueDirtyJSFinalizationRegistry loses its last caller and is removed. - Record what the GC did to tracked registries and WeakRefs in a single ReplayGCPoll::Poll at every microtask checkpoint. - Track WeakRefs constructed at a point which replays ("weak-ref-collection" feature): the recording notes which ones its GC cleared, and the replay clears the same targets instead of keeping them alive until the WeakRef goes away. The list of tracked WeakRefs lives in old space. - Give a WeakCell its id before it is linked into the registry, cache IsReplaying() in the marking visitor, and crash when a replayed deref() or a delivered cell disagrees with the recording. V8 9.4 adaptations: src/replay/replayio.{h,cc} only carry AreEventsAvailable(), global-handles.h replaces global-handles-inl.h, and the Torque constructor uses assert where the fork uses dcheck. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
* Port WeakRef and FinalizationRegistry record/replay from the Chromium fork Port replayio/chromium-v8#279, nodejs#334 and nodejs#335, ending at the state of nodejs#335: - WeakRef.prototype.deref() records/replays whether the target is alive. When replaying, marking treats JSWeakRef::target as strong and a deref() the recording saw as dead clears the field. - FinalizationRegistry cleanup of registries constructed at a point that replays ("tracked", behind the "finalization-registry" feature) is driven by the recording: the GC no longer schedules the cleanup task for them, ReplayFinalizationRegistries::Poll records the decision at every microtask checkpoint, and the cleanup loop records which cell each callback is called for. When replaying, the targets of tracked cells are marked strongly, and the recorded cells are cleared right before their callbacks run. Registries the recording's GC collected are released. - unregister() and register() of a tracked cell while events are disallowed crash, since they would change the registry on one side only. - Per-isolate replay state lives in ReplayIsolateData, reset in Isolate::Deinit before the global handles go away. V8 9.4 adaptations: JSFinalizationRegistry accessors are hand-written, the unregister bookkeeping hooks into Unregister's match callback, and recordreplay::AreEventsPassedThrough is added (chromium-v8 f6f61489066). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Update the WeakRef/FinalizationRegistry port to the current fork PRs Bring the port in line with the latest replayio/chromium-v8#334 and nodejs#335 (up to 609b71dddca): - Rename the added fields and runtime functions with the record_replay / RecordReplay prefix, and pass Heap::RecordReplayTracking to the dirty registry helpers and the cleanup task. Heap::DequeueDirtyJSFinalizationRegistry loses its last caller and is removed. - Record what the GC did to tracked registries and WeakRefs in a single ReplayGCPoll::Poll at every microtask checkpoint. - Track WeakRefs constructed at a deterministic point ("weak-ref-collection" feature): the recording notes which ones its GC cleared, and the replay clears the same targets instead of keeping them alive until the WeakRef goes away. The list of tracked WeakRefs lives in old space. - Give a WeakCell its id before it is linked into the registry, cache IsReplaying() in the marking visitor, and crash when a replayed deref() or a delivered cell disagrees with the recording. V8 9.4 adaptations: src/replay/replayio.{h,cc} only carry AreEventsAvailable(), global-handles.h replaces global-handles-inl.h, and the Torque constructor uses assert where the fork uses dcheck. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Track FinalizationRegistries deserialized from a snapshot A FinalizationRegistry gets its record/replay id when it is constructed. One deserialized from the startup snapshot, like the registry AbortSignal.timeout() uses, was constructed when the snapshot was built and so has none, and its cleanup task was posted from inside the GC and run at a point which depends on when the GC ran. Give a registry without an id one on its first register() call, when it has no cells yet, so that it is tracked like any other from then on. When the GC still finds cleared cells of a registry without an id (the feature is off, or the registry was populated before it could be adopted), leave them uncollected instead of running the cleanup at a GC-dependent point, and report it as a diagnostic once. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> (cherry picked from commit cf7518182391241161b1981647add940da5c7e9b) * Keep the replay isolate data until the heap tear down has started Isolate::Deinit reset it before debug()->Unload() and the wasm compile job cleanup, which can still allocate and so run a GC. A mark-compact in that window which cleared a tracked WeakRef's target called ReplayWeakRefs::OnTargetCleared on the reset data. Reset it after Heap::StartTearDown, after which no GC runs and before the global handles its v8::Globals need are destroyed, and have OnTargetCleared tolerate missing data like ReplayGCPoll::Poll does. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> (cherry picked from commit ba9caeffae56707d94b22e2b1b174845e5da4bca) * Skip the WeakRef and FinalizationRegistry runtime calls when not recording or replaying Port of replayio/chromium-v8#343, first commit. WeakRef.prototype.deref(), the WeakRef constructor, the FinalizationRegistry constructor and register() each called a RecordReplay* runtime function unconditionally. Outside record/replay those functions do nothing, but the builtin-to-runtime transition costs every caller: deref() went from an inline field load to a runtime call. The process-wide recording flag is now readable by generated code through ExternalReference::record_replay_is_recording_or_replaying, and the builtins only call the runtime when it is set. When it is clear, deref() loads the target inline as upstream does and the record/replay ids stay 0, which the runtime side already treats as untracked. The flag is set by recordreplay::SetRecordingOrReplaying before any isolate exists and never changes, and it is per process like recordreplay::IsRecordingOrReplaying(). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * Return a WeakRef target as it is when events are unavailable Port of replayio/chromium-v8#343, second commit. Runtime_RecordReplayWeakRefDeref recorded the target's liveness even where the driver cannot record or replay a value, e.g. while a pause evaluates an expression which calls deref(). The driver passed the value through, so the result was the current target either way, but it added a recording warning on every call. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * Say "deterministic point", not "point which replays" What the record/replay bookkeeping needs is a point whose behaviour is semantically deterministic between recording and replay, where the same state is reached and the same decisions are made, so that a value recorded or cleanup run there reproduces. "A point which replays" named it badly: GC points replay too, at other places. Comments and diagnostics only. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Before this commit,
make test-addonsand the targets that depend onit (test-ci, test-all) would always do a full rebuild of the files in
test/addons and the addons scraped from doc/api/addons.markdown.
This commit introduces a proper dependency chain so that files are only
rebuilt when changed, shaving off about 10-20 seconds from each run.
R=@chrisdickinson