Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tools/bootstrap_node: preprocess gypi files to json #19140

Closed
wants to merge 1 commit into from

Conversation

devsnek
Copy link
Member

@devsnek devsnek commented Mar 5, 2018

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

bootstrap, tools

@nodejs-github-bot 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
@devsnek
Copy link
Member Author

devsnek commented Mar 5, 2018

tools/js2c.py Outdated
# 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))
Copy link
Member

@joyeecheung joyeecheung Mar 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

rvagg commented Mar 5, 2018

no good on windows

@devsnek
Copy link
Member Author

devsnek commented Mar 5, 2018

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
Copy link
Member Author

devsnek commented Mar 5, 2018

@Fishrock123
Copy link
Contributor

Fishrock123 commented Mar 5, 2018

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

@Fishrock123
Copy link
Contributor

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:

// 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

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:

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

Otherwise, fs is loaded from module here:

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

# 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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah. missed that context. thanks.

@devsnek 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 devsnek requested a review from joyeecheung March 9, 2018 01:04
Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@devsnek
Copy link
Member Author

devsnek commented Mar 9, 2018

landed in 7314b17

@devsnek devsnek closed this Mar 9, 2018
devsnek added a commit that referenced this pull request Mar 9, 2018
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
PR-URL: #19140
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@devsnek 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 targos mentioned this pull request Mar 18, 2018
MylesBorins pushed a commit that referenced this pull request Mar 20, 2018
PR-URL: #19140
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@devsnek 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
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
PR-URL: #19140
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Aug 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. process Issues and PRs related to the process subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants