tools/bootstrap_node: preprocess gypi files to json #19140

devsnek · 2018-03-05T00:40:49Z

Checklist

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

Affected core subsystem(s)

bootstrap, tools

devsnek · 2018-03-05T00:43:01Z

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

joyeecheung · 2018-03-05T01:16:53Z

tools/js2c.py

+    # if its a gypi file we're going to want it as json
+    # later on anyway, so get it out of the way now
+    if name.endswith(".gypi"):
+      lines = json.dumps(eval(lines))


There is code for similar purposes in tools/install.py and it only does:

node/tools/install.py

Lines 23 to 24 in a8b5192

s = re.sub(r'#.*?\n', '', s) # strip comments

s = re.sub(r'\'', '"', s) # convert quotes

Maybe we can just do the same so don't have to eval the config then redump it here..

imo eval is cleaner, what if (for some ungodly reason) a config value has an escaped quote in it.

It does the right thing. config.gypi is produced by pprint.pformat() and always uses single quotes for strings. \'hola\' becomes \"hola\" and that's legal JSON.

rvagg · 2018-03-05T01:27:22Z

no good on windows

devsnek · 2018-03-05T01:33:37Z

something somewhere is matching c:\ during startup, if anyone knows what it might be please let me know, i'm searching through all the bootstrap code now

edit: i think its happening here? but i don't know where its being called. i'm still searching around

devsnek · 2018-03-05T16:09:56Z

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

Fishrock123 · 2018-03-05T20:39:42Z

fwiw the commit prefix should be with a comma, probably either tools,bootstrap:, tools,lib: or tools,src.

Fishrock123 · 2018-03-05T20:56:00Z

edit: i think its happening here? but i don't know where its being called. i'm still searching around

It seems that splitRoot code is only used by realpath, which in turn is used for module resolution...

(The code link points to where fs is loaded so... some extra info maybe)

Fs load points in bootstrap

On BSD that happens here:

node/lib/internal/bootstrap_node.js

