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: introduce make help #17056

Merged
merged 1 commit into from Jul 17, 2017
Merged

Conversation

jordanlewis
Copy link
Member

@jordanlewis jordanlewis commented Jul 17, 2017

Add a help target to the Makefile that prints available Makefile
targets along with a brief help text for each.

Prints all targets that have an end-of-line comment starting with ##,
and all FLAG= variables with the same kind of end-of-line comment.

Example output:

Usage:
  make [target] [FLAG=foo FLAG2=bar...]

Available commands:
  acceptance                     Run acceptance tests.
  archive                        Build a source tarball from this repository.
  bench                          Run benchmarks.
  clean                          Clean all build artifacts.
  generate                       Regenerate generated code.
  help                           Print help for targets with comments.
  install                        Install CockroachDB binary.
  lint                           Run all style checkers and linters.
  lintshort                      Run a fast subset of the style checkers and linters.
  pre-push                       Run generate, lint, and test.
  protobuf                       Regenerate generated code for protobuf definitions.
  stress                         Run tests under stress.
  stressrace                     Run tests under stress with the race detector enabled.
  testlogic                      Run SQL Logic Tests.
  testrace                       Run tests with the Go race detector enabled.

Available flags:
  BENCHES                        Regexp to pass to the -run argument of the go benchmark runner.
  FILES                          Space delimited list of logic test files to run, for make testlogic.
  PKG                            Which package to run tests against, e.g. "./pkg/storage".
  TESTFLAGS                      Extra flags to pass to the go test runner, e.g. "-v --vmodule=raft=1"
  TESTS                          Regexp to pass to the -run argument of the go test runner. See go help testflag.

Approach cribbed from https://marmelab.com/blog/2016/02/29/auto-documented-makefile.html

@jordanlewis jordanlewis requested review from knz and benesch July 17, 2017 19:43
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@knz
Copy link
Contributor

knz commented Jul 17, 2017

The idea is fine - the implementation is... overboard :) I don't like the idea of sending colors on terminals that may not support these codes. either "%-30s %s\n" or make the color coding conditional on known values of $TERM.

Copy link
Contributor

@benesch benesch left a comment

Choose a reason for hiding this comment

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

This is awesome. Thanks, @jordanlewis. You might want to mention make help in CONTRIBUTING.md while you're here.

Makefile Outdated
@@ -180,7 +183,7 @@ gotestdashi: $(C_LIBS) $(CGO_FLAGS_FILES) $(BOOTSTRAP_TARGET)

testshort: override TESTFLAGS += -short

testrace: override GOFLAGS += -race
testrace: override GOFLAGS += -race ## Run tests with the Go race detector enabled.
Copy link
Contributor

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 if this were on its own line, like

testrace: ## Run tests with the Go race detector enabled

to minimize diff noise.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, done for all.

@benesch
Copy link
Contributor

benesch commented Jul 17, 2017

To address @knz's feedback, you could do something like the following:

TERM_CYAN = $(shell tput setaf 1 2> /dev/null)
TERM_RESET = $(shell tput sgr0 2> /dev/null)
awk "$(TERM_CYAN)%-30s$(TERM_RESET)" ...

@benesch
Copy link
Contributor

benesch commented Jul 17, 2017

Or, better yet, do it in the recipe to avoid the two extra shell invocations on every Make:

help:
    cyan=$$(tput setaf 1 2> /dev/null) && reset=$$(tput sgr0 2> /dev/null) && awk "$cyan%-30s$reset"

@knz
Copy link
Contributor

knz commented Jul 17, 2017

👍 on nikhil's suggestion, but please check the output is not borked if tput doesn't exist. Not sure whether the /dev/null redirect is sufficient in that case.

@benesch
Copy link
Contributor

benesch commented Jul 17, 2017

Woohoo! Love it.

@benesch
Copy link
Contributor

benesch commented Jul 17, 2017

We should document build and buildoss too, but feel free to leave for a follow up/me.

Makefile Outdated
@echo "Available commands:"
@grep -Eh '^[a-zA-Z._-]+:.*?## .*$$' $(MAKEFILE_LIST) | sort | awk 'BEGIN {FS = ":.*?## "}; {printf " %-30s %s\n", $$1, $$2}'
@echo ""
@echo "Available flags:"
Copy link
Contributor

Choose a reason for hiding this comment

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

Annnnnnd I just caused a conflict. Sorry! Since you'll have to rebase anyway, two optional nits:

  1. These might be better described as "Useful {commands|flags}", since they're not exhaustive.
  2. I'd write make [target...] [VAR=foo VAR2=bar] and "useful variables" since they're not technically flags, but perhaps I'm too deep in Make-land to know what would be more useful to users.

Add a `help` target to the Makefile that prints available Makefile
targets along with a brief help text for each.

Prints all targets that have an end-of-line comment starting with ##,
and all FLAG= variables with the same kind of end-of-line comment.

Example output:

```
Usage:
  make [target] [FLAG=foo FLAG2=bar...]

Available commands:
  acceptance                     Run acceptance tests.
  archive                        Build a source tarball from this repository.
  bench                          Run benchmarks.
  clean                          Clean all build artifacts.
  generate                       Regenerate generated code.
  help                           Print help for targets with comments.
  install                        Install CockroachDB binary.
  lint                           Run all style checkers and linters.
  lintshort                      Run a fast subset of the style checkers and linters.
  pre-push                       Run generate, lint, and test.
  protobuf                       Regenerate generated code for protobuf definitions.
  stress                         Run tests under stress.
  stressrace                     Run tests under stress with the race detector enabled.
  testlogic                      Run SQL Logic Tests.
  testrace                       Run tests with the Go race detector enabled.

Available flags:
  BENCHES                        Regexp to pass to the -run argument of the go benchmark runner.
  FILES                          Space delimited list of logic test files to run, for make testlogic.
  PKG                            Which package to run tests against, e.g. "./pkg/storage".
  TESTFLAGS                      Extra flags to pass to the go test runner, e.g. "-v --vmodule=raft=1"
  TESTS                          Regexp to pass to the -run argument of the go test runner. See go help testflag.
```

Approach cribbed from https://marmelab.com/blog/2016/02/29/auto-documented-makefile.html
@jordanlewis
Copy link
Member Author

Rebased, added back colors with @benesch's technique, added help for build/buildoss.

@benesch
Copy link
Contributor

benesch commented Jul 17, 2017

❤️

@jordanlewis jordanlewis merged commit c477365 into cockroachdb:master Jul 17, 2017
@jordanlewis jordanlewis deleted the make-help branch July 17, 2017 21:02
@irfansharif
Copy link
Contributor

nice!

@tbg tbg mentioned this pull request Jul 17, 2017
tbg added a commit to tbg/cockroach that referenced this pull request Jul 17, 2017
No good deed goes unpunished: cockroachdb#17056 accidentally messed up the variables'
default values. For example, TEST=". ".
On the plus side, this makes our CI green much more often. But, I decided
to change it back.
@tbg tbg mentioned this pull request Jul 17, 2017
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.

None yet

5 participants