tools/bootstrap_node: preprocess gypi files to json by devsnek · Pull Request #19140 · nodejs/node · GitHub

Checklist

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

Affected core subsystem(s)

bootstrap, tools

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

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

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.

no good on windows

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

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

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

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');

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.

LGTM

landed in 7314b17

PR-URL: #19140 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

PR-URL: nodejs#19140 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

PR-URL: #19140 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

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 outstanding review comments, 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 <anna@addaleax.net> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

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 <anna@addaleax.net> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

devsnek removed the author ready PRs that have at least one approval, no outstanding review comments, 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 <anna@addaleax.net> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

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 <anna@addaleax.net> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

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 <anna@addaleax.net> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

MylesBorins mentioned this pull request Aug 17, 2018

v8.12.0 proposal #21593

Merged

+                  # 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)

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

tools/bootstrap_node: preprocess gypi files to json by devsnek · Pull Request #19140 · nodejs/node · GitHub

Uh oh!

Uh oh!

tools/bootstrap_node: preprocess gypi files to json - #19140

tools/bootstrap_node: preprocess gypi files to json#19140devsnek wants to merge 1 commit intonodejs:masternodejs/node:masterfrom devsnek:js2c-gypidevsnek/node:js2c-gypiCopy head branch name to clipboard

Conversation

devsnek commented Mar 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

devsnek commented Mar 5, 2018

Uh oh!

joyeecheung Mar 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devsnek Mar 5, 2018

Choose a reason for hiding this comment

Uh oh!

bnoordhuis Mar 5, 2018

Choose a reason for hiding this comment

Uh oh!

rvagg commented Mar 5, 2018

Uh oh!

devsnek commented Mar 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

devsnek commented Mar 5, 2018

Uh oh!

Fishrock123 commented Mar 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Fishrock123 commented Mar 5, 2018

Uh oh!

hashseed Mar 6, 2018

Choose a reason for hiding this comment

Uh oh!

devsnek Mar 6, 2018

Choose a reason for hiding this comment

Uh oh!

hashseed Mar 6, 2018

Choose a reason for hiding this comment

Uh oh!

joyeecheung left a comment

Choose a reason for hiding this comment

Uh oh!

devsnek commented Mar 9, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

tools/bootstrap_node: preprocess gypi files to json#19140
devsnek wants to merge 1 commit into
nodejs:masterfrom
devsnek:js2c-gypi

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