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.

Sign up for GitHub

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

Jump to bottom

build: add a make help option for common targets #17323

Merged
merged 1 commit into from
Dec 9, 2017
Merged

build: add a make help option for common targets #17323

merged 1 commit into from
Dec 9, 2017

Conversation

gibfahn
Copy link
Member

@gibfahn gibfahn commented Nov 26, 2017

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

build

The Makefile has quite a large number of targets, so I only added the ones that I thought people would most need to know about.

Output:

image

@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Nov 26, 2017
maclover7
maclover7 approved these changes Nov 26, 2017
View reviewed changes
Copy link
Contributor

@maclover7 maclover7 left a comment

Choose a reason for hiding this comment

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

One comment, otherwise LGTM

Makefile Outdated
@@ -205,7 +207,7 @@ v8:
tools/make-v8.sh
$(MAKE) -C deps/v8 $(V8_ARCH).$(BUILDTYPE_LOWER) $(V8_BUILD_OPTIONS)

test: all
test: all ## Default test target. Runs default tests, linters, and build docs.
Copy link
Contributor

Choose a reason for hiding this comment

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

build docs --> builds docs?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

bnoordhuis
bnoordhuis approved these changes Nov 26, 2017
View reviewed changes
Copy link
Member

@bnoordhuis bnoordhuis left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM but cc @joyeecheung, this PR overlaps to some extent with #16975.

(edit: nevermind, just saw this PR is in response to that PR.)

Makefile Outdated
@@ -1,5 +1,9 @@
-include config.mk

help: ## Print help for targets with comments.
@printf "For more targets and info see the comments in the Makefile.\n\n"
@grep -E '^[a-zA-Z0-9._-]+:.*?## .*$$' Makefile | sort | awk 'BEGIN {FS = ":.*?## "}; {printf "\033[36m%-15s\033[0m %s\n", $$1, $$2}'
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a line break to keep it < 81 columns?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure.

@gibfahn
Copy link
Member Author

gibfahn commented Nov 26, 2017

LGTM but cc @joyeecheung, this PR overlaps to some extent with #16975.

(edit: nevermind, just saw this PR is in response to that PR.)

Yep, this is meant to complement that PR, so would definitely appreciate a review from @joyeecheung !

addaleax
addaleax approved these changes Nov 27, 2017
View reviewed changes
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 really like this.

@gibfahn
Copy link
Member Author

gibfahn commented Nov 27, 2017

Might be too much of a change, but just for info, the alacritty Makefile I got the idea from sets help as the default command, so if you do make it runs make help. Something for a future PR maybe.

https://github.com/jwilm/alacritty/blob/8ff3c5d170abe47554f5d2a41ad35ddc451d254d/Makefile#L18

@richardlau
Copy link
Member

Doesn't make default to the first target?

@gibfahn
Copy link
Member Author

gibfahn commented Nov 27, 2017

@richardlau good point, yes it does, should have rechecked after moving it up to the top.

I've moved it down to below. TBH I think having it be the default makes sense, but I'd rather get this landed so people can start using it, and have that discussion afterwards.

evanlucas
evanlucas reviewed Nov 27, 2017
View reviewed changes
Makefile Outdated
@@ -125,8 +130,6 @@ distclean:

check: test

# Remove files generated by running coverage, put the non-instrumented lib back
# in place
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be removed or just moved down to the next line?

Copy link
Member

Choose a reason for hiding this comment

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

Yes..why are we removing this?

Copy link
Member Author

Choose a reason for hiding this comment

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

My bad, I made it an inline comment, then decided the help was getting too long and removed it again. Fixed.

@refack
Copy link
Contributor

refack commented Nov 27, 2017

Is the ## trick documented anywhere?

joyeecheung
joyeecheung approved these changes Nov 28, 2017
View reviewed changes
Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

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

LGTM minus a question about removing coverage-clean comments

Makefile Outdated
@@ -125,8 +130,6 @@ distclean:

check: test

# Remove files generated by running coverage, put the non-instrumented lib back
# in place
Copy link
Member

Choose a reason for hiding this comment

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

Yes..why are we removing this?

@gibfahn gibfahn force-pushed the make-help branch 2 times, most recently from fba663c to ab388f1 Compare November 28, 2017 12:08
@gibfahn
Copy link
Member Author

gibfahn commented Nov 28, 2017

Is the ## trick documented anywhere?

Added a line to explain.

richardlau
richardlau approved these changes Nov 28, 2017
View reviewed changes
Copy link
Member

@richardlau richardlau left a comment

Choose a reason for hiding this comment

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

New target help should be phony but otherwise LGTM.

@gibfahn
Copy link
Member Author

gibfahn commented Nov 29, 2017

New target help should be phony but otherwise LGTM.

Done

@apapirovski
Copy link
Member

ping @gibfahn — does this need anything else to land?

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

@gibfahn
build: add a make help option for common targets
d865395 
PR-URL: nodejs#17323
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@gibfahn gibfahn merged commit d865395 into nodejs:master Dec 9, 2017
@gibfahn gibfahn deleted the make-help branch December 9, 2017 08:11
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
@gibfahn @MylesBorins
build: add a make help option for common targets
3b27948 
PR-URL: #17323
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
@gibfahn @MylesBorins
build: add a make help option for common targets
4bb27a2 
PR-URL: #17323
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Dec 12, 2017
Merged
@gibfahn gibfahn self-assigned this Dec 20, 2017
gibfahn added a commit that referenced this pull request Dec 20, 2017
@gibfahn
build: add a make help option for common targets
58a667c 
PR-URL: #17323
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@gibfahn gibfahn mentioned this pull request Dec 20, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@refack refack refack approved these changes

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@jasnell jasnell jasnell approved these changes

@evanlucas evanlucas evanlucas approved these changes

@addaleax addaleax addaleax approved these changes

@cjihrig cjihrig cjihrig approved these changes

@maclover7 maclover7 maclover7 approved these changes

@joyeecheung joyeecheung joyeecheung approved these changes

@richardlau richardlau richardlau approved these changes

Assignees

@gibfahn gibfahn

Labels
build Issues and PRs related to build files or the CI.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.