Skip to content
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

Build node with GN #21410

Closed
nornagon opened this issue Jun 19, 2018 · 57 comments
Closed

Build node with GN #21410

nornagon opened this issue Jun 19, 2018 · 57 comments
Labels
build Issues and PRs related to build files or the CI. question Issues that look for answers.

Comments

@nornagon
Copy link
Contributor

Electron is starting to move its build system from GYP to GN to align with Chromium, which will make rolling Chromium substantially easier. However, we still have to shell out to GYP to build node.js, which is a lot of bridging code to maintain. I see there have been prior conversations about porting node's build system to GN from GYP, but they seem to have petered out without conclusion.

Would node.js be willing to accept patches to switch from GYP to GN?

NB, not suggesting that node does such a thing for 3rd-party native modules, just for node's own build.

@bnoordhuis
Copy link
Member

bnoordhuis commented Jun 19, 2018

That's already possible with ./configure --build-v8-with-gn.

edit: unless you mean porting all of the build system over. The answer is 'no' in that case.

@bnoordhuis bnoordhuis added question Issues that look for answers. build Issues and PRs related to build files or the CI. labels Jun 19, 2018
@nornagon
Copy link
Contributor Author

Sorry, I'm not referring to building v8 with GN, I'm referring to building node.js itself with GN.

@bnoordhuis
Copy link
Member

Don't know if you saw my ninja edit but I think it's safe to say a switch to GN is not on the table.

@nornagon
Copy link
Contributor Author

Ah, no, I missed it (would be awesome if GH would refresh edits live). Could you expand on why that's the case? (Is there perhaps a document describing blockers/reasoning that you could refer me to?)

@bnoordhuis
Copy link
Member

See nodejs/TSC#464 which touches on that.

Speaking for myself, getting burned by a made-by-Google-for-Google build system once is enough; the next one should be one with broad community support.

@mhdawson
Copy link
Member

Based on earlier discussion Google said they don't support other projects using GN and recommended against it.

@joyeecheung
Copy link
Member

joyeecheung commented Jun 19, 2018

We are basically left with 3 choices after gyp:

  • Blaze: Google uses that (Blaze) internally so they can support it, see RFC: Bazel for build system TSC#464 for a breakdown. It's a Java tool although it has distributions with JDK bundled. Would be the most difficult to transition the addon tooling into.
  • GN: Currently used by V8 but people from the V8/Chromium/GN side all recommended against it in RFC: Bazel for build system TSC#464 due to support issues, it's also coupled in depot_tools and platform support is limited. There is already ./configure --build-v8-with-gn in our code base though. Future of addon tooling is bleak.
  • CMake: We'll have to port the build files over and maintain them by ourselves (I guess we would need a revamp of [request] cmake build job build#1003) No additional dependencies other than cmake itself. If we want to transition the addon tooling in the future as well there is already cmake.js

See https://docs.google.com/document/d/101BP4BpZoP4tsMGo4j_MhoyLv169-2Oq_HeyWykCNGc/edit (OP of that TSC issue) for a breakdown

@bnoordhuis
Copy link
Member

Also apropos cmake, I'm reasonably confident I can teach node-gyp to convert a binding.gyp to CMakeLists.txt on the fly and effectively phase out GYP.

@nornagon
Copy link
Contributor Author

It's totally possible to use GN without depot_tools, see e.g. https://github.com/yue/build-gn. GN works fine on win/mac/linux, not sure about arm64 support but chromium itself builds against arm64: e.g.. @joyeecheung can you expand on what you mean by "platform support is limited"? Also not sure what you mean by "addon tooling".

@joyeecheung
Copy link
Member

joyeecheung commented Jun 19, 2018

Regarding OP's request:

Would node.js be willing to accept patches to switch from GYP to GN?

I think patches for GN support can be accepted as long as they are not the default flow of building Node.js and are more in a situation similar to our Android builds (self-serve). If Electron needs GN to build Chromium then building Node.js with GN would be the most viable option for Electron, but it is not the case for Node.js itself and we have been explicitly advised against using it from the V8/Chromium/GN team.

@joyeecheung
Copy link
Member

It's totally possible to use GN without depot_tools, see e.g. https://github.com/yue/build-gn.

Didn't know that, good to hear, thanks

