Skip to content

Replaced libnode with V8 #1414

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 1 commit into
base: conan
Choose a base branch
from
Draft

Conversation

AnotherFoxGuy
Copy link
Contributor

@AnotherFoxGuy AnotherFoxGuy commented Apr 8, 2025

This pr replaces libnode with https://github.com/bnoordhuis/v8-cmake
This should improve build times for the dependencies

Todo

  • Test on Windows
  • Test on Linux

@JulianGro
Copy link
Member

This failing to build on Linux already kind of shows its downside. NodeJS has very long term support for their stuff; As far as I can tell, they still support the LTS version we use (last update two weeks ago). The v8 version you are pulling, either doesn't support C++17 yet, or doesn't support GCC13 yet. To support this, we would need to keep up with v8-cmake, instead of being able to take a year or two by staying on libnode LTS.

I get that libnode currently only builds on one core on Windows, but personally I would prefer that to the additional maintenance burden that not having libnode's quick updates to LTS would entail. Besides, the plan was prebuilding it on the CI anyway, right? Short term or mid term, we could just prebuild it on the CI “manually” instead of using fancy tools. (Just by having a Workflow for only libnode, since we currently only support like 4 different versions of it [vs-2019, vs-2022, debug, release].)

@ksuprynowicz
Copy link
Member

I'd say both have their advantages. For the libnode there are the ones Julian mentioned, and for v8-cmake the advantage is much simpler build system, which could maybe make it easier to build on weird platforms like Android?
I'd sat that we would have to benchmark it before merging to see if there's a performance loss, and to see if things like profiling work.

@ksuprynowicz
Copy link
Member

Maybe this PR should be targeted towards Conan branch? Right now it's targeted towards master branch, so it shows 298 files changed.

@ksuprynowicz
Copy link
Member

This could be really useful for Android, but there's also extra maintenance burden - one thing we need to be really cautious of is that CVEs in libnode are still being addressed even for old LTS versions.
Here this necessity would be on us. Does the PR currently use newest version of V8?

@JulianGro
Copy link
Member

@ksuprynowicz it is an old version of v8. (Probably slightly newer v8 than what we use in libnode, but minus all the current patches.)
Retargeting against the Conan branch doesn't really change much, as the Conan branch is still actively being worked on. The commit history here will become a mess again as soon as we rebase or rewrite the Conan branch.

Personally, I don't think Android is a good argument here. Please do correct me if I am wrong: We don't have anyone willing to tackle Android; We don't know if libnode is in fact hard to build for Android; And we don't know if v8-cmake even builds for Android either. (I don't know if libnode works on Android, but there are at least pull requests fixing issues there and people reporting issues as well.)

Also, speed wise, it looks like this would only really be faster on Windows? I come to this conclusion because it looks to me like we still build all of v8.

@ksuprynowicz
Copy link
Member

ksuprynowicz commented Apr 11, 2025

Does Conan allow us to supply prebuilt libnode on Windows?

@JulianGro
Copy link
Member

Does Conan allow us to supply prebuilt libnode on Windows?

Yes, that is the plan for almost all dependencies.

@AnotherFoxGuy AnotherFoxGuy changed the base branch from master to conan April 12, 2025 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants