denoland
Namespace Abseil to prevent symbol collisions across dependencies
Problem
In projects that integrate rusty_v8 (as we do through Deno) with other Rust crates that have C/C++ dependencies, conflicts can arise when multiple dependencies use Abseil but at different versions. Since Cargo doesn't track or version C/C++ dependencies, this leads to symbol collisions during linking, or worse: dependencies being linked to the one selected version of Abseil, which is wrong and not ABI compatible for at least one of call-sites, resulting in runtime crashes. These issues are inconsistent and depend on factors like the OS and linker behavior. This is a broader challenge in the Rust ecosystem when mixing Rust and C/C++ libs, but it's particularly acute for large projects pulling in diverse dependencies (e.g., AI inference libraries that also rely on Abseil as in our case).
Solution
This PR activates Abseil's inline namespace option, wrapping it in a custom namespace within rusty_v8. This changes the symbol names, effectively isolating rusty_v8's Abseil usage and preventing collisions with other versions of Abseil from external dependencies.
Testing and Validation
We've implemented this change in our fork of rusty_v8 (see the comparison here: https://github.com/denoland/rusty_v8/compare/main...coasys:rusty_v8:v0.137.1-abseil-namespaced) and confirmed it resolves the crashes in our production builds on macOS and Linux. Our fork includes additional modifications to support our custom build systems, but this PR isolates only the namespacing change for cleaner upstream integration.
Why Upstream?
rusty_v8 provides prebuilt binaries, which we'd prefer to use directly from crates.io instead of maintaining a fork. Building rusty_v8 from source is challenging, especially on Windows where we've struggled to get consistent results. Merging this would allow us (and potentially others facing similar issues) to drop our fork and rely on official releases.
If there are any concerns or suggestions for improvements, happy to iterate!
IIRC crates.io will not accept publishing this due to it modifying its own source. Can you double check with cargo publish --dry-run?
IIRC crates.io will not accept publishing this due to it modifying its own source. Can you double check with
cargo publish --dry-run?
Yes, that's right - it complains about uncommitted changes:
cargo publish --dry-run
Updating crates.io index
warning: crate [email protected] already exists on crates.io index
error: 2 files in the working directory contain changes that were not yet committed into git:
base/trace_event/common/trace_event_common.h
third_party/abseil-cpp/absl/base/options.h
to proceed despite this and include the uncommitted changes, pass the `--allow-dirty` flag
and as it says, with --allow-dirty it won't bother:
cargo publish --dry-run --allow-dirty
Updating crates.io index
warning: crate [email protected] already exists on crates.io index
Packaging v8 v139.0.0 (/Users/nicolasluck/Coasys/code/rusty_v8)
Updating crates.io index
Packaged 12874 files, 151.6MiB (32.7MiB compressed)
Verifying v8 v139.0.0 (/Users/nicolasluck/Coasys/code/rusty_v8)
warning: file `/Users/nicolasluck/Coasys/code/rusty_v8/target/package/v8-139.0.0/build.rs` found to be present in multiple build targets:
* `integration-test` target `build`
* `build-script` target `build-script-build`
Blocking waiting for file lock on package cache
Blocking waiting for file lock on package cache
Compiling proc-macro2 v1.0.101
Compiling unicode-ident v1.0.18
Compiling stable_deref_trait v1.2.0
...
The failed CI step is not during publishing though, but at the Install Rust step:
Run : create cachekey
info: syncing channel updates for '1.86.0-aarch64-apple-darwin'
error: failed to download file error=Reqwest(reqwest::Error { kind: Request, url: "https://static.rust-lang.org/dist/channel-rust-1.86.0.toml.sha256", source: hyper_util::client::legacy::Error(Connect, ConnectError("dns error", Custom { kind: Uncategorized, error: "failed to lookup address information: nodename nor servname provided, or not known" })) })
error: could not download file from 'https://static.rust-lang.org/dist/channel-rust-1.86.0.toml.sha256' to '/Users/runner/.rustup/tmp/cetlrh5cp092jz5v_file'
Caused by:
0: error downloading file
1: error sending request for url (https://static.rust-lang.org/dist/channel-rust-1.86.0.toml.sha256)
2: client error (Connect)
3: dns error: failed to lookup address information: nodename nor servname provided, or not known
4: failed to lookup address information: nodename nor servname provided, or not known
Error: Process completed with exit code 1.
Was this just a transient network glitch?
looks like a transient network issue yes. wrt publishing, I don't think we can accept using --allow-dirty, just to ensure the integrity of what we are publishing.
What would be a viable strategy then? Forking Abseil and pointing the submodule to a commit with that change?
That's much harder as there are multiple and deeper down the dependency graph. Also the crashes that got us investigating happened during Deno initialization.
I believe using that inline namespace option is definitely a benefit for rusty_v8 since others will likely bump into the same issue at some point too.
And being explicitly a Rust wrapper for v8, I'd say it makes more sense to have that wrapping being done well in here.
Just committed another suggestion to avoid either breaking cargo publish or requiring --allow-dirty. I've added the changed options.h to the exclude list in Cargo.toml. I guess we could even put the whole submodule directory in that list since the crate won't need any C++ sources if I'm not mistaken?
Yes, and it's coming in through the abseil submodule so it's there when building from source. Just won't be included in the crates.io publish.
Yes I'm saying, building the crates.io release from source is currently a supported feature.
Ah right, building crates.io package from source. Sorry, didn't think about that.
Well there is another option: having the build script, or maybe rather the CI script, commit the changed file just before running cargo publish. That way cargo doesn't regard it as dirty. If that only happens in the context of running the build for publishing and doesn't get pushed it wouldn't hurt anything else.
Would that be a viable option for you folks? Happy to look into committing the needed change here. Would that be the ci/publish workflow?
So it looks like you are currently running it with --allow-dirty: cargo publish -vv --locked --allow-dirty
https://github.com/denoland/rusty_v8/blob/1bc16604555847e020945abe39b8d4b2fec5dd9e/.github/workflows/ci.yml#L323
Ok, after format and clippy fixes CI tests pass except for debug x86_64 darwin which looks like it was cancelled after 180 minutes while still running.
Are you fine with my added task in the publish workflow which should make the cargo publish work even when you take out the --allow-dirty in the future?
@devsnek, I believe my last changes address your concern. Is there anything more I can do to help get this merged?