GN works fine on win/mac/linux, not sure about arm64 support but chromium itself builds against arm64: e.g.. @joyeecheung can you expand on what you mean by "platform support is limited"?

https://docs.google.com/document/d/101BP4BpZoP4tsMGo4j_MhoyLv169-2Oq_HeyWykCNGc/edit touched on that a little bit (regarding FreeBSD and AIX). See supported platforms for the full list of platforms and tiers that we support.

Also not sure what you mean by "addon tooling".

I meant providing support for building 3rd-party native modules (as mentioned in the OP) with one of those tools and transitioning the ecosystem into it.

@bnoordhuis
Copy link
Member

platform support is limited

Little to no freebsd/openbsd/etc. support. You're basically restricted to what Google is willing to support.

I think patches for GN support can be accepted

Supporting multiple build systems is already a hassle for libuv, and that's a fairly simple project to build.

I don't see it happening for Node.js. Without major ongoing maintenance it bitrots in no time.

@joyeecheung
Copy link
Member

Supporting multiple build systems is already a hassle for libuv, and that's a fairly simple project to build.
I don't see it happening for Node.js. Without major ongoing maintenance it bitrots in no time.

I agree, but Idon't see any harm if we do not support GN, just allowing the build files to exist in the code base and get maintained by people who need them (e.g. Electron) though.

@bnoordhuis
Copy link
Member

I'm skeptical that works. If it's there, people will use it and complain when it's broken.

As well, someone needs to review those updates. And thus us maintainers' workloads expand ever further towards infinity.

@joyeecheung
Copy link
Member

I'm skeptical that works. If it's there, people will use it and complain when it's broken.
As well, someone needs to review those updates. And thus us maintainers' workloads expand ever further towards infinity.

Would it help if we make the configure option prints stuff (maybe in red) about this being not supported by Node.js core and do not document it/document about the lack of support?

(Just saying...or am I?)

@bnoordhuis
Copy link
Member

You could cat an .au to /dev/dsp where we scream at the top of our lungs "DO NOT USE!" and still people will file bugs.

We already print warnings. People even include them in their bug reports, then later admit they didn't see them. It's hopeless.

@joyeecheung
Copy link
Member

joyeecheung commented Jun 19, 2018

We already print warnings. People even include them in their bug reports, then later admit they didn't see them. It's hopeless.

Yeah, but for someone who even knows what GN is, and wants to use it to build Node, and goes through the docs to find the configure option that does it, they are probably very unlikely to ignore the warnings. Also there probably would not be that many people who want to do it (I guess building Electron could lead you to that but how many people actually builds Electron along with Chromium and Node.js?)

@bnoordhuis
Copy link
Member

I expect the answer is "many" because of how many downstream users electron has. My cmake branch hasn't even been merged yet and there are already at least two people using it.

@nornagon
Copy link
Contributor Author

btw, it seems like cmake doesn't support smartos, freebsd or AIX officially either: https://open.cdash.org/index.php?project=CMake

@aduh95
Copy link
Contributor

aduh95 commented Jul 31, 2018

