tools: allow single JS file for --link-module by danbev · Pull Request #28443 · nodejs/node · GitHub

The description for the --link-module configuration option is as
follows:

$ ./configure --help | grep -A 5 'link-module'
  --link-module=LINKED_MODULE
                      Path to a JS file to be bundled in the binary as a
                      builtin. This module will be referenced by path
                      without extension; e.g. /root/x/y.js will be
                      referenced via require('root/x/y'). Can be used
                      multiple times

This lead me to think that it was possible to specify a file like this:

$ ./configure --link-module=something.js
$ NODE_DEBUG=mkcodecache make -j8

This will lead to a compilation error as an entry in the source_ map in
node_javascript.cc will end up having an empty string as its key:

source_.emplace("", UnionBytes{_raw, 105});

This will then be used by CodeCacheBuilder when it iterates over the
module ids, which will lead to the following compilation errors:

/node/out/Release/obj/gen/node_code_cache.cc:12:23: warning:
ISO C++17 does not allow a decomposition group to be
empty [-Wempty-decomposition]
static const uint8_t [] = {
                      ^
/node/out/Release/obj/gen/node_code_cache.cc:12:22: warning:
decomposition declarations are a C++17 extension [-Wc++17-extensions]
static const uint8_t [] = {
                     ^~
/node/out/Release/obj/gen/node_code_cache.cc:12:1: error:
decomposition declaration cannot be declared 'static'
static const uint8_t [] = {
^~~~~~
/node/out/Release/obj/gen/node_code_cache.cc:12:22: error:
decomposition declaration cannot be declared with type 'const uint8_t'
(aka 'const unsigned char'); declared type must be 'auto' or
reference to 'auto'
static const uint8_t [] = {
                     ^
/node/out/Release/obj/gen/node_code_cache.cc:12:22: error:
excess elements in scalar initializer
static const uint8_t [] = {
                     ^
/node/out/Release/obj/gen/node_code_cache.cc:660:7: error:
expected expression
      ,
      ^
/node/out/Release/obj/gen/node_code_cache.cc:661:24: error:
no matching function for call to 'arraysize'
      static_cast<int>(arraysize()), policy
                       ^~~~~~~~~
../src/util.h:667:18: note: candidate function template not viable:
requires 1 argument, but 0 were provided
constexpr size_t arraysize(const T (&)[N]) {
                 ^
2 warnings and 5 errors generated.

This commit suggests that passing a single file be allowed by modifying
tools/js2c.py, or alternatively raise an Exception with an message that
indicates the cause of this.

With the changes in this PR the usage would look like this:

$ cat something.js
'use strict';

function logit() {
  console.log('something module...');
}

module.exports = {
  logit,
}
$ ./configure --link-module=something.js && NODE_DEBUG=mkcodecache make -j8
...
Generated cache for something, size = 840.00B, total = 643.00KB
...
$ rm something.js
$ ./node -p "require('something').logit()"
something module...
undefined

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

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

Before this change, what input to --link-module works?

Should not land without a test.

Before this change, what input to --link-module works?

anything with at least 1 / like lib//gaga.js should work.

Should not land without a test.

I'll take a look at adding a test for this, might not be until next week though. Thanks

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

@refack Test was added. OK to dismiss your request for changes?

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

No as critical, but should be

Suggested change

      
              if split != []:
          
              if len(split):

as it's more "pythonic", and doesn't assume split is a list, only that it's enumerable.

Thanks

I’m currently on PTO and will be back in August and I’m happy to address this then. Thanks. tis 23 juli 2019 kl. 04:41 skrev Refael Ackermann (רפאל פלחי) < notifications@github.com>:

…

***@***.**** commented on this pull request. ------------------------------ In tools/js2c.py <#28443 (comment)>: > @@ -261,7 +261,8 @@ def NormalizeFileName(filename): split = ['internal'] + split else: # `lib/**/*.js` so drop the 'lib' part split = split[1:] - filename = '/'.join(split) + if split != []: No as critical, but should be ⬇️ Suggested change - if split != []: + if len(split): as it's more "pythonic", and doesn't assume split is a list, only that it's enumerable. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#28443?email_source=notifications&email_token=AADJRX73YINRA5ODGBOKW53QAX5M5A5CNFSM4H3X3532YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOB7FVFKY#pullrequestreview-264983211>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADJRX4FRUZ6YYGFBOUQE6TQAX5M5ANCNFSM4H3X353Q> .

The description for the --link-module configuration option is as follows: $ ./configure --help | grep -A 5 'link-module' --link-module=LINKED_MODULE Path to a JS file to be bundled in the binary as a builtin. This module will be referenced by path without extension; e.g. /root/x/y.js will be referenced via require('root/x/y'). Can be used multiple times This lead me to think that it was possible to specify a file like this: $ ./configure --link-module=something.js $ NODE_DEBUG=mkcodecache make -j8 This will lead to a compilation error as an entry in the source_ map in node_javascript.cc will end up having an empty string as its key: source_.emplace("", UnionBytes{_raw, 105}); This will then be used by CodeCacheBuilder when it iterates over the module ids, which will lead to the following compilation errors: /node/out/Release/obj/gen/node_code_cache.cc:12:23: warning: ISO C++17 does not allow a decomposition group to be empty [-Wempty-decomposition] static const uint8_t [] = { ^ /node/out/Release/obj/gen/node_code_cache.cc:12:22: warning: decomposition declarations are a C++17 extension [-Wc++17-extensions] static const uint8_t [] = { ^~ /node/out/Release/obj/gen/node_code_cache.cc:12:1: error: decomposition declaration cannot be declared 'static' static const uint8_t [] = { ^~~~~~ /node/out/Release/obj/gen/node_code_cache.cc:12:22: error: decomposition declaration cannot be declared with type 'const uint8_t' (aka 'const unsigned char'); declared type must be 'auto' or reference to 'auto' static const uint8_t [] = { ^ /node/out/Release/obj/gen/node_code_cache.cc:12:22: error: excess elements in scalar initializer static const uint8_t [] = { ^ /node/out/Release/obj/gen/node_code_cache.cc:660:7: error: expected expression , ^ /node/out/Release/obj/gen/node_code_cache.cc:661:24: error: no matching function for call to 'arraysize' static_cast<int>(arraysize()), policy ^~~~~~~~~ ../src/util.h:667:18: note: candidate function template not viable: requires 1 argument, but 0 were provided constexpr size_t arraysize(const T (&)[N]) { ^ 2 warnings and 5 errors generated. This commit suggests that passing a single file be allowed by modifying tools/js2c.py, or alternatively raise an Exception with an message that indicates the cause of this.

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

Landed in b4f0a18.

The description for the --link-module configuration option is as follows: $ ./configure --help | grep -A 5 'link-module' --link-module=LINKED_MODULE Path to a JS file to be bundled in the binary as a builtin. This module will be referenced by path without extension; e.g. /root/x/y.js will be referenced via require('root/x/y'). Can be used multiple times This lead me to think that it was possible to specify a file like this: $ ./configure --link-module=something.js $ NODE_DEBUG=mkcodecache make -j8 This will lead to a compilation error as an entry in the source_ map in node_javascript.cc will end up having an empty string as its key: source_.emplace("", UnionBytes{_raw, 105}); This will then be used by CodeCacheBuilder when it iterates over the module ids, which will lead to the following compilation errors: /node/out/Release/obj/gen/node_code_cache.cc:12:23: warning: ISO C++17 does not allow a decomposition group to be empty [-Wempty-decomposition] static const uint8_t [] = { ^ /node/out/Release/obj/gen/node_code_cache.cc:12:22: warning: decomposition declarations are a C++17 extension [-Wc++17-extensions] static const uint8_t [] = { ^~ /node/out/Release/obj/gen/node_code_cache.cc:12:1: error: decomposition declaration cannot be declared 'static' static const uint8_t [] = { ^~~~~~ /node/out/Release/obj/gen/node_code_cache.cc:12:22: error: decomposition declaration cannot be declared with type 'const uint8_t' (aka 'const unsigned char'); declared type must be 'auto' or reference to 'auto' static const uint8_t [] = { ^ /node/out/Release/obj/gen/node_code_cache.cc:12:22: error: excess elements in scalar initializer static const uint8_t [] = { ^ /node/out/Release/obj/gen/node_code_cache.cc:660:7: error: expected expression , ^ /node/out/Release/obj/gen/node_code_cache.cc:661:24: error: no matching function for call to 'arraysize' static_cast<int>(arraysize()), policy ^~~~~~~~~ ../src/util.h:667:18: note: candidate function template not viable: requires 1 argument, but 0 were provided constexpr size_t arraysize(const T (&)[N]) { ^ 2 warnings and 5 errors generated. This commit suggests that passing a single file be allowed by modifying tools/js2c.py. PR-URL: nodejs#28443 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

The description for the --link-module configuration option is as follows: $ ./configure --help | grep -A 5 'link-module' --link-module=LINKED_MODULE Path to a JS file to be bundled in the binary as a builtin. This module will be referenced by path without extension; e.g. /root/x/y.js will be referenced via require('root/x/y'). Can be used multiple times This lead me to think that it was possible to specify a file like this: $ ./configure --link-module=something.js $ NODE_DEBUG=mkcodecache make -j8 This will lead to a compilation error as an entry in the source_ map in node_javascript.cc will end up having an empty string as its key: source_.emplace("", UnionBytes{_raw, 105}); This will then be used by CodeCacheBuilder when it iterates over the module ids, which will lead to the following compilation errors: /node/out/Release/obj/gen/node_code_cache.cc:12:23: warning: ISO C++17 does not allow a decomposition group to be empty [-Wempty-decomposition] static const uint8_t [] = { ^ /node/out/Release/obj/gen/node_code_cache.cc:12:22: warning: decomposition declarations are a C++17 extension [-Wc++17-extensions] static const uint8_t [] = { ^~ /node/out/Release/obj/gen/node_code_cache.cc:12:1: error: decomposition declaration cannot be declared 'static' static const uint8_t [] = { ^~~~~~ /node/out/Release/obj/gen/node_code_cache.cc:12:22: error: decomposition declaration cannot be declared with type 'const uint8_t' (aka 'const unsigned char'); declared type must be 'auto' or reference to 'auto' static const uint8_t [] = { ^ /node/out/Release/obj/gen/node_code_cache.cc:12:22: error: excess elements in scalar initializer static const uint8_t [] = { ^ /node/out/Release/obj/gen/node_code_cache.cc:660:7: error: expected expression , ^ /node/out/Release/obj/gen/node_code_cache.cc:661:24: error: no matching function for call to 'arraysize' static_cast<int>(arraysize()), policy ^~~~~~~~~ ../src/util.h:667:18: note: candidate function template not viable: requires 1 argument, but 0 were provided constexpr size_t arraysize(const T (&)[N]) { ^ 2 warnings and 5 errors generated. This commit suggests that passing a single file be allowed by modifying tools/js2c.py. PR-URL: #28443 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

nodejs/node#28443

nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Jun 27, 2019

lpinca approved these changes Jun 27, 2019

View reviewed changes

refack previously requested changes Jun 28, 2019

View reviewed changes

BridgeAR requested a review from refack July 4, 2019 17:40

Trott approved these changes Jul 19, 2019

View reviewed changes

refack reviewed Jul 22, 2019

View reviewed changes

danbev added 3 commits August 12, 2019 05:26

squash: add test for js2c.py

d1dae25

squash: only assume that split is a enumerable

a656ed3

danbev force-pushed the tools_js2c_link-module branch from d31f2e7 to a656ed3 Compare August 12, 2019 03:49

danbev added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Aug 12, 2019

danbev closed this Aug 12, 2019

danbev deleted the tools_js2c_link-module branch August 12, 2019 11:08

targos mentioned this pull request Aug 19, 2019

v12.9.0 release proposal #29210

Merged

This was referenced Aug 20, 2019

2019-08-20 Version 12.9.0 (Current) @targos beevelop/docker-android-nodejs#328

Closed

2019-08-20 Version 12.9.0 (Current) @targos beevelop/docker-android-nodejs#329

Closed

codebytere added a commit to electron/electron that referenced this pull request Aug 21, 2019

update build_add_gn_build_files.patch

36b41d5

nodejs/node#28443

tools: allow single JS file for --link-module by danbev · Pull Request #28443 · nodejs/node · GitHub

Uh oh!

Uh oh!

tools: allow single JS file for --link-module - #28443

tools: allow single JS file for --link-module#28443
danbev wants to merge 3 commits into
nodejs:masterfrom
danbev:tools_js2c_link-module

danbev commented Jun 27, 2019

Uh oh!

nodejs-github-bot commented Jun 27, 2019

Uh oh!

devsnek commented Jun 27, 2019

Uh oh!

refack left a comment

Uh oh!

refack commented Jun 28, 2019

Uh oh!

danbev commented Jun 28, 2019

Uh oh!

nodejs-github-bot commented Jul 2, 2019

Uh oh!

Trott commented Jul 19, 2019

Uh oh!

nodejs-github-bot commented Jul 19, 2019

Uh oh!

refack Jul 22, 2019

Uh oh!

danbev commented Jul 26, 2019 via email

Uh oh!

nodejs-github-bot commented Aug 12, 2019

Uh oh!

danbev commented Aug 12, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

tools: allow single JS file for --link-module by danbev · Pull Request #28443 · nodejs/node · GitHub

Uh oh!

Uh oh!

tools: allow single JS file for --link-module - #28443

tools: allow single JS file for --link-module#28443danbev wants to merge 3 commits intonodejs:masternodejs/node:masterfrom danbev:tools_js2c_link-moduledanbev/node:tools_js2c_link-moduleCopy head branch name to clipboard

Conversation

danbev commented Jun 27, 2019

Checklist

Uh oh!

nodejs-github-bot commented Jun 27, 2019

Uh oh!

devsnek commented Jun 27, 2019

Uh oh!

refack left a comment

Choose a reason for hiding this comment

Uh oh!

refack commented Jun 28, 2019

Uh oh!

danbev commented Jun 28, 2019

Uh oh!

nodejs-github-bot commented Jul 2, 2019

Uh oh!

Trott commented Jul 19, 2019

Uh oh!

nodejs-github-bot commented Jul 19, 2019

Uh oh!

refack Jul 22, 2019

Choose a reason for hiding this comment

Uh oh!

danbev commented Jul 26, 2019 via email

Uh oh!

nodejs-github-bot commented Aug 12, 2019

Uh oh!

danbev commented Aug 12, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

tools: allow single JS file for --link-module#28443
danbev wants to merge 3 commits into
nodejs:masterfrom
danbev:tools_js2c_link-module