Lines 78 to 81 in 1d2ab79

    
           // On OpenBSD process.execPath will be relative unless we 
        
           // get the full path before process.execPath is used. 
        
           if (process.platform === 'openbsd') { 
        
             const { realpathSync } = NativeModule.require('fs');

If ESM is enabled, it is possible that this tree of loads loads fs

node/lib/internal/bootstrap_node.js

Lines 106 to 110 in 1d2ab79

    
           if (process.binding('config').experimentalModules) { 
        
             process.emitWarning( 
        
               'The ESM module loader is experimental.', 
        
               'ExperimentalWarning', undefined); 
        
             NativeModule.require('internal/process/modules').setup();

If there are --require modules, fs is loaded from module here:

node/lib/internal/bootstrap_node.js

Lines 583 to 586 in 1d2ab79

    
           // Load preload modules 
        
           function preloadModules() { 
        
             if (process._preload_modules) { 
        
               NativeModule.require('module')._preloadModules(process._preload_modules);

Otherwise, fs is loaded from module here:

node/lib/internal/bootstrap_node.js

Line 201 in 1d2ab79

const Module = NativeModule.require('module');

hashseed · 2018-03-06T22:00:27Z

tools/js2c.py

+    # if its a gypi file we're going to want it as json
+    # later on anyway, so get it out of the way now
+    if name.endswith(".gypi"):
+      lines = re.sub(r'#.*?\n', '', lines)


Does python maybe offer a package that converts a dict into json? That would be cleaner than replacing characters.

#19140 (review)

ah. missed that context. thanks.

joyeecheung

LGTM

devsnek · 2018-03-09T18:39:12Z

landed in 7314b17

PR-URL: #19140 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

PR-URL: nodejs#19140 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

PR-URL: #19140 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

nodejs-github-bot added build Issues and PRs related to build files or the CI. process Issues and PRs related to the process subsystem. labels Mar 5, 2018

joyeecheung reviewed Mar 5, 2018

View reviewed changes

tools/bootstrap_node: preprocess gypi files to json

6a9dfcc

devsnek force-pushed the js2c-gypi branch from 2c93d33 to 6a9dfcc Compare March 5, 2018 16:08

hashseed reviewed Mar 6, 2018

View reviewed changes

devsnek mentioned this pull request Mar 6, 2018

bootstrap_node: remove side effects on RegExp #19138

Closed

3 tasks

addaleax approved these changes Mar 7, 2018

View reviewed changes

devsnek added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 8, 2018

devsnek requested a review from joyeecheung March 9, 2018 01:04

joyeecheung approved these changes Mar 9, 2018

View reviewed changes

devsnek closed this Mar 9, 2018

devsnek added a commit that referenced this pull request Mar 9, 2018

tools,bootstrap: preprocess gypi files to json

7314b17

PR-URL: #19140 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

targos pushed a commit that referenced this pull request Mar 17, 2018

tools,bootstrap: preprocess gypi files to json

3e4e0ad

PR-URL: #19140 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

devsnek removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 18, 2018

targos mentioned this pull request Mar 18, 2018

v9.9.0 proposal #19428

Merged

MylesBorins pushed a commit that referenced this pull request Mar 20, 2018

tools,bootstrap: preprocess gypi files to json

9613e02

PR-URL: #19140 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

gireeshpunathil mentioned this pull request Mar 29, 2018

lib: bootstrap should not have RegExp side effects #18930

Closed

devsnek deleted the js2c-gypi branch March 29, 2018 12:26

MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018

tools,bootstrap: preprocess gypi files to json

fac7c66

PR-URL: nodejs#19140 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

MylesBorins pushed a commit that referenced this pull request Aug 17, 2018

tools,bootstrap: preprocess gypi files to json

4df93dc

PR-URL: #19140 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

MylesBorins mentioned this pull request Aug 17, 2018

v8.12.0 proposal #21593

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tools/bootstrap_node: preprocess gypi files to json #19140

tools/bootstrap_node: preprocess gypi files to json #19140

devsnek commented Mar 5, 2018 •

edited

Loading

devsnek commented Mar 5, 2018

joyeecheung Mar 5, 2018 •

edited

Loading

devsnek Mar 5, 2018

bnoordhuis Mar 5, 2018

rvagg commented Mar 5, 2018

devsnek commented Mar 5, 2018 •

edited

Loading

devsnek commented Mar 5, 2018

Fishrock123 commented Mar 5, 2018 •

edited

Loading

Fishrock123 commented Mar 5, 2018

hashseed Mar 6, 2018

devsnek Mar 6, 2018

hashseed Mar 6, 2018

joyeecheung left a comment

devsnek commented Mar 9, 2018

	s = re.sub(r'#.*?\n', '', s) # strip comments
	s = re.sub(r'\'', '"', s) # convert quotes

tools/bootstrap_node: preprocess gypi files to json #19140

tools/bootstrap_node: preprocess gypi files to json #19140

Conversation

devsnek commented Mar 5, 2018 • edited Loading

Checklist

Affected core subsystem(s)

devsnek commented Mar 5, 2018

joyeecheung Mar 5, 2018 • edited Loading

Choose a reason for hiding this comment

devsnek Mar 5, 2018

Choose a reason for hiding this comment

bnoordhuis Mar 5, 2018

Choose a reason for hiding this comment

rvagg commented Mar 5, 2018

devsnek commented Mar 5, 2018 • edited Loading

devsnek commented Mar 5, 2018

Fishrock123 commented Mar 5, 2018 • edited Loading

Fishrock123 commented Mar 5, 2018

hashseed Mar 6, 2018

Choose a reason for hiding this comment

devsnek Mar 6, 2018

Choose a reason for hiding this comment

hashseed Mar 6, 2018

Choose a reason for hiding this comment

joyeecheung left a comment

Choose a reason for hiding this comment

devsnek commented Mar 9, 2018

devsnek commented Mar 5, 2018 •

edited

Loading

joyeecheung Mar 5, 2018 •

edited

Loading

devsnek commented Mar 5, 2018 •

edited

Loading

Fishrock123 commented Mar 5, 2018 •

edited

Loading