Sitelet https://github.com/nodejs/node/pull/335
Skip to content

build: rebuild test addons conditionally - #335

Closed
bnoordhuis wants to merge 1 commit into
nodejs:v1.xfrom
bnoordhuis:fix-make-test-addons
Closed

bnoordhuis wants to merge 1 commit into
nodejs:v1.xfrom
bnoordhuis:fix-make-test-addons

Conversation

@bnoordhuis

Copy link
Copy Markdown
Member

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.

R=@chrisdickinson

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.
Comment thread Makefile

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it doesn't look like addon-verify is run, so those doc tests won't be built?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Which I'm fine with for now, by the by, I'm not a huge fan of doctests.)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, perhaps I made it too obtuse for the sake of DRY. In the line below, $< is replaced with tools/doc/addon-verify.js.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@chrisdickinson

Copy link
Copy Markdown
Contributor

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.

@jbergstroem

Copy link
Copy Markdown
Member

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 test-ci. How can we proceed? @chrisdickinson could you possibly guide from an assumed empty repo?

@mscdex mscdex added the build Issues and PRs related to Node.js builds or CI infrastructure. label Mar 22, 2015
@bnoordhuis

Copy link
Copy Markdown
Member Author

Closing, stale.

@bnoordhuis bnoordhuis closed this Jun 26, 2015
Andarist added a commit to replayio/node that referenced this pull request Oct 2, 2026
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>
Andarist added a commit to replayio/node that referenced this pull request Oct 7, 2026
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

build Issues and PRs related to Node.js builds or CI infrastructure.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants