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

Introduce new build targets for debugging purpose #186

Closed
wants to merge 1 commit into from

Conversation

romandev
Copy link
Contributor

@romandev romandev commented Nov 17, 2017

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

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

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

DaAitch commented Jun 12, 2018

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

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

@DaAitch
Copy link
Contributor

DaAitch commented Dec 28, 2018

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

Copy link
Contributor

@gabrielschulhof gabrielschulhof left a comment

Choose a reason for hiding this comment

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

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

@mhdawson
Copy link
Member

mhdawson commented Jan 2, 2019

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

@gabrielschulhof
Copy link
Contributor

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

mhdawson commented Jan 4, 2019

@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
Copy link
Contributor

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

@DaAitch
Copy link
Contributor

DaAitch commented Jan 4, 2019

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

mhdawson commented Jan 7, 2019

+1 for adding additional targets.

Copy link
Contributor

@gabrielschulhof gabrielschulhof left a comment

Choose a reason for hiding this comment

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

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

@gabrielschulhof gabrielschulhof dismissed their stale review January 15, 2019 18:05

A better idea has emerged.

@romandev romandev changed the title Use incremental builds rather than full builds Introduce new build targets for debugging purpose Jan 17, 2019
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.
Copy link
Contributor

@gabrielschulhof gabrielschulhof left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@NickNaso NickNaso left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

mhdawson pushed a commit that referenced this pull request Mar 4, 2019
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 <gabriel.schulhof@intel.com>
Reviewed-By: NickNaso <nicoladelgobbo@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@mhdawson
Copy link
Member

mhdawson commented Mar 4, 2019

Landed as fcfc612

@mhdawson mhdawson closed this Mar 4, 2019
kevindavies8 added a commit to kevindavies8/node-addon-api-Develop that referenced this pull request Aug 24, 2022
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 <gabriel.schulhof@intel.com>
Reviewed-By: NickNaso <nicoladelgobbo@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Marlyfleitas added a commit to Marlyfleitas/node-api-addon-Development that referenced this pull request Aug 26, 2022
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 <gabriel.schulhof@intel.com>
Reviewed-By: NickNaso <nicoladelgobbo@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
wroy7860 added a commit to wroy7860/addon-api-benchmark-node that referenced this pull request Sep 19, 2022
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 <gabriel.schulhof@intel.com>
Reviewed-By: NickNaso <nicoladelgobbo@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
johnfrench3 pushed a commit to johnfrench3/node-addon-api-git that referenced this pull request Aug 11, 2023
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 <gabriel.schulhof@intel.com>
Reviewed-By: NickNaso <nicoladelgobbo@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants