Introduce new build targets for debugging purpose #186

romandev · 2017-11-17T14:12:22Z

In this project, we can use the following command to test examples.
$ npm test

It might be very inefficient, especially, if the number of files
increases. So, this patch introduces new build targets for debugging
purpose as follows:

$ npm run-script dev                # Build with --debug option
$ npm run-script dev:incremental    # Incremental dev build

This idea comes from @DaAitch.

romandev · 2017-11-17T14:24:08Z

@mhdawson, and others PTAL

I'm not sure if this patch is useful to you. If you want to always make a clean full build for use in CI tool, this patch might not be useful. But if someone like me uses the command in local environment, doing clean full build every time might be very inefficient. WDYT?

mhdawson · 2017-11-17T20:57:12Z

So far the build time is not a big deal, but that does not mean it won't be later on. We do want full builds in the CI. How about adding a new target which would be test-incremental that could be used when people want an incremental instead of full build ?

DaAitch · 2018-06-12T09:06:26Z

Some days before I worked on a PR and I also find out that npm test seems not the right way to go for dev because of the rebuild, so I cd test; node-gyp configure build; node mytest.js but this worked stable only for release. with node-gyp configure --debug sometimes the incremental builds getting me segfaults in the test. Although this PR is not merged or closed within the last 7months I think it's worth to improve the build.

gabrielschulhof · 2018-07-10T23:08:28Z

I personally use ccache to drastically reduce build time. It essentially does an incremental build behind node-gyp's back.

DaAitch · 2018-12-28T02:27:43Z

@mhdawson @gabrielschulhof to recap:
build time is not a big thing for CI, but for writing tests, hacking around etc. short dev cycles are better. Maybe we can have sth. like npm run test:dev to not complete rebuild, but do it like in the PR. It's really more fun. If there is no reason against incremental builds, why not use them?

I also started with a cmake-js branch. I think, cmake is really a much better build tool than node-gyp :). Also think nodejs repo wants to get rid of node-gyp.

gabrielschulhof

As long as "configure build === rebuild" when starting from a clean repo this does not affect CI, and helps out with tinkering locally.

mhdawson · 2019-01-02T21:04:30Z

@gabrielschulhof "configure build === rebuild" would go into the CI job, right?

gabrielschulhof · 2019-01-04T02:08:06Z

@mhdawson do you mean that the build should fail if it turns out that configure build !== rebuild? If not, I don't think we need to make any changes to the CI. I believe that as long as the CI runs npm install && npm test and the tests pass the changes under test should be considered validated by a CI run.

mhdawson · 2019-01-04T14:07:12Z

@gabrielschulhof what I was asking is if by adding a command to the CI run we could ensure it does a full rebuild (even without a clean checkout which I don't think jenkins does by default) instead of an incremental rebuild then I'd be comfortable.

gabrielschulhof · 2019-01-04T21:38:28Z

@mhdawson can we simply rm -rf test/build? That should ensure a full rebuild.

DaAitch · 2019-01-04T23:55:40Z

Wouldn't it just be easier to have more build commands, then it's also documented.

It would also be nice to test "Debug" with --debug because it has tests that are ignored with "Release" and compilation should be faster. On CI I think also the debug tests are needed.

Something like this:

{
  "scripts": {
    "test": "node test",
    "pretest": "node-gyp rebuild -C test",

    "test:dev": "node test",
    "pretest:dev": "node-gyp rebuild -C test --debug",

    "test:dev:incremental": "node test",
    "pretest:dev:incremental": "node-gyp configure build -C test --debug",

    "clean": "node-gyp clean -C test",
    "doc": "doxygen doc/Doxyfile"
  }
}

CI runs

npm run test:dev rebuild with debug tests
npm run test rebuild release

Dev runs:

npm run test:dev:incremental for dev
~~npm run test:incremental~~ I think no incremental release build is needed

mhdawson · 2019-01-07T15:59:11Z

+1 for adding additional targets.

gabrielschulhof

@romandev I believe @DaAitch's idea is pretty good. Could you change the PR to add more targets as described?

A better idea has emerged.

@DaAitch

In this project, we can use the following command to test examples. $ npm test It might be very inefficient, especially, if the number of files increases. So, this patch introduces new build targets for debugging purpose as follows: $ npm run-script dev # Build with --debug option $ npm run-script dev:incremental # Incremental dev build This idea comes from @DaAitch.

gabrielschulhof

LGTM

NickNaso

LGTM

mhdawson

LGTM

@DaAitch

In this project, we can use the following command to test examples. $ npm test It might be very inefficient, especially, if the number of files increases. So, this patch introduces new build targets for debugging purpose as follows: $ npm run-script dev # Build with --debug option $ npm run-script dev:incremental # Incremental dev build This idea comes from @DaAitch. PR-URL: #186 Reviewed-By: Gabriel Schulhof <[email protected]> Reviewed-By: NickNaso <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

mhdawson · 2019-03-04T19:24:38Z

Landed as fcfc612

@DaAitch

In this project, we can use the following command to test examples. $ npm test It might be very inefficient, especially, if the number of files increases. So, this patch introduces new build targets for debugging purpose as follows: $ npm run-script dev # Build with --debug option $ npm run-script dev:incremental # Incremental dev build This idea comes from @DaAitch. PR-URL: nodejs/node-addon-api#186 Reviewed-By: Gabriel Schulhof <[email protected]> Reviewed-By: NickNaso <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

@DaAitch

In this project, we can use the following command to test examples. $ npm test It might be very inefficient, especially, if the number of files increases. So, this patch introduces new build targets for debugging purpose as follows: $ npm run-script dev # Build with --debug option $ npm run-script dev:incremental # Incremental dev build This idea comes from @DaAitch. PR-URL: nodejs/node-addon-api#186 Reviewed-By: Gabriel Schulhof <[email protected]> Reviewed-By: NickNaso <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

@DaAitch

In this project, we can use the following command to test examples. $ npm test It might be very inefficient, especially, if the number of files increases. So, this patch introduces new build targets for debugging purpose as follows: $ npm run-script dev # Build with --debug option $ npm run-script dev:incremental # Incremental dev build This idea comes from @DaAitch. PR-URL: nodejs/node-addon-api#186 Reviewed-By: Gabriel Schulhof <[email protected]> Reviewed-By: NickNaso <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

@DaAitch

In this project, we can use the following command to test examples. $ npm test It might be very inefficient, especially, if the number of files increases. So, this patch introduces new build targets for debugging purpose as follows: $ npm run-script dev # Build with --debug option $ npm run-script dev:incremental # Incremental dev build This idea comes from @DaAitch. PR-URL: nodejs/node-addon-api#186 Reviewed-By: Gabriel Schulhof <[email protected]> Reviewed-By: NickNaso <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

mhdawson force-pushed the master branch from 12fe6f9 to a750ed1 Compare May 8, 2018 19:30

gabrielschulhof previously approved these changes Dec 28, 2018

View reviewed changes

gabrielschulhof reviewed Jan 15, 2019

View reviewed changes

romandev force-pushed the build branch from 33a2327 to 87d2cb9 Compare January 17, 2019 00:19

romandev changed the title ~~Use incremental builds rather than full builds~~ Introduce new build targets for debugging purpose Jan 17, 2019

romandev force-pushed the build branch from 87d2cb9 to 9502eaa Compare January 23, 2019 01:18

gabrielschulhof approved these changes Feb 15, 2019

View reviewed changes

NickNaso approved these changes Feb 15, 2019

View reviewed changes

mhdawson approved these changes Mar 4, 2019

View reviewed changes

mhdawson closed this Mar 4, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce new build targets for debugging purpose #186

Introduce new build targets for debugging purpose #186

romandev commented Nov 17, 2017 •

edited

Loading

romandev commented Nov 17, 2017

mhdawson commented Nov 17, 2017

DaAitch commented Jun 12, 2018 •

edited

Loading

gabrielschulhof commented Jul 10, 2018

DaAitch commented Dec 28, 2018

gabrielschulhof left a comment •

edited

Loading

mhdawson commented Jan 2, 2019

gabrielschulhof commented Jan 4, 2019

mhdawson commented Jan 4, 2019

gabrielschulhof commented Jan 4, 2019

DaAitch commented Jan 4, 2019 •

edited

Loading

mhdawson commented Jan 7, 2019

gabrielschulhof left a comment

gabrielschulhof left a comment

NickNaso left a comment

mhdawson left a comment

mhdawson commented Mar 4, 2019

Introduce new build targets for debugging purpose #186

Introduce new build targets for debugging purpose #186

Conversation

romandev commented Nov 17, 2017 • edited Loading

romandev commented Nov 17, 2017

mhdawson commented Nov 17, 2017

DaAitch commented Jun 12, 2018 • edited Loading

gabrielschulhof commented Jul 10, 2018

DaAitch commented Dec 28, 2018

gabrielschulhof left a comment • edited Loading

Choose a reason for hiding this comment

mhdawson commented Jan 2, 2019

gabrielschulhof commented Jan 4, 2019

mhdawson commented Jan 4, 2019

gabrielschulhof commented Jan 4, 2019

DaAitch commented Jan 4, 2019 • edited Loading

mhdawson commented Jan 7, 2019

gabrielschulhof left a comment

Choose a reason for hiding this comment

gabrielschulhof left a comment

Choose a reason for hiding this comment

NickNaso left a comment

Choose a reason for hiding this comment

mhdawson left a comment

Choose a reason for hiding this comment

mhdawson commented Mar 4, 2019

romandev commented Nov 17, 2017 •

edited

Loading

DaAitch commented Jun 12, 2018 •

edited

Loading

gabrielschulhof left a comment •

edited

Loading

DaAitch commented Jan 4, 2019 •

edited

Loading