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

[danbev]

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 beempty [-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' orreference to 'auto'static const uint8_t [] ={ ^/node/out/Release/obj/gen/node_code_cache.cc:12:22: error:excess elements in scalar initializerstatic 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 providedconstexpr 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

[nodejs-github-bot]

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

[devsnek]

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

[refack]

Should not land without a test.

[refack]

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

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

[danbev]

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

[nodejs-github-bot]

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

[Trott]

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

[nodejs-github-bot]

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

[refack]

No as critical, but should be

Suggested change

	ifsplit!= []:
	iflen(split):

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

Thanks

[danbev]

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 (רפאל פלחי) < [email protected]>:

…

***@***.**** 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> .

[nodejs-github-bot]

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

[danbev]

Landed in b4f0a18.