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

build,tools: refactor build config into config.json #25798

Closed
wants to merge 3 commits into from

Conversation

refack
Copy link
Contributor

@refack refack commented Jan 29, 2019

First step in a process to normalize and formalize process.config and allow more generic consumption of the build config by other tools as config.json.

Ref: https://mobile.twitter.com/hashseed/status/1090284584116371462

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

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. meta Issues and PRs related to the general management of the project. labels Jan 29, 2019
@refack
Copy link
Contributor Author

refack commented Jan 29, 2019

/CC @nodejs/build-files @nodejs/python @hashseed

@refack refack added python PRs and issues that require attention from people who are familiar with Python. tools Issues and PRs related to the tools directory. and removed meta Issues and PRs related to the general management of the project. labels Jan 29, 2019
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.

This would need to be added to make clean?

@@ -272,8 +276,13 @@ def AddModule(module, source):
if name.endswith('.gypi'):
# Currently only config.gypi is allowed
assert name == 'config.gypi'
lines = re.sub(r'\'true\'', 'true', lines)
lines = re.sub(r'\'false\'', 'false', lines)
config_filename = str(name).replace('gypi', 'json')
Copy link
Member

Choose a reason for hiding this comment

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

Why not just pass config.json to js2c in node.gyp?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the time being I'm assuming 1:1 correspondence, and I want to keep the embedded config_raw data exactly the same using:
https://github.com/nodejs/node/blob/c663277042ed56a221bfbb25b5acf18a1ce2fdbd/tools/js2c.py#L285
So the tight coupling is there anyway...

@joyeecheung
Copy link
Member

joyeecheung commented Jan 29, 2019

On a second thought...@bnoordhuis suggested replacing config.gypi with something closer to JavaScript (in stead of closer to Python) in #24816 , can't we just replace config.gypi with config.json, or does that break anybody?

@refack
Copy link
Contributor Author

refack commented Jan 29, 2019

can't we just replace config.gypi with config.json, or does that break anybody?

Since process.config is documented API, we need to deprecate it in it's current format.
I'm setting things up for an alternative property (maybe call it process.buildConfiguration) that should be exactly that.
https://github.com/nodejs/node/blob/c663277042ed56a221bfbb25b5acf18a1ce2fdbd/tools/js2c.py#L196-L198
that will leave config.gypi as something we can deprecate and later eliminate.

@joyeecheung
Copy link
Member

@refack If it's just for preserving the format of process.config, we can just restore it back in bootstrap/node.js after parsing it though?

process.buildConfiguration

I am going to be -1 if we are going to add yet another property to process..

@joyeecheung
Copy link
Member

joyeecheung commented Jan 29, 2019

BTW, I think this breaks tools/install.py which evaluates config.gypi as a Python dict and uses the variables directly?

Umm...wait, we even distribute config.gypi in the releases under include..

@refack
Copy link
Contributor Author

refack commented Jan 29, 2019

BTW, I think this breaks tools/install.py which evaluates config.gypi as a Python dict
and uses the variables directly?

Yep...
(that why I wanted to do this in small steps, since config.gypi is used in surprising places. For building native addons for example...)

@joyeecheung
Copy link
Member

@refack I believe if we do this in small steps, this commit simply breaks the releases if landed as-is without a follow-up landed in time?

@refack
Copy link
Contributor Author

refack commented Jan 29, 2019

@refack I believe if we do this in small steps, this commit simply breaks the releases if landed as-is without a follow-up landed in time?

Yes, sorry. I Do consider those as bugs and I'll fix them until everything works as at was.

To the clear the goal of this PR is to have the "interesting" parts of config.gypi available as a JSON conforming config.json, while keeping the rest of the tree stable.
After that I'd try to:

  1. Add schema validation to config.json and document the properties.
  2. Replace config.gypi with the JSON file in our GYP scaffold.
  3. Deprecate process.config and try to design a better defined alternative.

@joyeecheung
Copy link
Member

