★ wanayoo — archive 1999 https://github.com/nodejs/node/pull/20905Nouvelle recherche | Portail wanayoo
Skip to content

/node

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

Reviewers

@joyeecheung joyeecheung

@richardlau richardlau

@BridgeAR BridgeAR

@Trott Trott

@addaleax addaleax

Assignees
No one assigned
Labels
build
Projects
None yet
Milestone
No milestone
8 participants
@kenny-y @Trott @BridgeAR @gabrielschulhof @addaleax @joyeecheung @richardlau @nodejs-github-bot
@kenny-y
Copy link
Contributor

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 label May 23, 2018

@richardlau

richardlau reviewed May 23, 2018
View changes

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

This comment has been minimized.

Sign in to view
Copy link
@richardlau

richardlau May 23, 2018 •
edited

Member

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.

This comment has been minimized.

Sign in to view
Copy link
@joyeecheung

joyeecheung May 25, 2018

Member

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

This comment has been minimized.

Sign in to view
Copy link
@kenny-y

kenny-y May 28, 2018 •
edited

Author Contributor

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

This comment has been minimized.

Sign in to view
Copy link
@joyeecheung

joyeecheung May 28, 2018

Member

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

This comment has been minimized.

Sign in to view
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

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

kenny-y commented May 23, 2018 •
edited

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

BridgeAR reviewed May 24, 2018
View changes

Copy link
Member

BridgeAR left a comment

No strong opinion. Code LGTM
@BridgeAR

This comment has been minimized.

Sign in to view
Copy link
Member

BridgeAR commented May 28, 2018

@nodejs/build PTAL
1 similar comment
@gabrielschulhof

This comment has been minimized.

Sign in to view
Copy link
Contributor

gabrielschulhof commented Jul 10, 2018

@nodejs/build PTAL
@Trott

This comment has been minimized.

Sign in to view
Copy link
Member

Trott commented Jul 10, 2018

ping @nodejs/build-files
@addaleax

addaleax approved these changes Jul 10, 2018
View changes

Copy link
Member

addaleax left a comment

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

This comment has been minimized.

Sign in to view
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 kenny-y:benchmark-addons-build branch 3 times, most recently from a9992f3 to 395a6a7 Jul 13, 2018

@kenny-y

This comment has been minimized.

Sign in to view
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)
@Trott

Trott reviewed Jul 13, 2018
View changes

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

.PHONY: bench
bench:
bench: bench-addons

This comment has been minimized.

Sign in to view
Copy link
@Trott

Trott Jul 13, 2018

Member

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

Trott reviewed Jul 13, 2018
View changes

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 \

This comment has been minimized.

Sign in to view
Copy link
@Trott

Trott Jul 13, 2018

Member

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.

This comment has been minimized.

Sign in to view
Copy link
@kenny-y

kenny-y Jul 16, 2018

Author Contributor

Good point.

@kenny-y kenny-y force-pushed the kenny-y:benchmark-addons-build branch from 395a6a7 to d2123e9 Jul 16, 2018

@kenny-y
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.
Loading status checks…
d2123e9
@Trott

Trott approved these changes Jul 16, 2018
View changes

@Trott

This comment has been minimized.

Sign in to view
Copy link
Member

Trott commented Jul 16, 2018

CI: https://ci.nodejs.org/job/node-test-pull-request/15901/
@gabrielschulhof

This comment has been minimized.

Sign in to view
Copy link
Contributor

gabrielschulhof commented Jul 19, 2018

New CI: https://ci.nodejs.org/job/node-test-pull-request/15952/
@gabrielschulhof

This comment has been minimized.

Sign in to view
Copy link
Contributor

gabrielschulhof commented Jul 19, 2018

CI resumed as: https://ci.nodejs.org/job/node-test-pull-request/15954/
@gabrielschulhof

This comment has been minimized.

Sign in to view
Copy link
Contributor

gabrielschulhof commented Jul 20, 2018

Landed in 266c1f4.

@gabrielschulhof gabrielschulhof closed this Jul 20, 2018

gabrielschulhof added a commit that referenced this pull request Jul 20, 2018

@kenny-y @gabrielschulhof
build: add new benchmark targets 
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>
Loading status checks…
266c1f4

targos added a commit that referenced this pull request Jul 20, 2018

@kenny-y @targos
build: add new benchmark targets 
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>
Verified
This commit was signed with a verified signature.
@targos targos Michaël Zasso
GPG key ID: 770F7A9A5AE15600 Learn about signing commits
4f00562

@targos targos referenced this pull request Jul 31, 2018

Merged

v10.8.0 proposal #22040

This was referenced Aug 1, 2018

Closed
Closed
Closed

This was referenced Aug 2, 2018

Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.