@nornagon Actually it seems that smartos is the only one not officially supported by CMake (https://gitlab.kitware.com/cmake/cmake):

Supported Platforms

  • Microsoft Windows
  • Apple macOS
  • Linux
  • FreeBSD
  • OpenBSD
  • Solaris
  • AIX

Other UNIX-like operating systems may work too out of the box, if not
it should not be a major problem to port CMake to this platform.
Subscribe and post to the CMake Users List to ask if others have
had experience with the platform.

@tojocky
Copy link

tojocky commented Sep 23, 2018

Fuchsia build system uses GN: https://github.com/fuchsia-mirror/docs/blob/master/development/build/overview.md
Zircon is built with GNU Make.

Also GN team did an effort to isolate GN from chromium code: https://gn.googlesource.com/gn/

@codebytere
Copy link
Member

codebytere commented Oct 12, 2018

@hashseed we've done work locally on this and have all the necessary build files in place for building Node with GN as part of Electron here. There are a few more fixup commits after that but that's the bulk!

@refack
Copy link
Contributor

refack commented Oct 14, 2018

we've done work locally on this and have all the necessary build files in place for building Node with GN as part of Electron here. There are a few more fixup commits after that but that's the bulk!

It was discussed several times before, and IMHO GN is not the best way forward.

  1. We still use GYP for our native addons. Switching to GN will essentially abandon them. The status there is not good as is.
  2. From my experience GN works where Google has got it to work. It's a much less mature system then GYP (or CMake)
  3. We have a lot of institutional knowledge both in GYP and in the .gyp files.
  4. Using GYP does not require anything other then python, which we are already dependant on.

@ofrobots
Copy link
Contributor

ofrobots commented Oct 14, 2018 via email

@joyeecheung
Copy link
Member

joyeecheung commented Oct 14, 2018

We still use GYP for our native addons. Switching to GN will essentially abandon them. The status there is not good as is.

I believe the request has been switched to "adding GN files in core, in parallel with GYP files", not "replacing GYP files with GN files".

Using GYP does not require anything other then python, which we are already dependant on.

https://gn.googlesource.com/gn/+/master seems to only depends on python, ninja and the usual compiler toolchains (judging from the output of build/gen.py, it doesn't seem to be terribly difficult to remove the ninja dependency either), but again if we are not replacing GYP with GN, just adding GN files so that people can build with GN, that probably doesn't matter, whoever uses them will find a way to put GN on their PATH somehow.

On the maintenance issue, we don't support building ninja in core either, but you can still build node with ninja with build files generated from GYP files anyway, and we even have a document on how to do that. If Electron has decided to build Node.js with GN, when the GN files are broken, there will be people making sure they are fixed anyway - there is a company (and another giant company very soon) behind that project, after all. Worst case scenario, they probably won't fix the GN files for all the platforms we support and won't port all of our flags to GN, and we might potentially get bug reports from people trying to use those flags with GN files or on unsupported platforms, we can either leave the issues open, or remind them that we do not maintain those files and close the issue.

@hashseed
Copy link
Member

I believe that there is a lot of value in adding GN as alternative, but non-default build system. Reasons:

  • GN configs already exists and is maintained by the Electron project. I think it would be a big sign of goodwill to accommodate the Electron project by making integration with them a smoother experience.
  • Not requiring gyp configs could decouple the canary builds from manually updating gyp configs. This currently is done mostly by @targos and @refack.
  • Any other build system is not going to solve the add-on problem either. We should not conflate building core with building add-ons imo.
  • It should be a lot easier to build with sanitizer instrumentation with GN. From experience, sanitizer builds provide a lot of value in finding memory leaks and race conditions.

@mhdawson
Copy link
Member

I have no opposition with GN as an alternative but do want to ask on question. I think last time we talked about GN as an option the message from the v8 team was that GN was not really supported for use other than with Chrome. Has that changed?

@hashseed
Copy link
Member

I'm not convinced that checking in what will essentially be "dead code" (WRT to node core and our CI) is good practice. It might be a good will gesture towards Electron, but it is not free for us. (And I totally understand Electrons motivation, since AFAIK they need to GN build Chromium as well)

By the same argument V8 should never have added tools/gen-postmortem-metadata.py or platform support for FreeBSD, AIX, Cygwin, QNX, Solaris, and OpenBSD either. These are dead code to us too, and only maintained by whoever needs them downstream. More often than not these files get out of date, but that never caused any trouble for us.

I would consider it a success if the GN config files are kept up-to-date with only the bare minimum set of features, and not necessarily include options not used by downstream users. How about we land these files and see how things work out?

@refack
Copy link
Contributor

refack commented Oct 17, 2018

How about we land these files and see how things work out?

Wasn't it your team that removed the gyp scaffolding from V8 since it was deprecated 😉

I'm not totally opposed, I'm just not sold yet. I'm worried about maintenance, and I still don't see the upside, (if all the maintenance will be done down stream anyway) but I'm willing to listen.

Let see the PR and figure out how this is going to work.

@hashseed
Copy link
Member

Wasn't it your team that removed the gyp scaffolding from V8 since it was deprecated

touché :)

I was actually also hoping that GN would benefit Node.js as well, from being integrated, e.g. providing a wider window when upgrading V8, e.g. node canary would not be blocked on gyp configs. Also, sanitizers.

@joyeecheung
Copy link
Member

Would it make sense if we use GN to build the canary? It should be easier to port our necessary GYP changes to GN to build that (I guess the necessary changes are probably mostly file list changes, maybe a build flag or two once in a while) than to port V8 GN changes to GYP, so that when the bot bumps V8 in the canary it'll be easier to fix and we will not have all the canary releases missing until the GYP files are fixed with V8 lkgr. One downside is that we will not be notified about GYP breakage sooner, but it seems to be a lot of overhead to keep the build working with lkgr when we only land stable on master.

@targos
Copy link
Member

targos commented Oct 17, 2018

@joyeecheung I would be very happy if we could do it. It would probably spare me ~30 minutes per week.

@refack
Copy link
Contributor

refack commented Oct 17, 2018

Would it make sense if we use GN to build the canary?

Yes and no. For one we could do it today with ./configure --build-v8-with-gn. But the v8-canary is built and tested in our CI cluster which doesn't have GN...

One downside is that we will not be notified about GYP breakage sooner, but it seems to be a lot of overhead to keep the build working with lkgr when we only land stable on master.

AFAIK we don't test with LKGR, we test with the head of the new dev branches, and porting to gyp is work that needs to be done anyway. From my experience there where few instances when that work had to be undone because of V8 dev branch changes.

IMHO we could think about creating auto-porting tools. The feature set and metaphors used of GN and GYP are not that dissimilar. See @nornagon's electron/node#63
Actually I think that building a limited GYP-GN bridge transpiler that covers our use cases is not unreasonable. (RE @hashseed's mention of other GN benefits)

@refack
Copy link
Contributor

refack commented Oct 17, 2018

It would probably spare me ~30 minutes per week.

BTW: this is exactly what I was asking during the V8 gyp deprecation process. Who is going to do the porting (nodejs/TSC#411), and is part of my apprehension regarding this change. I could make the argument that the V8 team could help with gyp porting, since AFAIK they have more resources than us...

@sam-github
Copy link
Contributor

I use ninja, the gyp builds on my machine occaisonally don't build the the js2c files, for reasons I haven't been able to track down through the muck that is gyp. The ninja builds don't have this problem. So, thanks for working on them!

@refack Here's the consensus among the people sitting around the table as I understood it, I hope I don't misstate anyone.

A number of us (including me) share your concern about dead code drops. After a fair bit of discussion and airing of the problems that could occur, everyone present seemed to agree that:

  1. delivering gn via our own node repo would have advantages to multiple downstream embedders (not just electron)
  2. It could be of benefit in our own ci to run memory and other error checking builds, apparently there are some sanitization capabilities built into gn we could use even if we only did the build on linux.

Very importantly, electron committed to maintaining the gn files. Its something they do already, just out of tree. I assume if that doesn't happen and they are left to rot we would rip the gn files out again.

Electron is floating some specific to them patches they don't think they can upstream, but some more concerted effort has kicked off to get what they can upstream, I think I saw some folks clustered around laptops. There was some conversation in particular about upstreaming changes that are potentially useful for any node embedder.

@hashseed
Copy link
Member

Building a GN to GYP transpiler or vice versa for a single use case sounds like more work than worthwhile tbh.

--build-v8-with-gn works with caveats. Specifically, V8 has ICU as dependency, and Node does too. That means using the hybrid build, ICU is built twice, one shipped with Node and one shipped with gclient with V8. If the versions mismatch, we run into crashes at runtime. That has forced us to sync ICU updates between Node and Chromium. With a full GN build this would not be necessary.

@mmarchini
Copy link
Contributor

--build-v8-with-gn works with caveats. Specifically, V8 has ICU as dependency, and Node does too. That means using the hybrid build, ICU is built twice, one shipped with Node and one shipped with gclient with V8. If the versions mismatch, we run into crashes at runtime. That has forced us to sync ICU updates between Node and Chromium. With a full GN build this would not be necessary.

Does that mean we would need to use gclient sync to build node, as well as have the entire depot_tools installed?

It could be of benefit in our own ci to run memory and other error checking builds, apparently there are some sanitization capabilities built into gn we could use even if we only did the build on linux.

Sanitizers come from GN or clang? If they come from clang we could probably use them with gyp. Also, I think I saw some PRs here to add support to build node with sanitizers.

To be clear, I'm not opposed to add GN to the project, I'm just trying to understand the implications.

@refack
Copy link
Contributor

refack commented Oct 17, 2018

Here's the consensus among the people sitting around the table as I understood it, I hope I don't misstate anyone.

Thank you very much @sam-github. I have voiced concern in the past that collab summit can turn into an exclusive decision making forum (IIUC the mandate is for high bandwidth discussion that is brought back to GitHub for consensus seeking). For the sake of inclusivity, I really appreciate you bringing the context back to the public!

Very importantly, electron committed to maintaining the gn files. Its something they do already, just out of tree. I assume if that doesn't happen and they are left to rot we would rip the gn files out again.

👍 👍 👍

That means using the hybrid build, ICU is built twice

@hashseed (It is actually built 3 times: host&target for V8 and once more for node)
Anyway that's a bug, I'm sure we can fix anyway

Does that mean we would need to use gclient sync to build node, as well as have the entire depot_tools installed?

IIUC no. If I'm wrong, and we do need depot_tools that is a problem.

@hashseed
Copy link
Member

Depot tools come with GN and ninja binaries, so yeah. Gclient sync is only necessary to fetch dependencies, which for Node we probably don't need, as the dependencies live in the same repository. Unless of course you want to ensure a particular version of clang to be fetched, like done for Chrome and V8.

@hashseed
Copy link
Member

I thought we only build separately for host and target for cross compiled builds, e.g. arm?

@nornagon
Copy link
Contributor Author

nornagon commented Oct 17, 2018

FYI, I've been working on a prototype of this here: https://github.com/electron/node/tree/gn-upstreaming

It's not ready to be a PR yet, but it does successfully build node (at least, on my machine 😉).

One tricky piece is that v8's GN files depend on Chromium's build/ infrastructure, which is fairly large and which I don't think would make sense to vendor into the node repo. I've solved that hackily in my current branch with a little setup script that fetches the repository, but there's probably a more sensible way to do it.

@joyeecheung
Copy link
Member

joyeecheung commented Oct 17, 2018

I have voiced concern in the past that collab summit can turn into an exclusive decision making forum (IIUC the mandate is for high bandwidth discussion that is brought back to GitHub for consensus seeking). For the sake of inclusivity, I really appreciate you bringing the context back to the public!

I think by around the table @sam-github was talking about this thread? There wasn't a physical table where we actually sat down and seeked consensus about GN v.s. GYP AFAIR (and most sessions were like brainstorming and demoing instead of consensus seeking). I didn't go to every session but I couldn't find any session specific to build in the agenda either.

(At least all the tables that I could remember sharing with people in this thread had a lot of food on them and we weren't talking about code that much :S The sudden wave of replies probably stemmed from chats but AFAIK no decisions were attempted to be made there)

@sam-github
Copy link
Contributor

@joyeecheung there was a physical table, it was the LTS and Release group discussion. @addaleax was present and probably has a better memory than me who else was there.

@joyeecheung
Copy link
Member

joyeecheung commented Oct 17, 2018

@sam-github Thanks for the correction. Were there minutes taken? (Didn't find anything in nodejs/Release#358)

@sam-github
Copy link
Contributor

I don't recall. @MylesBorins kicked the meeting off, he may remember.

@hashseed
Copy link
Member

FWIW I hacked together a standalone Node build with GN and dependency management using gclient.

screenshot

@hashseed
Copy link
Member

hashseed commented Feb 15, 2019

We are now running GN-built Node.js on V8' CI. That will soon replace the --build-v8-with-gn option.

@refack
Copy link
Contributor

refack commented Apr 9, 2019

I have a slightly different approach in mind; teach GYP3 to parse GN syntax... AFAICT the delta in actual semantics is small.

This will not solve the V8 CI use case, but once we have GN parsing, nodejs/node-v8 & hashseed/gn-node should give us good coverage.

For the tooling integration use case, I'm planning on improving the "Compilation Database (a.k.a. compile_commands.json) generator, as that seems to converge into a standard consumed by XCode, CLion, MSVS, and VSCode (as well as CLI tools, like clang-tidy, and https://github.com/cquery-project/cquery).

@zuohuadong
Copy link

zuohuadong commented Jul 22, 2019

https://electronjs.org/blog/gn
GN +1

Node.js needs great changes!
Now, we are too slow.

vs Java / Dart / deno /Go
This seems too backward.

@jasnell
Copy link
Member

jasnell commented Jun 19, 2020

There's been no further activity here in nearly a year. Closing. Can reopen if absolutely necessary

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 build files or the CI. question Issues that look for answers.
Projects
None yet
Development

No branches or pull requests