-
Notifications
You must be signed in to change notification settings - Fork 29.1k
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: add GN build files #47637
build: add GN build files #47637
Conversation
Review requested:
|
Maybe add gn setup to https://github.com/nodejs/node/blob/main/.gitpod.yml
Would be good since Node.js only have ASAN for now. |
Does this attempt to build |
I have |
I didn't know gitpod, will give it a try. |
No, the GN config has some significant differences, for example it links with a custom libc++ statically. Eventually I would like to be able to build
It uses the same workflow with checking out V8, the github action file has some instructions in it. First checkout the code:
Then you run gn and ninja:
Note that the https://github.com/photoionization/node-ci repo is a fork of https://chromium.googlesource.com/v8/node-ci/, the latter is used by V8 team to build Node with GN. I'll upstream the changes in my fork once GN configurations are merged into Node. |
ff7e849
to
b0a0174
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is the GN files supposed to be used? Just gn args outdir
and then run the build with ninja?
create a FROM gitpod/workspace-full
ENV PATH=${PATH}:/home/gitpod/depot_tools
RUN cd ~ && git clone https://chromium.googlesource.com/chromium/tools/depot_tools.git --depth=1
// other setup Then in image:
file: .gitpod.Dockerfile |
Thanks for the guidance! I'll try to support it in a separate PR. |
#49279 just landed. |
Is this ready for review now? |
Landed in 32af45d |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code wise LGTM but I have a question
# Copyright 2023 Microsoft Inc. | ||
# Use of this source code is governed by a BSD-style license that can be | ||
# found in the LICENSE file. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What LICENSE file is this referring to? Do we need to check with the foundation about this?
oh I see that I am a bit too late to push the button. But I think we still need some clarification about the license. |
I think what normally is done is an entry about https://github.com/nodejs/node/blob/83e6350b826a23b03417fd6978528aacace6c283/LICENSE#L49-52 otherwise the top-level license automatically applies to the files. This PR however did not add anything to the LICENSE file. I am not a lawyer so I am not sure if this is a concern (it seems to be more of a concern for Microsoft than for us?). Not sure what needs to be done, cc @nodejs/tsc |
PR-URL: #47637 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
The "BSD-style license" refers to Chromium's license. The The I'll start a new PR correcting them. |
PR-URL: #47637 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
PR-URL: #47637 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
As per discussion in nodejs/TSC#1353, this PR adds GN build files into Node's repo.
I have provided 2 ways to test the build:
depot_tools
from Chromium.Both have GitHub actions successfully building for win/mac/linux on arm/arm64/x64/x86.
This PR only adds configurations for building the node target, it does not include configurations for building tests, which I plan to add in a later PR, to make review easier.
After this is merged to Node, I'll upstream my changes in https://github.com/photoionization/node-ci to https://chromium.googlesource.com/v8/node-ci/, which is used by V8 team to test Node.js with latest V8. I'll also setup a nightly build of Node with gn in https://github.com/photoionization/node_with_gn, so we can find out early when the gn build is broken.
/cc @codebytere @victorgomes @joyeecheung @targos