gyp: put filenames in variables #46965

zcbenz · 2023-03-06T02:11:18Z

This PR move source code filenames from the sources field to variables in gyp files.

With this, it becomes much easier extracting the filenames from the gyp files with tools like gypi_to_gn.py, and it significantly eases the work of compiling Node.js with another build system. Chromium was doing the exact same thing during their migration from GYP to GN.

Other than helping embedders, doing this also improves the readability of the gyp files, and actually quite a lot of places in node's gyp files are already doing it.

nodejs-github-bot · 2023-03-06T02:11:22Z

nodejs-github-bot · 2023-03-06T04:54:43Z

CI: https://ci.nodejs.org/job/node-test-pull-request/50221/

bnoordhuis · 2023-03-06T11:17:07Z

I won't block this but I'm pretty meh on this approach. Maybe it helps downstream projects but it just adds another layer of indirection for us.

zcbenz · 2023-03-06T12:08:45Z

A few places are already doing this: ngtcp2.gyp , node_inspector.gypi, and the deps_files and library_files variables in node.gyp (I'm leaving out the tons of openssl's gyp files since it is a special case). As someone not familiar with node's gyp files, I personally find it easier understanding what the gyp file does with all the filenames moved out of the target.

Also probably the only other projects largely used GYP, Chromium chose to put the filenames in variables: content_browser.gypi, and so did Electron. But they abandoned GYP eventually so it may be not very persuading.

For targets that only have one or two files, I agree this is unnecessary indirection, but having a united style feels better, and additionally it helps us embedders.

zcbenz · 2023-03-06T13:12:14Z

Also another thing to consider is, whether node will stick to GYP for forever. If the answer is no, then reorganizing gyp files for easy scripting is something that node will do eventually. The cost is almost none, and arguably improves readability.

A live example of how node's gyp files are reused in another build system can be checked at: https://github.com/photoionization/node_with_gn/blob/main/gn_files/node/deps/ngtcp2/BUILD.gn#L53-L57.

targos

I like the change. I think it makes the config easier to read without big lists of files in the middle.
@zcbenz could you please fix the conflict?

nodejs-github-bot · 2023-03-13T08:44:34Z

CI: https://ci.nodejs.org/job/node-test-pull-request/50350/

nodejs-github-bot · 2023-03-16T09:16:43Z

CI: https://ci.nodejs.org/job/node-test-pull-request/50393/

targos · 2023-03-18T10:06:02Z

Depends on #44004 to land on v19.x

nodejs/node#46965

* chore: upgrade to Node.js v20 * src: allow embedders to override NODE_MODULE_VERSION nodejs/node#49279 * src: fix missing trailing , nodejs/node#46909 * src,tools: initialize cppgc nodejs/node#45704 * tools: allow passing absolute path of config.gypi in js2c nodejs/node#49162 * tools: port js2c.py to C++ nodejs/node#46997 * doc,lib: disambiguate the old term, NativeModule nodejs/node#45673 * chore: fixup Node.js BSSL tests * nodejs/node#49492 * nodejs/node#44498 * deps: upgrade to libuv 1.45.0 nodejs/node#48078 * deps: update V8 to 10.7 nodejs/node#44741 * test: use gcUntil() in test-v8-serialize-leak nodejs/node#49168 * module: make CJS load from ESM loader nodejs/node#47999 * src: make BuiltinLoader threadsafe and non-global nodejs/node#45942 * chore: address changes to CJS/ESM loading * module: make CJS load from ESM loader (nodejs/node#47999) * lib: improve esm resolve performance (nodejs/node#46652) * bootstrap: optimize modules loaded in the built-in snapshot nodejs/node#45849 * test: mark test-runner-output as flaky nodejs/node#49854 * lib: lazy-load deps in modules/run_main.js nodejs/node#45849 * url: use private properties for brand check nodejs/node#46904 * test: refactor `test-node-output-errors` nodejs/node#48992 * assert: deprecate callTracker nodejs/node#47740 * src: cast v8::Object::GetInternalField() return value to v8::Value nodejs/node#48943 * test: adapt test-v8-stats for V8 update nodejs/node#45230 * tls: ensure TLS Sockets are closed if the underlying wrap closes nodejs/node#49327 * test: deflake test-tls-socket-close nodejs/node#49575 * net: fix crash due to simultaneous close/shutdown on JS Stream Sockets nodejs/node#49400 * net: use asserts in JS Socket Stream to catch races in future nodejs/node#49400 * lib: fix BroadcastChannel initialization location nodejs/node#46864 * src: create BaseObject with node::Realm nodejs/node#44348 * src: implement DataQueue and non-memory resident Blob nodejs/node#45258 * sea: add support for V8 bytecode-only caching nodejs/node#48191 * chore: fixup patch indices * gyp: put filenames in variables nodejs/node#46965 * build: modify js2c.py into GN executable * fix: (WIP) handle string replacement of fs -> original-fs * [v20.x] backport vm-related memory fixes nodejs/node#49874 * src: make BuiltinLoader threadsafe and non-global nodejs/node#45942 * src: avoid copying string in fs_permission nodejs/node#47746 * look upon my works ye mighty and dispair * chore: patch cleanup * [api] Remove AllCan Read/Write https://chromium-review.googlesource.com/c/v8/v8/+/5006387 * fix: missing include for NODE_EXTERN * chore: fixup patch indices * fix: fail properly when js2c fails in Node.js * build: fix js2c root_gen_dir * fix: lib/fs.js -> lib/original-fs.js * build: fix original-fs file xforms * fixup! module: make CJS load from ESM loader * build: get rid of CppHeap for now * build: add patch to prevent extra fs lookup on esm load * build: greatly simplify js2c modifications Moves our original-fs modifications back into a super simple python script action, wires up the output of that action into our call to js2c * chore: update to handle moved internal/modules/helpers file * test: update @types/node test * feat: enable preventing cppgc heap creation * feat: optionally prevent calling V8::EnableWebAssemblyTrapHandler * fix: no cppgc initialization in the renderer * gyp: put filenames in variables nodejs/node#46965 * test: disable single executable tests * fix: nan tests failing on node headers missing file * tls,http2: send fatal alert on ALPN mismatch nodejs/node#44031 * test: disable snapshot tests * nodejs/node#47887 * nodejs/node#49684 * nodejs/node#44193 * build: use deps/v8 for v8/tools Node.js hard depends on these in their builtins * test: fix edge snapshot stack traces nodejs/node#49659 * build: remove js2c //base dep * build: use electron_js2c_toolchain to build node_js2c * fix: don't create SafeSet outside packageResolve Fixes failure in parallel/test-require-delete-array-iterator: === release test-require-delete-array-iterator === Path: parallel/test-require-delete-array-iterator node:internal/per_context/primordials:426 constructor(i) { super(i); } // eslint-disable-line no-useless-constructor ^ TypeError: object is not iterable (cannot read property Symbol(Symbol.iterator)) at new Set (<anonymous>) at new SafeSet (node:internal/per_context/primordials:426:22) * fix: failing crashReporter tests on Linux These were failing because our change from node::InitializeNodeWithArgs to node::InitializeOncePerProcess meant that we now inadvertently called PlatformInit, which reset signal handling. This meant that our intentional crash function ElectronBindings::Crash no longer worked and the renderer process no longer crashed when process.crash() was called. We don't want to use Node.js' default signal handling in the renderer process, so we disable it by passing kNoDefaultSignalHandling to node::InitializeOncePerProcess. * build: only create cppgc heap on non-32 bit platforms * chore: clean up util:CompileAndCall * src: fix compatility with upcoming V8 12.1 APIs nodejs/node#50709 * fix: use thread_local BuiltinLoader * chore: fixup v8 patch indices --------- Co-authored-by: Keeley Hammond <vertedinde@electronjs.org> Co-authored-by: Samuel Attard <marshallofsound@electronjs.org>

nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Mar 6, 2023

gengjiawen added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 6, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 6, 2023

gengjiawen approved these changes Mar 6, 2023

View reviewed changes

targos approved these changes Mar 12, 2023

View reviewed changes

gyp: put filenames in variables

9b59718

zcbenz force-pushed the patch-5 branch from 3129350 to 9b59718 Compare March 13, 2023 06:29

gengjiawen added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 13, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 13, 2023

This was referenced Mar 14, 2023

CI Reliability 2023-03-14 nodejs/reliability#523

Open

CI Reliability 2023-03-15 nodejs/reliability#524

Open

zcbenz mentioned this pull request Mar 15, 2023

Add GN configurations to Node nodejs/TSC#1353

Closed

github-actions bot mentioned this pull request Mar 16, 2023

CI Reliability 2023-03-16 nodejs/reliability#525

Open

22 tasks

gengjiawen added commit-queue Add this label to land a pull request using GitHub Actions. request-ci Add this label to start a Jenkins CI on a PR. labels Mar 16, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 16, 2023

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 16, 2023

targos added the backport-blocked-v19.x label Mar 18, 2023

This was referenced Mar 19, 2023

CI Reliability 2023-03-19 nodejs/reliability#528

Open

CI Reliability 2023-03-20 nodejs/reliability#530

Open

CI Reliability 2023-03-21 nodejs/reliability#531

Open

CI Reliability 2023-03-22 nodejs/reliability#532

Open

RafaelGSS mentioned this pull request Apr 3, 2023

2023-04-18, Version 20.0.0 (Current) #47381

Merged

danielleadams added backport-open-v18.x Indicate that the PR has an open backport. backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. and removed backport-open-v18.x Indicate that the PR has an open backport. labels Jul 2, 2023

danielleadams mentioned this pull request Jul 6, 2023

deps: upgrade to libuv 1.45.0 #48078

Closed

zcbenz mentioned this pull request Aug 15, 2023

gyp: put cctest filenames in variables #49178

Merged

ruyadorno mentioned this pull request Sep 8, 2023

v18.18.0 proposal #49220

Merged

richardlau mentioned this pull request Oct 11, 2023

[v18.x] deps: update nghttp2 to 1.57.0 #50151

Closed

codebytere added a commit to electron/electron that referenced this pull request Nov 6, 2023

gyp: put filenames in variables

eb8fd15

nodejs/node#46965

codebytere added a commit to electron/electron that referenced this pull request Nov 14, 2023

gyp: put filenames in variables

49f6555

nodejs/node#46965

codebytere added a commit to electron/electron that referenced this pull request Nov 15, 2023

gyp: put filenames in variables

22cd37f

nodejs/node#46965

codebytere added a commit to electron/electron that referenced this pull request Nov 16, 2023

gyp: put filenames in variables

7ad0c05

nodejs/node#46965

codebytere added a commit to electron/electron that referenced this pull request Nov 16, 2023

gyp: put filenames in variables

f9efa59

nodejs/node#46965

codebytere added a commit to electron/electron that referenced this pull request Nov 21, 2023

gyp: put filenames in variables

74d6617

nodejs/node#46965

codebytere added a commit to electron/electron that referenced this pull request Nov 21, 2023

gyp: put filenames in variables

af87d0b

nodejs/node#46965

codebytere added a commit to electron/electron that referenced this pull request Nov 22, 2023

gyp: put filenames in variables

cb13f94

nodejs/node#46965

codebytere added a commit to electron/electron that referenced this pull request Nov 22, 2023

gyp: put filenames in variables

08ff302

nodejs/node#46965

zcbenz mentioned this pull request Nov 28, 2023

Things blocking a full-featured GN build of upstream Node.js photoionization/node_with_gn#1

Closed

27 tasks

codebytere added a commit to electron/electron that referenced this pull request Nov 28, 2023

gyp: put filenames in variables

b6aa181

nodejs/node#46965

codebytere added a commit to electron/electron that referenced this pull request Nov 28, 2023

gyp: put filenames in variables

5fe44d8

nodejs/node#46965

codebytere added a commit to electron/electron that referenced this pull request Nov 29, 2023

gyp: put filenames in variables

3847e64

nodejs/node#46965

codebytere added a commit to electron/electron that referenced this pull request Nov 29, 2023

gyp: put filenames in variables

9542d88

nodejs/node#46965

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gyp: put filenames in variables #46965

gyp: put filenames in variables #46965

zcbenz commented Mar 6, 2023

nodejs-github-bot commented Mar 6, 2023

nodejs-github-bot commented Mar 6, 2023

bnoordhuis commented Mar 6, 2023

zcbenz commented Mar 6, 2023

zcbenz commented Mar 6, 2023

targos left a comment

nodejs-github-bot commented Mar 13, 2023

nodejs-github-bot commented Mar 16, 2023

targos commented Mar 18, 2023

gyp: put filenames in variables #46965

gyp: put filenames in variables #46965

Conversation

zcbenz commented Mar 6, 2023

nodejs-github-bot commented Mar 6, 2023

nodejs-github-bot commented Mar 6, 2023

bnoordhuis commented Mar 6, 2023

zcbenz commented Mar 6, 2023

zcbenz commented Mar 6, 2023

targos left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Mar 13, 2023

nodejs-github-bot commented Mar 16, 2023

targos commented Mar 18, 2023