@refack

To the clear the goal of this PR is to have the "interesting" parts of config.gypi available as a JSON conforming config.json, while keeping the rest of the tree stable.

Can't we just rename config.gypi to config.json everywhere and use json.dumps/json.loads in the python scripts? (That is, skip step 1). I don't think it's entirely necessary to remove target_defaults and hoist variables to the top level in the resulting object? That way to maintain the compatibility of install.py I believe we just need to dump it as a json but rename it as config.gypi in the destination.

@refack
Copy link
Contributor Author

refack commented Jan 29, 2019

P.S. I fixed install.py

I don't think it's entirely necessary to remove target_defaults

IMHO we need to start with getting rid of the cruft... For example what's left now in config.gypi is a purly static non-informative wrapper:

# Do not edit. Generated by the configure script.
{ 'target_defaults': { 'cflags': [],
                       'default_configuration': 'Release',
                       'defines': [],
                       'include_dirs': [],
                       'libraries': []},
  'variables': { 'includes': ['config.json']}}

but the scope of where that cruft is required to keep thing from breaking need to be mapped out...

RE you twitter comment, I agree that process.features is exactly the place we should refactor the information to.

@@ -173,6 +166,7 @@ def ignore_inspector_headers(files, dest):
action([
'common.gypi',
'config.gypi',
'config.json',
Copy link
Member

Choose a reason for hiding this comment

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

Does any downstream rely on the variables being in config.gypi instead of being included? (Not sure who to ping but I guess this could make the PR semver-major?)

Copy link
Member

Choose a reason for hiding this comment

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

Can't we just recreate config.gypi in this script? Or does that defeat the purpose of this PR if there is an inconsistency between what's generated in the build and what gets distributed in the tarballs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have considered that. Since they are both created by ./configure we could do that to solve this if we find it's a problem.
I wanted to do it like this to see if we can map the scope by looking for things that break.

(P.S. if we decide to make this semver-major, then I guess it would make sense to add the deprecation of process.config here)

tools/js2c.py Outdated
config = ReadFile(config_filename)
config = re.sub(r'\'true\'', 'true', config)
config = re.sub(r'\'false\'', 'false', config)
config_definition = GetDefinition('build_config', lines)
Copy link
Member

Choose a reason for hiding this comment

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

Following the convention this should be build_config_raw

@jasnell
Copy link
Member

jasnell commented Jan 29, 2019

Since process.config is documented API, we need to deprecate it in it's current format.
I'm setting things up for an alternative property (maybe call it process.buildConfiguration) that should be exactly that.

Also keep in mind that there are userland modules that do weird things with process.config, including completely replacing it with their own objects... which is precisely why we don't rely on it much internally.

tools/js2c.py Outdated Show resolved Hide resolved
@refack
Copy link
Contributor Author

refack commented Jan 30, 2019

@refack
Copy link
Contributor Author

refack commented Jan 30, 2019

Ohh I just remembered why we have process.config be config.gypi verbatim - we use it in node-gyp:
https://github.com/nodejs/node-gyp/blob/53b607426f9e8780e403a82dc5944456f5823336/lib/configure.js#L108

We could still keep it internal

@refack
Copy link
Contributor Author

refack commented Jan 31, 2019

@refack refack self-assigned this Jan 31, 2019
@BridgeAR
Copy link
Member

BridgeAR commented Mar 5, 2019

Ping @refack this failed across the board and it requires rebasing.

@BridgeAR
Copy link
Member

BridgeAR commented Apr 3, 2019

Ping @refack

@cclauss
Copy link
Contributor

cclauss commented Nov 1, 2019

Should we close this one?

@jasnell
Copy link
Member

jasnell commented Jun 25, 2020

This has not been worked on in a while and is out of date. Closing.

@jasnell jasnell closed this Jun 25, 2020
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. python PRs and issues that require attention from people who are familiar with Python. stalled Issues and PRs that are stalled. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants