build,deps,src: bring back v8 abseil-cpp#349
build,deps,src: bring back v8 abseil-cpp#349santigimeno wants to merge 1 commit intonode-v22.x-nsolid-v5.xfrom
Conversation
WalkthroughThis set of changes updates build configuration files to rename the Abseil dependency from Changes
Sequence Diagram(s)Not applicable: Only build configuration and include directive changes, no new or modified control flow. Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (9)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (5)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
agents/grpc/src/grpc_agent.cc (1)
73-75: Fix misleading size comment (4 GB → 4 MB).
4L * 1024 * 1024equals 4 MB, not 4 GB. The constant currently matches the default gRPC message size limit; the comment should reflect that to avoid confusion.-const size_t GRPC_MAX_SIZE = 4L * 1024 * 1024; // 4GB +const size_t GRPC_MAX_SIZE = 4L * 1024 * 1024; // 4MB (default gRPC max)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
agents/grpc/src/grpc_agent.cc(1 hunks)deps/protobuf/third_party/abseil-cpp/absl/debugging/internal/symbolize.h(1 hunks)deps/protobuf/third_party/abseil-cpp/absl/debugging/symbolize_elf.inc(1 hunks)node.gyp(5 hunks)src/nsolid/nsolid_api.h(0 hunks)tools/v8_gypfiles/v8.gyp(8 hunks)
💤 Files with no reviewable changes (1)
- src/nsolid/nsolid_api.h
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: santigimeno
PR: nodesource/nsolid#339
File: src/nsolid/nsolid_elf_utils.cc:18-18
Timestamp: 2025-07-08T16:05:45.341Z
Learning: In the NSolid project, the `GetBuildId` function in `src/nsolid/nsolid_elf_utils.cc` is designed to be called only from the NSolid thread, so the static cache doesn't require thread safety mechanisms like mutex protection.
agents/grpc/src/grpc_agent.cc (3)
Learnt from: santigimeno
PR: #327
File: test/addons/nsolid-code-event-hook/binding.cc:64-78
Timestamp: 2025-06-10T09:52:49.401Z
Learning: In the nsolid codebase, test files in test/addons/ consistently use assert() for input validation and runtime checks, not CHECK() macros. This is the established testing convention.
Learnt from: santigimeno
PR: #339
File: src/nsolid/nsolid_elf_utils.cc:18-18
Timestamp: 2025-07-08T16:05:45.341Z
Learning: In the NSolid project, the GetBuildId function in src/nsolid/nsolid_elf_utils.cc is designed to be called only from the NSolid thread, so the static cache doesn't require thread safety mechanisms like mutex protection.
Learnt from: santigimeno
PR: #339
File: test/addons/nsolid-elf-utils/binding.cc:13-28
Timestamp: 2025-07-08T14:48:04.827Z
Learning: In nsolid test native addons (e.g., test/addons/*/binding.cc), additional JavaScript-facing argument validation is typically omitted because the tests supply well-formed inputs.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: test-macOS
- GitHub Check: build-tarball
- GitHub Check: lint-js-and-md
- GitHub Check: build-docs
- GitHub Check: test-linux (ubuntu-24.04)
- GitHub Check: test-linux (ubuntu-24.04-arm)
- GitHub Check: coverage-windows
🔇 Additional comments (7)
agents/grpc/src/grpc_agent.cc (1)
6-6: Header include looks appropriate.Adding the explicit
nsolid/continuous_profiler.hinclude is the right move now that the dependency is no longer transitively pulled in bynsolid_api.h.deps/protobuf/third_party/abseil-cpp/absl/debugging/symbolize_elf.inc (1)
1759-1764: AbslInternalGetFileMappingHint removal is safeSearch results confirm that the C wrapper is never called elsewhere in this codebase, and the underlying C++
GetFileMappingHintremains implemented and in use:• No occurrences of
AbslInternalGetFileMappingHintoutside its own definition in
deps/protobuf/third_party/abseil-cpp/absl/debugging/symbolize_elf.inc(lines 1759–1764)
•absl::debugging_internal::GetFileMappingHintis still declared in
deps/protobuf/third_party/abseil-cpp/absl/debugging/internal/symbolize.hand defined in
symbolize_elf.inc
• Other code paths callGetFileMappingHint(...)directlyThis change cleanly disables an unused extern "C" shim without impacting functionality.
deps/protobuf/third_party/abseil-cpp/absl/debugging/internal/symbolize.h (1)
149-151: LGTM! Consistent with implementation file changes.The function declaration is properly commented out, maintaining consistency with the corresponding definition being disabled in
symbolize_elf.inc. This coordinated approach prevents compilation errors that could arise from having a declaration without a definition.node.gyp (4)
416-532: LGTM! Good modularization of source files.The reorganization separating agent sources from nsolid sources improves build modularity and aligns with the PR objective of bringing back v8 abseil-cpp. The variable renaming from
nsolid_sourcestoagent_sourcesand the creation of a newnsolid_sourceslist clearly separates concerns.
982-982: LGTM! Abseil dependency path updated to v8 version.The change from
deps/protobuf/abseil.gyp:abseiltotools/v8_gypfiles/abseil.gyp:abseilcorrectly implements the PR objective of bringing back v8 abseil-cpp for the main node library and cctest targets.Also applies to: 1350-1350
984-984: LGTM! Proper dependency on the new libagents target.Adding the
libagentsdependency to the main node library target correctly establishes the build relationship after modularizing the agent code.
1181-1208: LGTM! Well-structured intermediate library target.The
libagentstarget is properly configured with appropriate include directories, dependencies, and build settings. This modularization will improve build organization and maintainability.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
deps/grpc/grpc.gyp(4 hunks)deps/opentelemetry-cpp/otlp-http-exporter.gyp(1 hunks)deps/protobuf/abseil.gyp(2 hunks)deps/protobuf/protobuf.gyp(1 hunks)deps/protobuf/utf8_range.gyp(1 hunks)node.gyp(5 hunks)tools/v8_gypfiles/v8.gyp(9 hunks)
✅ Files skipped from review due to trivial changes (3)
- deps/protobuf/protobuf.gyp
- deps/opentelemetry-cpp/otlp-http-exporter.gyp
- tools/v8_gypfiles/v8.gyp
🚧 Files skipped from review as they are similar to previous changes (3)
- deps/protobuf/abseil.gyp
- deps/grpc/grpc.gyp
- node.gyp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: test-macOS
- GitHub Check: build-tarball
- GitHub Check: test-linux (ubuntu-24.04-arm)
- GitHub Check: test-linux (ubuntu-24.04)
- GitHub Check: coverage-windows
- GitHub Check: build-docs
- GitHub Check: lint-js-and-md
| 'type': 'static_library', | ||
| 'dependencies': [ | ||
| './abseil.gyp:abseil', | ||
| './abseil.gyp:abseil_proto', |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the target rename is consistent across the build system.
The dependency update from abseil to abseil_proto looks correct and aligns with the PR's objective to reintroduce v8 abseil-cpp. However, ensure that the corresponding target in abseil.gyp has been properly renamed and all dependent build targets have been updated consistently.
🏁 Script executed:
#!/bin/bash
# Description: Verify that the abseil_proto target exists and all references are consistent
# Check if abseil_proto target exists in abseil.gyp
echo "Checking for abseil_proto target definition:"
rg -A 5 "target_name.*abseil_proto" deps/protobuf/abseil.gyp
# Check for any remaining references to the old 'abseil' target
echo -e "\nChecking for any remaining references to old 'abseil' target:"
rg "abseil\.gyp:abseil[^_]" --type gyp
# Check all files that reference abseil_proto to ensure consistency
echo -e "\nChecking all references to abseil_proto:"
rg "abseil\.gyp:abseil_proto" --type gypLength of output: 728
🏁 Script executed:
#!/bin/bash
# Description: Re-verify that no old 'abseil' references remain and all 'abseil_proto' usages are consistent
echo "Checking for any remaining references to the old 'abseil' target (excluding abseil_proto):"
rg -P "abseil\.gyp:abseil(?!_proto)" -g "*.gyp" -g "*.gypi" || echo "No old references found."
echo -e "\nChecking all references to 'abseil_proto':"
rg "abseil.gyp:abseil_proto" -g "*.gyp" -g "*.gypi" || echo "No abseil_proto references found."Length of output: 1485
Update remaining abseil target references to abseil_proto
The search shows several build files still depending on the old abseil target. To keep the rename consistent and avoid build failures, please update these references:
• node.gyp
• tools/v8_gypfiles/d8.gyp
• tools/v8_gypfiles/v8.gyp
Each line like
'…/abseil.gyp:abseil',
should be changed to
'…/abseil.gyp:abseil_proto',
🤖 Prompt for AI Agents
In deps/protobuf/utf8_range.gyp at line 7 and similarly in node.gyp,
tools/v8_gypfiles/d8.gyp, and tools/v8_gypfiles/v8.gyp, update all references
from '…/abseil.gyp:abseil' to '…/abseil.gyp:abseil_proto' to maintain
consistency with the renamed target and prevent build errors.
santigimeno
force-pushed
the
santi/fix_abseil
branch
5 times, most recently
from
2e35f87 to
65716d2
Compare
Make sure v8 is built with its own abseil-cpp version while the other dependencies use the one from protobuffer. The actual fix was avoid using the same name for the abseil target as it doesn't work on Windows.
santigimeno
force-pushed
the
santi/fix_abseil
branch
from
65716d2 to
4fda0a1
Compare
Make sure v8 is built with its own abseil-cpp version while the other dependencies use the one from protobuffer. The actual fix was avoid using the same name for the abseil target as it doesn't work on Windows. PR-URL: #349 Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
|
Landed in cd19746 |
2 participants
Make sure v8 is built with its own abseil-cpp version while the other
dependencies use the one from protobuffer.
In order to avoid name collisions when building
libnsolid, a newlibagentstarget has been added.Also, commented
AbslInternalGetFileMappingHintfrom protobuf'sabseil-cpp to avoid collisions, as it's defined outside a namespace.
Summary by CodeRabbit