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

feat: build with config.gypi from node headers #2497

Merged
merged 1 commit into from
Nov 5, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,22 @@ Python executable, it will be used instead of any of the other configured or
builtin Python search paths. If it's not a compatible version, no further
searching will be done.

### Build for Third Party Node.js Runtimes

When building modules for thid party Node.js runtimes like Electron, which have
different build configurations from the official Node.js distribution, you
should use `--dist-url` or `--nodedir` flags to specify the headers of the
runtime to build for.

Also when `--dist-url` or `--nodedir` flags are passed, node-gyp will use the
`config.gypi` shipped in the headers distribution to generate build
configurations, which is different from the default mode that would use the
`process.config` object of the running Node.js instance.

Some old versions of Electron shipped malformed `config.gypi` in their headers
distributions, and you might need to pass `--force-process-config` to node-gyp
to work around configuration errors.

## How to Use

To compile your native addon, first go to its root directory:
Expand Down Expand Up @@ -198,6 +214,7 @@ Some additional resources for Node.js native addons and writing `gyp` configurat
| `--python=$path` | Set path to the Python binary
| `--msvs_version=$version` | Set Visual Studio version (Windows only)
| `--solution=$solution` | Set Visual Studio Solution version (Windows only)
| `--force-process-config` | Force using runtime's `process.config` object to generate `config.gypi` file

## Configuration

Expand Down
12 changes: 5 additions & 7 deletions lib/configure.js
Original file line number Diff line number Diff line change
Expand Up @@ -97,17 +97,15 @@ function configure (gyp, argv, callback) {
process.env.GYP_MSVS_VERSION = Math.min(vsInfo.versionYear, 2015)
process.env.GYP_MSVS_OVERRIDE_PATH = vsInfo.path
}
createConfigGypi({ gyp, buildDir, nodeDir, vsInfo }, (err, configPath) => {
createConfigGypi({ gyp, buildDir, nodeDir, vsInfo }).then(configPath => {
configs.push(configPath)
findConfigs(err)
findConfigs()
zcbenz marked this conversation as resolved.
Show resolved Hide resolved
}).catch(err => {
callback(err)
})
}

function findConfigs (err) {
if (err) {
return callback(err)
}

function findConfigs () {
var name = configNames.shift()
if (!name) {
return runGyp()
Expand Down
49 changes: 38 additions & 11 deletions lib/create-config-gypi.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,19 +4,45 @@ const fs = require('graceful-fs')
const log = require('npmlog')
const path = require('path')

function getBaseConfigGypi () {
const config = JSON.parse(JSON.stringify(process.config))
function parseConfigGypi (config) {
// translated from tools/js2c.py of Node.js
// 1. string comments
config = config.replace(/#.*/g, '')
// 2. join multiline strings
config = config.replace(/'$\s+'/mg, '')
Copy link
Member

Choose a reason for hiding this comment

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

do we have any examples of this in config.gypi? I'm looking but can't see any, maybe this is safe to include but it's a bit of an uncomfortable regex that would be nice to see tested against an example (even if just manually)

Copy link
Contributor Author

@zcbenz zcbenz Nov 1, 2021

Choose a reason for hiding this comment

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

A test was added for this in test/test-create-config-gypi.js:

  const str = "# Some comments\n{'variables': {'multiline': 'A'\n'B'}}"
  const config = parseConfigGypi(str)
  t.deepEqual(config, { variables: { multiline: 'AB' } })

// 3. normalize string literals from ' into "
config = config.replace(/'/g, '"')
return JSON.parse(config)
}

async function getBaseConfigGypi ({ gyp, nodeDir }) {
// try reading $nodeDir/include/node/config.gypi first when:
// 1. --dist-url or --nodedir is specified
// 2. and --force-process-config is not specified
const shouldReadConfigGypi = (gyp.opts.nodedir || gyp.opts['dist-url']) && !gyp.opts['force-process-config']
if (shouldReadConfigGypi && nodeDir) {
try {
const baseConfigGypiPath = path.resolve(nodeDir, 'include/node/config.gypi')
const baseConfigGypi = await fs.promises.readFile(baseConfigGypiPath)
return parseConfigGypi(baseConfigGypi.toString())
} catch (err) {
log.warn('read config.gypi', err.message)
}
}

// fallback to process.config if it is invalid
return JSON.parse(JSON.stringify(process.config))
}

async function getCurrentConfigGypi ({ gyp, nodeDir, vsInfo }) {
const config = await getBaseConfigGypi({ gyp, nodeDir })
if (!config.target_defaults) {
config.target_defaults = {}
}
if (!config.variables) {
config.variables = {}
}
return config
}

function getCurrentConfigGypi ({ gyp, nodeDir, vsInfo }) {
const config = getBaseConfigGypi()
const defaults = config.target_defaults
const variables = config.variables

Expand Down Expand Up @@ -85,13 +111,13 @@ function getCurrentConfigGypi ({ gyp, nodeDir, vsInfo }) {
return config
}

function createConfigGypi ({ gyp, buildDir, nodeDir, vsInfo }, callback) {
async function createConfigGypi ({ gyp, buildDir, nodeDir, vsInfo }) {
const configFilename = 'config.gypi'
const configPath = path.resolve(buildDir, configFilename)

log.verbose('build/' + configFilename, 'creating config file')

const config = getCurrentConfigGypi({ gyp, nodeDir, vsInfo })
const config = await getCurrentConfigGypi({ gyp, nodeDir, vsInfo })

// ensures that any boolean values in config.gypi get stringified
function boolsToString (k, v) {
Expand All @@ -108,12 +134,13 @@ function createConfigGypi ({ gyp, buildDir, nodeDir, vsInfo }, callback) {

const json = JSON.stringify(config, boolsToString, 2)
log.verbose('build/' + configFilename, 'writing out config file: %s', configPath)
fs.writeFile(configPath, [prefix, json, ''].join('\n'), (err) => {
callback(err, configPath)
})
await fs.promises.writeFile(configPath, [prefix, json, ''].join('\n'))

return configPath
}

module.exports = createConfigGypi
module.exports.test = {
parseConfigGypi: parseConfigGypi,
getCurrentConfigGypi: getCurrentConfigGypi
}
3 changes: 2 additions & 1 deletion lib/node-gyp.js
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,8 @@ proto.configDefs = {
'dist-url': String, // 'install'
tarball: String, // 'install'
jobs: String, // 'build'
thin: String // 'configure'
thin: String, // 'configure'
'force-process-config': Boolean // 'configure'
}

/**
Expand Down
6 changes: 6 additions & 0 deletions test/fixtures/nodedir/include/node/config.gypi
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
# Test configuration
{
'variables': {
'build_with_electron': true
}
}
5 changes: 4 additions & 1 deletion test/test-configure-python.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,10 @@ const configure = requireInject('../lib/configure', {
closeSync: function () { },
writeFile: function (file, data, cb) { cb() },
stat: function (file, cb) { cb(null, {}) },
mkdir: function (dir, options, cb) { cb() }
mkdir: function (dir, options, cb) { cb() },
promises: {
writeFile: function (file, data) { return Promise.resolve(null) }
}
}
})

Expand Down
47 changes: 40 additions & 7 deletions test/test-create-config-gypi.js
Original file line number Diff line number Diff line change
@@ -1,37 +1,70 @@
'use strict'

const path = require('path')
const { test } = require('tap')
const gyp = require('../lib/node-gyp')
const createConfigGypi = require('../lib/create-config-gypi')
const { getCurrentConfigGypi } = createConfigGypi.test
const { parseConfigGypi, getCurrentConfigGypi } = createConfigGypi.test

test('config.gypi with no options', function (t) {
test('config.gypi with no options', async function (t) {
t.plan(2)

const prog = gyp()
prog.parseArgv([])

const config = getCurrentConfigGypi({ gyp: prog, vsInfo: {} })
const config = await getCurrentConfigGypi({ gyp: prog, vsInfo: {} })
t.equal(config.target_defaults.default_configuration, 'Release')
t.equal(config.variables.target_arch, process.arch)
})

test('config.gypi with --debug', function (t) {
test('config.gypi with --debug', async function (t) {
t.plan(1)

const prog = gyp()
prog.parseArgv(['_', '_', '--debug'])

const config = getCurrentConfigGypi({ gyp: prog, vsInfo: {} })
const config = await getCurrentConfigGypi({ gyp: prog, vsInfo: {} })
t.equal(config.target_defaults.default_configuration, 'Debug')
})

test('config.gypi with custom options', function (t) {
test('config.gypi with custom options', async function (t) {
t.plan(1)

const prog = gyp()
prog.parseArgv(['_', '_', '--shared-libxml2'])

const config = getCurrentConfigGypi({ gyp: prog, vsInfo: {} })
const config = await getCurrentConfigGypi({ gyp: prog, vsInfo: {} })
t.equal(config.variables.shared_libxml2, true)
})

test('config.gypi with nodedir', async function (t) {
t.plan(1)

const nodeDir = path.join(__dirname, 'fixtures', 'nodedir')

const prog = gyp()
prog.parseArgv(['_', '_', `--nodedir=${nodeDir}`])

const config = await getCurrentConfigGypi({ gyp: prog, nodeDir, vsInfo: {} })
t.equal(config.variables.build_with_electron, true)
})

test('config.gypi with --force-process-config', async function (t) {
t.plan(1)

const nodeDir = path.join(__dirname, 'fixtures', 'nodedir')

const prog = gyp()
prog.parseArgv(['_', '_', '--force-process-config', `--nodedir=${nodeDir}`])

const config = await getCurrentConfigGypi({ gyp: prog, nodeDir, vsInfo: {} })
t.equal(config.variables.build_with_electron, undefined)
})

test('config.gypi parsing', function (t) {
t.plan(1)

const str = "# Some comments\n{'variables': {'multiline': 'A'\n'B'}}"
const config = parseConfigGypi(str)
t.deepEqual(config, { variables: { multiline: 'AB' } })
})