Repository navigation
Conversation
aduh95
added
the
semver-major
|
Review requested:
|
nodejs-github-bot
added
build
This comment was marked as resolved.
This comment was marked as resolved.
|
@aduh95, I'll look into Windows failures. |
|
This is what got it to compile for me locally. In short, it was a configuration problem(s), so gyp changes resolved it all. DetailsFrom f31ec1fc602e9711756d9ca1d7ae386221e14662 Mon Sep 17 00:00:00 2001
From: StefanStojanovic <stefan.stojanovic@janeasystems.com>
Date: Wed, 9 Sep 2026 08:24:06 +0200
Subject: [PATCH] build,win: fix Perfetto
Signed-off-by: StefanStojanovic <stefan.stojanovic@janeasystems.com>
---
deps/perfetto/perfetto.gyp | 8 ++++++++
node.gypi | 5 +++++
tools/v8_gypfiles/v8.gyp | 6 +++++-
3 files changed, 18 insertions(+), 1 deletion(-)
diff --git a/deps/perfetto/perfetto.gyp b/deps/perfetto/perfetto.gyp
index 083d0b386dd..697256401a3 100644
--- a/deps/perfetto/perfetto.gyp
+++ b/deps/perfetto/perfetto.gyp
@@ -18,6 +18,14 @@
'sources': [
'<@(perfetto_sdk_sources)',
],
+ 'conditions': [
+ ['OS=="win"', {
+ 'defines': [
+ 'WIN32_LEAN_AND_MEAN',
+ 'NOMINMAX',
+ ],
+ }],
+ ],
},
]
}
diff --git a/node.gypi b/node.gypi
index 104e884e661..1ca77fa3fee 100644
--- a/node.gypi
+++ b/node.gypi
@@ -93,6 +93,11 @@
[ 'OS=="linux" and target_arch=="s390x"', {
'cflags': [ '-march=z14' ],
}],
+ [ 'v8_use_perfetto==1', {
+ 'dependencies': [
+ 'deps/perfetto/perfetto.gyp:perfetto_sdk',
+ ],
+ }],
[ 'node_enable_d8=="true"', {
'dependencies': [ 'tools/v8_gypfiles/d8.gyp:d8' ],
}],
diff --git a/tools/v8_gypfiles/v8.gyp b/tools/v8_gypfiles/v8.gyp
index a976148ddab..2b1ee51c8f1 100644
--- a/tools/v8_gypfiles/v8.gyp
+++ b/tools/v8_gypfiles/v8.gyp
@@ -1758,7 +1758,11 @@
}],
['v8_use_perfetto==1', {
'sources!': [
- '<!@pymod_do_main(GN-scraper "<(V8_ROOT)/BUILD.gn" "\\"v8_libplatform.*?v8_use_perfetto.*?sources \\-= ")',
+ '<(V8_ROOT)/src/libplatform/tracing/trace-buffer.cc',
+ '<(V8_ROOT)/src/libplatform/tracing/trace-buffer.h',
+ '<(V8_ROOT)/src/libplatform/tracing/trace-writer.cc',
+ '<(V8_ROOT)/src/libplatform/tracing/trace-writer.h',
+ '<(V8_ROOT)/src/tracing/trace-event-no-perfetto.h',
],
'sources': [
'<!@pymod_do_main(GN-scraper "<(V8_ROOT)/BUILD.gn" "\\"v8_libplatform.*?v8_use_perfetto.*?sources \\+= ")',
--
2.45.2.windows.1
|
This comment was marked as outdated.
This comment was marked as outdated.
|
@nodejs/platform-aix Build failure: |
I think the actual error is this: Same failure is seen on SmartOS. |
|
doh! |
|
How should we proceed? Force |
|
good question. I think as long as the observable behavior with trace events is still the same either way, forcing |
8e59ba9 to
53d853b
Compare
aduh95
added
the
tsc-agenda
|
Adding to the TSC agenda to make sure 53d853b has consensus (the alternative would be to block Perfetto enabling until support for all platforms is added upstream) |
This comment was marked as off-topic.
This comment was marked as off-topic.
1 similar comment
This comment was marked as off-topic.
This comment was marked as off-topic.
legendecas
added
the
trace_events
The reason that we built with perfetto sdk only was that the full perfetto library has a lot of dependencies (specifically in trace_processor). The Node.js runtime only needs the data producer part (i.e. perfetto_sdk). If we could setup an independent build (outside of the node.js core build), I think it is acceptable to take in all of perfetto dependencies. But I'd prefer not to include them in |
|
For reference, I tried cross compiling Cross-compiling for ppc64le works at least, that could be an option. |
Signed-off-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
Refs: google/perfetto#7590 Signed-off-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
Signed-off-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
except on incompatible platforms
add TRACE_PROCESSOR_SHELL_PATH as dependency of JS test
allow /dev/null value to opt-out of trace_processor tests
As mentioned in this comment only the emitter/tracer was patched on big-endian, we are not currently planning to fix the processor as it's not needed for emitting traces on Node.js or V8. |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
I'm testing cross-compiling on GHA, store the result on Cachix, and have Jenkins download it from there; I've opened google/perfetto#8256 to get s390x support upstream, we should likely wait for that to land before progressing (looks like even our CI is not happy with it, so I guess it will require more work). Never attempts to download it:
Fails to download:
Download but fails to run:
|
Signed-off-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
In nodejs/node#65794, we're trying to test our Perfetto integration on several platforms, one of which is s390x (support was added by #7449) but the cross-compilation fails when it reaches little-endian assertions. I've tasked an LLM-agent to make a patch, and here's what it came up with. I've validated that I was able to cross-compile for s390x.
|
I need to clear a few questions/assumptions about this PR, specially around big-endian support: Node.js itself only uses the tracer/emitter correct? It doesn't use the perfetto analyzer. I'm also assuming the prebuilt analyzer is being downloaded for x64 and arm64 and compiled for ppc64 and s390x? I have compiled V8 on s390x native which includes this BE patch with Meaning the patch is not complete and the analyzer (aka. Having said all that, my question is, isn't it better to fix/rewrite the Node.js tests to use the perfetto output directly instead of relying on the analyzer to do the json conversions? This way we avoid dragging this (partially broken) tool in Node.js just for doing a conversion. |
Yes, Node.js core only uses emitter/producer. The perfetto
Producing perfetto json output would require a bigger runtime dependencie, like libjsoncpp, protobuf etc, essentially building the analyzer into the runtime. I would prefer not introducing these analyzer dependencies for the core. |
Refs: nodejs/diagnostics#654