Allow build directory to be changed. #919

peterbraden · 2016-05-06T10:31:24Z

Change with NODE_GYP_BUILD_DIR environment variable

Fixes #263

bnoordhuis · 2016-05-06T10:35:59Z

It should be a switch, like --builddir. The way node-gyp works, it can then also be configured by setting npm_config_builddir in the environment. See #916 for an example.

guymguym · 2016-08-08T23:29:51Z

Planning to merge? Would be great to allow it for tensorflow.

bnoordhuis · 2016-08-09T10:46:29Z

lib/configure.js


    // now write out the config.gypi file to the build/ dir
    var prefix = '# Do not edit. File was generated by node-gyp\'s "configure" step'
      , json = JSON.stringify(config, boolsToString, 2)
-    log.verbose('build/' + configFilename, 'writing out config file: %s', configPath)
+    log.verbose(gyp.buildDir + '/' + configFilename, 'writing out config file: %s', configPath)


Can you wrap this line at 80 columns? I'd break at the the comma, then line up the second argument with the first one.

bnoordhuis · 2016-08-09T10:49:15Z

@peterbraden Can you add a regression test to test/test-addon.js?

peterbraden · 2016-08-17T11:52:28Z

added a very basic test.

bnoordhuis · 2016-08-18T08:39:35Z

test/test-addon.js

+      t.strictEqual(binding.hello(), 'world')
+
+      // Cleanup
+      exec('rm -rf foo', t.end)


This won't work on Windows. Can you run node-gyp --build_dir=foo clean?

Naming bikeshed: what about calling it --build-dir? That's more in line with --dist-url. (OTOH there's --msvs_version but that mimics the gyp variable.)

bnoordhuis · 2016-10-07T08:59:34Z

@peterbraden Can you rebase and squash? Thanks.

Change with `--build-dir` parameter. ie: ``` node-gyp --build-dir=foo rebuild ``` PR nodejs#919 Fixes nodejs#263

peterbraden · 2016-10-24T09:24:07Z

Done

rvagg · 2016-11-08T10:27:19Z

Your test doesn't work, for two reasons: require cache saving "hello_world" and hello.js using the bindings module to load from the existing build directory. So here's a patch to fix your test:

diff --git a/test/test-addon.js b/test/test-addon.js
index 4b8bc5e..2816cd1 100644
--- a/test/test-addon.js
+++ b/test/test-addon.js
@@ -7,6 +7,20 @@ var exec = childProcess.exec
 var path = require('path')
 var addonPath = path.resolve(__dirname, 'node_modules', 'hello_world')
 var nodeGyp = path.resolve(__dirname, '..', 'bin', 'node-gyp.js')
+var rimraf = require('rimraf')
+
+function cleanup (dir) {
+  return function teardown (t) {
+    Object.keys(require.cache)
+      .forEach(function (d) {
+        if (d.indexOf(addonPath) == 0)
+          delete require.cache[d]
+      })
+    rimraf(dir, t.end)
+  }
+}
+
+test('build simple addon - setup', cleanup(path.join(addonPath, 'build')))

 test('build simple addon', function (t) {
   t.plan(3)
@@ -29,6 +43,10 @@ test('build simple addon', function (t) {
   proc.stderr.setEncoding('utf-8')
 })

+test('build simple addon - teardown', cleanup(path.join(addonPath, 'build')))
+
+test('build with different build dir - setup', cleanup(path.join(addonPath, 'foo')))
+
 test('build with different build dir', function(t) {
   var cmd = [nodeGyp, 'rebuild', '-C',
               addonPath, '--loglevel=verbose', '--build-dir=foo']
@@ -39,7 +57,7 @@ test('build with different build dir', function(t) {
     t.strictEqual(lastLine, 'gyp info ok', 'should end in ok')

     try {
-      var binding = require('hello_world')
+      var binding = require(path.join(addonPath, 'foo/Release/hello.node'))
       t.strictEqual(binding.hello(), 'world')

       // Cleanup
@@ -52,3 +70,5 @@ test('build with different build dir', function(t) {
   proc.stdout.setEncoding('utf-8')
   proc.stderr.setEncoding('utf-8')
 })
+
+test('build with different build dir - teardown', cleanup(path.join(addonPath, 'foo')))

Fishrock123 · 2016-11-21T17:10:45Z

ping @peterbraden

Change with `--build-dir` parameter. ie: ``` node-gyp --build-dir=foo rebuild ``` PR nodejs#919 Fixes nodejs#263

peterbraden · 2016-11-23T07:56:49Z

Sorry, been busy with a new job. Done now, and squashed and rebased.

Thanks!

bnoordhuis · 2016-11-23T10:24:19Z

test/test-addon.js

+      t.strictEqual(binding.hello(), 'world')
+
+      // Cleanup
+      exec('node-gyp --build-dir=foo clean', t.end)


Can you use execFile(process.execPath, [nodeGyp, ...]) here? Calling out to node-gyp risks picking up the version that is on the path, not what's under test.

Aside: I know they're Rod's work but can you wrap lines at 80 columns? Thanks. :-)

Change with `--build-dir` parameter. ie: ``` node-gyp --build-dir=foo rebuild ``` PR nodejs#919 Fixes nodejs#263

peterbraden · 2016-12-02T11:24:38Z

Updated, squashed

bnoordhuis

LGTM, thanks. CI: https://ci.nodejs.org/view/All/job/nodegyp-test-commit/33/

Fishrock123 · 2016-12-05T17:41:05Z

CI failed on windows, @bnoordhuis could you take a look?

richardlau · 2016-12-07T06:45:44Z

Test failures on Windows are because the newly introduced cleanup() function in the tests is trying to remove a currently loaded .node file. Even if the module is deleted from require.cache the .node is not unloaded (Is it even possible to unload a required .node in Node.js?).

bnoordhuis · 2016-12-07T12:02:49Z

(Is it even possible to unload a required .node in Node.js?)

No.

richardlau · 2016-12-13T18:47:15Z

I don't think clearing entries from require.cache is needed, because the second require() call that was introduced to the test is requiring hello.node from a different location (path.join(addonPath, 'foo/Release/hello.node')) than hello.node that is required from require('hello_world') (via the bindings module) so would end up as a separate entry in the cache.

If the fear is contamination of the second test from the first, then perhaps the new test could be in a separate file? Or does tape run all tests in the same process?

bnoordhuis · 2017-02-15T04:04:48Z

@peterbraden Are you still interested in pursuing this? From #919 (comment) it sounds like it's almost there, it just needs one or two small fix-ups.

If the fear is contamination of the second test from the first, then perhaps the new test could be in a separate file? Or does tape run all tests in the same process?

I think tape uses a process-per-file model so that sounds like a good idea.

peterbraden · 2017-02-15T08:58:51Z

Sure, let me try when I have some free time, maybe this weekend.

bnoordhuis · 2017-02-20T11:59:48Z

I found out the hard way today that tape runs all tests in the same process. I've filed #1123.

599316527 · 2018-08-29T14:06:29Z

Has this feature been merged into master? I really need it.

jlipps · 2019-06-01T03:21:08Z

@peterbraden I'd still be interested in this patch if you want to resurrect it :-)

rvagg · 2019-06-21T03:19:08Z

if @peterbraden or someone else wants to pick this up, please either rebase this or take the code and open a new PR against the current master
test failures because of the way tape works should be fixed once we migrate to tap which I hope we can achieve soon.

peterbraden · 2019-06-21T08:07:17Z

I don't have time for this any more, hopefully someone else can take this on.

bnoordhuis · 2020-05-06T09:35:57Z

It's with slightly sad eyes that I'm closing this out. I just closed out #263 after no one stepped forward. Thanks anyway, @peterbraden!

ghost · 2021-08-06T18:05:21Z

I'm interested in helping to get this issue over the line, is the "tap/tape" thing mentioned above still an issue? What remains to be done here.

cclauss · 2021-08-06T18:17:44Z

https://github.com/nodejs/tap2junit

peterbraden mentioned this pull request May 9, 2016

[WIP] Initial proof-of-concept NodeJS binding. tensorflow/tensorflow#2206

Closed

bnoordhuis reviewed Aug 9, 2016
View reviewed changes

bnoordhuis reviewed Aug 18, 2016
View reviewed changes

peterbraden force-pushed the master branch from 1f18133 to faee61c Compare October 24, 2016 09:23

peterbraden added a commit to peterbraden/node-gyp that referenced this pull request Oct 24, 2016

Allow build directory to be changed.

faee61c

Change with `--build-dir` parameter. ie: ``` node-gyp --build-dir=foo rebuild ``` PR nodejs#919 Fixes nodejs#263

bnoordhuis mentioned this pull request Oct 27, 2016

Add --devdir flag. #916

Merged

bnoordhuis mentioned this pull request Nov 7, 2016

Propose release: v3.5.0 #1049

Closed

peterbraden added a commit to peterbraden/node-gyp that referenced this pull request Nov 23, 2016

Allow build directory to be changed.

63d9266

Change with `--build-dir` parameter. ie: ``` node-gyp --build-dir=foo rebuild ``` PR nodejs#919 Fixes nodejs#263

peterbraden force-pushed the master branch from 3710160 to f0d5014 Compare November 23, 2016 07:56

peterbraden added a commit to peterbraden/node-gyp that referenced this pull request Nov 23, 2016

Allow build directory to be changed.

f0d5014

Change with `--build-dir` parameter. ie: ``` node-gyp --build-dir=foo rebuild ``` PR nodejs#919 Fixes nodejs#263

bnoordhuis requested changes Nov 23, 2016

View reviewed changes

Allow build directory to be changed.

f0f078f

Change with `--build-dir` parameter. ie: ``` node-gyp --build-dir=foo rebuild ``` PR nodejs#919 Fixes nodejs#263

peterbraden force-pushed the master branch from f0d5014 to f0f078f Compare December 2, 2016 11:23

bnoordhuis approved these changes Dec 2, 2016

View reviewed changes

rvagg force-pushed the master branch from 1456ef2 to 7a71d68 Compare April 24, 2019 00:37

rvagg force-pushed the master branch from ed3ff7e to 611bc3c Compare June 21, 2019 02:25

rvagg force-pushed the master branch from bacf53d to 4ef83ed Compare July 22, 2019 03:16

bnoordhuis mentioned this pull request Apr 28, 2020

Configurable build dir #263

Closed

bnoordhuis closed this May 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow build directory to be changed. #919

Allow build directory to be changed. #919

peterbraden commented May 6, 2016

bnoordhuis commented May 6, 2016

guymguym commented Aug 8, 2016

bnoordhuis Aug 9, 2016

bnoordhuis commented Aug 9, 2016

peterbraden commented Aug 17, 2016

bnoordhuis Aug 18, 2016

bnoordhuis commented Oct 7, 2016

peterbraden commented Oct 24, 2016

rvagg commented Nov 8, 2016

Fishrock123 commented Nov 21, 2016

peterbraden commented Nov 23, 2016

bnoordhuis Nov 23, 2016

peterbraden commented Dec 2, 2016

bnoordhuis left a comment

Fishrock123 commented Dec 5, 2016

richardlau commented Dec 7, 2016

bnoordhuis commented Dec 7, 2016

richardlau commented Dec 13, 2016

bnoordhuis commented Feb 15, 2017

peterbraden commented Feb 15, 2017

bnoordhuis commented Feb 20, 2017

599316527 commented Aug 29, 2018

jlipps commented Jun 1, 2019

rvagg commented Jun 21, 2019

peterbraden commented Jun 21, 2019

bnoordhuis commented May 6, 2020

ghost commented Aug 6, 2021

cclauss commented Aug 6, 2021

Allow build directory to be changed. #919

Allow build directory to be changed. #919

Conversation

peterbraden commented May 6, 2016

bnoordhuis commented May 6, 2016

guymguym commented Aug 8, 2016

bnoordhuis Aug 9, 2016

Choose a reason for hiding this comment

bnoordhuis commented Aug 9, 2016

peterbraden commented Aug 17, 2016

bnoordhuis Aug 18, 2016

Choose a reason for hiding this comment

bnoordhuis commented Oct 7, 2016

peterbraden commented Oct 24, 2016

rvagg commented Nov 8, 2016

Fishrock123 commented Nov 21, 2016

peterbraden commented Nov 23, 2016

bnoordhuis Nov 23, 2016

Choose a reason for hiding this comment

peterbraden commented Dec 2, 2016

bnoordhuis left a comment

Choose a reason for hiding this comment

Fishrock123 commented Dec 5, 2016

richardlau commented Dec 7, 2016

bnoordhuis commented Dec 7, 2016

richardlau commented Dec 13, 2016

bnoordhuis commented Feb 15, 2017

peterbraden commented Feb 15, 2017

bnoordhuis commented Feb 20, 2017

599316527 commented Aug 29, 2018

jlipps commented Jun 1, 2019

rvagg commented Jun 21, 2019

peterbraden commented Jun 21, 2019

bnoordhuis commented May 6, 2020

ghost commented Aug 6, 2021

cclauss commented Aug 6, 2021