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: add build targets for easier build/clean #20905

Closed
wants to merge 1 commit into from

Conversation

kenny-y
Copy link
Contributor

@kenny-y kenny-y commented May 23, 2018

Adding new build targets: 'bench-addons' & 'bench-addons-clean'.
With these two, it'll be easier to manage the dependencies among
targets and easier to build/clean the addons which are being
used in benchmarking.

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

@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label May 23, 2018
Makefile Outdated
@echo "Please use benchmark/run.js or benchmark/compare.js to run the benchmarks."

# Build required addons for benchmark before running it.
.PHONY: bench-addons
bench-addons: benchmark/misc/function_call/build/Release/binding.node
Copy link
Member

@richardlau richardlau May 23, 2018

Choose a reason for hiding this comment

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

This was previously the bench-misc target, which was removed along with most of the bench-* targets in #18150.

Ref: #17053
cc @joyeecheung @nodejs/benchmarking

Edit: Sorry I can't read properly. At least this late at night.

Copy link
Member

Choose a reason for hiding this comment

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

The previous plan was to invoke node-gyp inside the benchmark, but I couldn't make the child process arguments work as intended.

Copy link
Contributor Author

@kenny-y kenny-y May 28, 2018

Choose a reason for hiding this comment

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

Thanks for the info... so what's the preferred way? Are we taking this approach to do it in Makefile, or using node-gyp to make it be able to run under benchmark dir? I'm neutral to either one.

BTW. I do have something to add to the benchmark dir so I kicked off with the refactoring of this Makefile -- otherwise it'll be too annoying to build/clean a number of addons in benchmark

Copy link
Member

Choose a reason for hiding this comment

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

I think it'd be better if the building can be done inside the benchmark script, since that will be run on Windows, otherwise one will need to update vcbuild.bat for Windows to have this.

@Trott
Copy link
Member

Trott commented May 23, 2018

Personally, I'd prefer we didn't abbreviate "benchmark" to "bench" in the target names, although I know this PR didn't originate that. Just, you know, if someone wanted to change it in another PR, that would be A-OK by me.

@kenny-y
Copy link
Contributor Author

kenny-y commented May 23, 2018

The purpose of this PR is that (a) make the current mechanism better, e.g no longer need to key in make benchmark/misc/function_call/build/Release/binding.node to build, and no longer need to manually rm -rf benchmark/misc/function_call/build/ to clean; (b) if someone wants to add a new bindings benchmark (e.g. a napi-function-call comparing to the existing function-call), it'll be much easier for her/him/zem to manage the new node-gyp build (based on the new targets introduced in this PR).

Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

No strong opinion. Code LGTM

@BridgeAR
Copy link
Member

@nodejs/build PTAL

1 similar comment
@gabrielschulhof
Copy link
Contributor

@nodejs/build PTAL

@Trott
Copy link
Member

Trott commented Jul 10, 2018

ping @nodejs/build-files

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

I think I’d prefer just landing this as opposed to letting it stall.

CI: https://ci.nodejs.org/job/node-test-pull-request/15790/

@kenny-y
Copy link
Contributor Author

kenny-y commented Jul 11, 2018

Oops, I guess this PR is outdated (the dir of the target was moved elsewhere) and I will rebase it on top of most recent changes.

@kenny-y kenny-y force-pushed the benchmark-addons-build branch 3 times, most recently from a9992f3 to 395a6a7 Compare July 13, 2018 07:35
@kenny-y
Copy link
Contributor Author

kenny-y commented Jul 13, 2018

Push the same change again to trigger Travis again to see if it goes green this time (1 out 2 had failed: not ok 2312 sequential/test-inspector-port-zero-cluster)

Makefile Outdated
@echo "Please use benchmark/run.js or benchmark/compare.js to run the benchmarks."

.PHONY: bench
bench:
bench: bench-addons
Copy link
Member

Choose a reason for hiding this comment

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

Not a blocking objection, but:

Currently, all make bench does is print a message telling people to run the tests manually instead, right?

This will build the addons and then tell the person to run their benchmarks manually?

Would it make more sense to not do that and just have people know they need to run make bench-addons if they are running one of the two benchmarks that require it? No one runs make bench anyway unless they stumble upon it. Compiling stuff and then printing a message saying, basically, "don't use this command" seems like something that could be improved.

Makefile Outdated
@echo "Please use benchmark/run.js or benchmark/compare.js to run the benchmarks."

# Build required addons for benchmark before running it.
.PHONY: bench-addons
bench-addons: benchmark/napi/function_call/build/Release/binding.node \
Copy link
Member

Choose a reason for hiding this comment

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

Should this be bench-addons-build instead, similar to (for example) lint-md-build? bench-addons makes it sound like it runs addons benchmarks. This just builds them but does not run them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point.

With these two, it will be easier to manage the dependencies among
targets and easier to build/clean the addons which are being
used in benchmarking.
@Trott
Copy link
Member

Trott commented Jul 16, 2018

@gabrielschulhof
Copy link
Contributor

@gabrielschulhof
Copy link
Contributor

@gabrielschulhof
Copy link
Contributor

Landed in 266c1f4.

gabrielschulhof pushed a commit that referenced this pull request Jul 20, 2018
Adding new build targets: 'bench-addons' & 'bench-addons-clean'. With
these two, it will be easier to manage the dependencies among targets
and easier to build/clean the addons which are being used in
benchmarking.

PR-URL: #20905
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
targos pushed a commit that referenced this pull request Jul 20, 2018
Adding new build targets: 'bench-addons' & 'bench-addons-clean'. With
these two, it will be easier to manage the dependencies among targets
and easier to build/clean the addons which are being used in
benchmarking.

PR-URL: #20905
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@targos targos mentioned this pull request Jul 31, 2018
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants