tools: make js2c.py usable for other build systems by zcbenz · Pull Request #46930 · nodejs/node

zcbenz · 2023-03-03T09:07:12Z

The js2c.py script assumes that: 1. the script is running under the source root dir; 2. passed paths are all relative paths.

This assumption makes it very hard for other build systems to reuse the script, because most build systems choose to run scripts in the build dir instead of source root dir.

This PR adds a --root argument to the script, allowing users to pass absolute paths to the script while correctly resolving module names.

aduh95 · 2023-03-03T12:41:29Z

/cc @nodejs/build

richardlau

Please update the tests in test/tools/test_js2c.py. You can run them via the tooltest Makefile target:

node/Makefile

Lines 314 to 316 in 31156a7

    
           .PHONY: tooltest 
        
           tooltest: 
        
           	@$(PYTHON) -m unittest discover -s ./test/tools

zcbenz · 2023-03-03T23:45:37Z

I have updated with tests.

tools/js2c.py

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

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

nodejs-github-bot · 2023-03-18T09:11:54Z

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

nodejs-github-bot · 2023-03-19T09:27:06Z

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

joyeecheung · 2023-03-20T16:04:23Z

@richardlau This is now rewritten with chdir, do we still need a test?

richardlau · 2023-03-20T16:12:21Z

@richardlau This is now rewritten with chdir, do we still need a test?

My main concern was that the existing tests were broken with the previous implementation of this PR. Looks like the tests are passing now with the rewrite.

nodejs-github-bot · 2023-04-06T13:10:32Z

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

nodejs-github-bot · 2023-04-07T05:15:00Z

Commit Queue failed

- Loading data for nodejs/node/pull/46930
✔  Done loading data for nodejs/node/pull/46930
----------------------------------- PR info ------------------------------------
Title      tools: make js2c.py usable for other build systems (#46930)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     zcbenz:patch-4 -> nodejs:main
Labels     build, python, author ready, needs-ci
Commits    1
 - tools: make js2c.py usable for other build systems
Committers 1
 - Cheng Zhao 
PR-URL: https://github.com/nodejs/node/pull/46930
Reviewed-By: Richard Lau 
Reviewed-By: Joyee Cheung 
Reviewed-By: Michaël Zasso 
Reviewed-By: Luigi Pinca 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/46930
Reviewed-By: Richard Lau 
Reviewed-By: Joyee Cheung 
Reviewed-By: Michaël Zasso 
Reviewed-By: Luigi Pinca 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Fri, 03 Mar 2023 09:07:12 GMT
   ✔  Approvals: 4
   ✔  - Richard Lau (@richardlau) (TSC): https://github.com/nodejs/node/pull/46930#pullrequestreview-1348928715
   ✔  - Joyee Cheung (@joyeecheung) (TSC): https://github.com/nodejs/node/pull/46930#pullrequestreview-1342560058
   ✔  - Michaël Zasso (@targos) (TSC): https://github.com/nodejs/node/pull/46930#pullrequestreview-1346957158
   ✔  - Luigi Pinca (@lpinca): https://github.com/nodejs/node/pull/46930#pullrequestreview-1357897216
   ✘  Last GitHub CI failed
   ℹ  Last Full PR CI on 2023-04-06T13:10:32Z: https://ci.nodejs.org/job/node-test-pull-request/51006/
- Querying data for job/node-test-pull-request/51006/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/4635641092

aduh95 · 2023-04-10T20:11:32Z

Landed in 3a98dd3

PR-URL: #46930 Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: nodejs#46930 Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

…and_custom_embedder_js.patch Xref: nodejs/node#46930 manually sync patch to minor upstream code shear

* chore: bump node in DEPS to v18.17.0 * chore: update build_modify_js2c_py_to_allow_injection_of_original-fs_and_custom_embedder_js.patch Xref: nodejs/node#46930 manually sync patch to minor upstream code shear * chore: update build_ensure_native_module_compilation_fails_if_not_using_a_new.patch Xref: nodejs/node#48248 manually sync patch to minor upstream code shear * chore: update fix_expose_the_built-in_electron_module_via_the_esm_loader.patch Xref: nodejs/node#47824 chore: upstream func throwIfUnsupportedURLProtocol() has been removed, so no need to patch it * chore: update api_pass_oomdetails_to_oomerrorcallback.patch Xref: nodejs/node#47695 manually sync patch to minor upstream code shear * chore: remove fix_prevent_changing_functiontemplateinfo_after_publish.patch Xref: nodejs/node#46979 (upstreamed patch) Xref: https://chromium-review.googlesource.com/c/v8/v8/+/2718147 (related) * chore: update fix_adapt_debugger_tests_for_upstream_v8_changes.patch Xref: nodejs/node#47274 manually sync patch to minor upstream code shear some tests moved from sequential to parallel * chore: remove fix_libc_buffer_overflow_in_string_view_ctor.patch Xref: fix_libc_buffer_overflow_in_string_view_ctor.patch patch is no longer needed due to upstream bump to ada 2.2.0 * chore: remove fix_preventing_potential_oob_in_ada_no_scheme_parsing.patch Xref: nodejs/node#47339 patch is no longer needed due to upstream bump to ada 2.2.0 * chore: rebuild filenames.json several files removed/added/changed upstream * chore: update build_add_gn_build_files.patch upstream dep histogram 0.11.7 moved its include path from src/ to include/ Xref: nodejs/node#47742 * chore: update fix_crypto_tests_to_run_with_bssl.patch Xref: nodejs/node#47160 BoringSSL doesn't support BIO_s_secmem() (a secure heap variant of BIO_s_mem()), so use BIO_s_mem() instead. Related discussion of secure heap support in BoringSSL: https://boringssl-review.googlesource.com/c/boringssl/+/54309 * fix: ftbfs in node dep ada * fix: ftbfs in node dep uvwasi * chore: rebuild patches * chore: update fix_handle_boringssl_and_openssl_incompatibilities.patch Upstream used `BIO_s_secmem()`, a secure heap variant of `BIO_s_mem()`. BoringSSL doesn't support it, so this PR opts for `BIO_s_mem()` instead. Upstream Node.js change that prompted this: nodejs/node#47160 Related discussion of BoringSSL support of secure heap: https://boringssl-review.googlesource.com/c/boringssl/+/54309 * fix: work around Node 18 isURL() regression * chore: sort script/node-disabled-tests.json alphabetically * test: add parallel/test-snapshot-argv1 to disabled list test: add parallel/test-snapshot-namespaced-builtin to disabled list We don't support that type of snapshotting at the moment. * chore: disable flaky node test parallel/test-dgram-send-cb-quelches-error fails upstream in v18.x on my box as well * ci: ensure spawned node tests have ELECTRON_RUN_AS_NODE set * fixup! fix: work around Node 18 isURL() regression fix: infinite loop regression * fixup! fix: work around Node 18 isURL() regression * chore: patch fixtures/errors/force_colors.snapshot The line numbers in the stacktrace from our v8 build don't match what Node's tests are expecting, so update the stacktrace to match our build. The specific numbers probably aren't t needed for the force_colors test, which is trying to see whether or not the lines are greyed out. One option is to upstream a test change to stop hardcoding the stacktrace. * fixup! fix: work around Node 18 isURL() regression fix; pull in upstream bugfix * fixup! ci: ensure spawned node tests have ELECTRON_RUN_AS_NODE set chore: do not inject ELECTRON_RUN_AS_NODE in test-assert-colors.js * chore: disable flaky node test parallel/test-debugger-random-port-with-inspect-port --------- Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com> Co-authored-by: Charles Kerr <charles@charleskerr.com>

nodejs-github-bot added build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run. python PRs and issues that require attention from people who are familiar with Python. labels Mar 3, 2023

richardlau requested changes Mar 3, 2023

View reviewed changes

zcbenz force-pushed the patch-4 branch from 1904b30 to 7408e96 Compare March 3, 2023 23:09

joyeecheung reviewed Mar 5, 2023

View reviewed changes

tools/js2c.py Outdated Show resolved Hide resolved

tools: make js2c.py usable for other build systems

5f22737

zcbenz force-pushed the patch-4 branch from 7408e96 to 5f22737 Compare March 8, 2023 10:33

joyeecheung approved these changes Mar 15, 2023

View reviewed changes

zcbenz mentioned this pull request Mar 15, 2023

Add GN configurations to Node nodejs/TSC#1353

Closed

gengjiawen requested a review from richardlau March 16, 2023 09:09

gengjiawen added the request-ci Add this label to start a Jenkins CI on a PR. label 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

This was referenced Mar 17, 2023

CI Reliability 2023-03-17 nodejs/reliability#526

Open

CI Reliability 2023-03-18 nodejs/reliability#527

Open

targos approved these changes Mar 18, 2023

View reviewed changes

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

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

Open

19 tasks

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

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

Open

17 tasks

richardlau approved these changes Mar 20, 2023

View reviewed changes

This was referenced Mar 21, 2023

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

Open

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

Open

lpinca approved these changes Mar 25, 2023

View reviewed changes

This comment was marked as outdated.

Sign in to view

joyeecheung added the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 29, 2023

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

CI Reliability 2023-03-30 nodejs/reliability#534

Open

18 tasks

gengjiawen added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 6, 2023

This comment was marked as outdated.

Sign in to view

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

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

gengjiawen added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 7, 2023

nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Apr 7, 2023

gengjiawen mentioned this pull request Apr 7, 2023

Inconsistent CI status nodejs/node-core-utils#692

Open

jasnell approved these changes Apr 10, 2023

View reviewed changes

aduh95 merged commit 3a98dd3 into nodejs:main Apr 10, 2023

RafaelGSS mentioned this pull request Apr 11, 2023

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

Merged

danielleadams mentioned this pull request Jul 10, 2023

v18.17.0 release proposal #48694

Merged

ckerr added a commit to electron/electron that referenced this pull request Jul 19, 2023

chore: update build_modify_js2c_py_to_allow_injection_of_original-fs_…

c9104ec

…and_custom_embedder_js.patch Xref: nodejs/node#46930 manually sync patch to minor upstream code shear

ckerr added a commit to electron/electron that referenced this pull request Jul 25, 2023

chore: update build_modify_js2c_py_to_allow_injection_of_original-fs_…

4c43c9e

…and_custom_embedder_js.patch Xref: nodejs/node#46930 manually sync patch to minor upstream code shear

ckerr added a commit to electron/electron that referenced this pull request Aug 7, 2023

chore: update build_modify_js2c_py_to_allow_injection_of_original-fs_…

8e6400c

…and_custom_embedder_js.patch Xref: nodejs/node#46930 manually sync patch to minor upstream code shear

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

	.PHONY: tooltest
	tooltest:
	@$(PYTHON) -m unittest discover -s ./test/tools

Uh oh!

Conversation

zcbenz commented Mar 3, 2023

Uh oh!

aduh95 commented Mar 3, 2023

Uh oh!

richardlau left a comment

Choose a reason for hiding this comment

Uh oh!

zcbenz commented Mar 3, 2023

Uh oh!

Uh oh!

nodejs-github-bot commented Mar 16, 2023

Uh oh!

nodejs-github-bot commented Mar 18, 2023

Uh oh!

nodejs-github-bot commented Mar 19, 2023

Uh oh!

joyeecheung commented Mar 20, 2023

Uh oh!

richardlau commented Mar 20, 2023

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

nodejs-github-bot commented Apr 6, 2023

Uh oh!

nodejs-github-bot commented Apr 7, 2023

Uh oh!

aduh95 commented Apr 10, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants