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: make test-doc and lint addon docs #16377

Closed
wants to merge 1 commit into from
Closed

build: make test-doc and lint addon docs #16377

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 42 additions & 14 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,8 @@ test: all
$(CI_ASYNC_HOOKS) \
$(CI_JS_SUITES) \
$(CI_NATIVE_SUITES) \
doctool known_issues
$(CI_DOC) \
known_issues
endif

# For a quick test, does not run linter or build doc
Expand Down Expand Up @@ -268,7 +269,6 @@ test/gc/build/Release/binding.node: test/gc/binding.cc test/gc/binding.gyp
--directory="$(shell pwd)/test/gc" \
--nodedir="$(shell pwd)"

# Implicitly depends on $(NODE_EXE), see the build-addons rule for rationale.
DOCBUILDSTAMP_PREREQS = tools/doc/addon-verify.js doc/api/addons.md

ifeq ($(OSTYPE),aix)
Expand All @@ -277,7 +277,7 @@ endif

test/addons/.docbuildstamp: $(DOCBUILDSTAMP_PREREQS)
$(RM) -r test/addons/??_*/
$(NODE) $<
[ -x $(NODE) ] && $(NODE) $< || node $<
Copy link
Contributor

Choose a reason for hiding this comment

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

Why just here? shouldn't this be handled in a global manner? Or just err with the message "to run with a precompiled node binary run make NODE=<path_to_node> <target>"

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 idea

touch $@

ADDONS_BINDING_GYPS := \
Expand Down Expand Up @@ -312,10 +312,10 @@ test/addons/.buildstamp: config.gypi \
done
touch $@

# .buildstamp and .docbuildstamp need $(NODE_EXE) but cannot depend on it
# .buildstamp needs $(NODE_EXE) but cannot depend on it
# directly because it calls make recursively. The parent make cannot know
# if the subprocess touched anything so it pessimistically assumes that
# .buildstamp and .docbuildstamp are out of date and need a rebuild.
# .buildstamp is out of date and need a rebuild.
# Just goes to show that recursive make really is harmful...
# TODO(bnoordhuis) Force rebuild after gyp update.
build-addons: $(NODE_EXE) test/addons/.buildstamp
Expand Down Expand Up @@ -347,10 +347,10 @@ test/addons-napi/.buildstamp: config.gypi \
done
touch $@

# .buildstamp and .docbuildstamp need $(NODE_EXE) but cannot depend on it
# .buildstamp needs $(NODE_EXE) but cannot depend on it
# directly because it calls make recursively. The parent make cannot know
# if the subprocess touched anything so it pessimistically assumes that
# .buildstamp and .docbuildstamp are out of date and need a rebuild.
# .buildstamp is out of date and need a rebuild.
# Just goes to show that recursive make really is harmful...
# TODO(bnoordhuis) Force rebuild after gyp or node-gyp update.
build-addons-napi: $(NODE_EXE) test/addons-napi/.buildstamp
Expand Down Expand Up @@ -382,6 +382,7 @@ test-all-valgrind: test-build
CI_NATIVE_SUITES ?= addons addons-napi
CI_ASYNC_HOOKS := async-hooks
CI_JS_SUITES ?= default
CI_DOC := doctool

# Build and test addons without building anything else
test-ci-native: LOGLEVEL := info
Expand All @@ -407,7 +408,8 @@ test-ci: | clear-stalled build-addons build-addons-napi doc-only
out/Release/cctest --gtest_output=tap:cctest.tap
$(PYTHON) tools/test.py $(PARALLEL_ARGS) -p tap --logfile test.tap \
--mode=release --flaky-tests=$(FLAKY_TESTS) \
$(TEST_CI_ARGS) $(CI_ASYNC_HOOKS) $(CI_JS_SUITES) $(CI_NATIVE_SUITES) doctool known_issues
$(TEST_CI_ARGS) $(CI_ASYNC_HOOKS) $(CI_JS_SUITES) $(CI_NATIVE_SUITES) \
$(CI_DOC) known_issues
# Clean up any leftover processes, error if found.
ps awwx | grep Release/node | grep -v grep | cat
@PS_OUT=`ps awwx | grep Release/node | grep -v grep | awk '{print $$1}'`; \
Expand Down Expand Up @@ -443,6 +445,10 @@ test-tick-processor: all
test-hash-seed: all
$(NODE) test/pummel/test-hash-seed.js

test-doc: doc-only
$(MAKE) lint
$(PYTHON) tools/test.py $(CI_DOC)

test-known-issues: all
$(PYTHON) tools/test.py known_issues

Expand Down Expand Up @@ -976,26 +982,38 @@ lint-md: lint-md-build
./*.md doc src lib benchmark tools/doc/ tools/icu/

LINT_JS_TARGETS = benchmark doc lib test tools
LINT_JS_CMD = tools/eslint/bin/eslint.js --cache \
--rulesdir=tools/eslint-rules --ext=.js,.mjs,.md \
$(LINT_JS_TARGETS)

lint-js:
@echo "Running JS linter..."
$(NODE) tools/eslint/bin/eslint.js --cache --rulesdir=tools/eslint-rules --ext=.js,.mjs,.md \
$(LINT_JS_TARGETS)
@if [ -x $(NODE) ]; then \
$(NODE) $(LINT_JS_CMD); \
else \
node $(LINT_JS_CMD); \
fi

jslint: lint-js
@echo "Please use lint-js instead of jslint"

lint-js-ci:
@echo "Running JS linter..."
$(NODE) tools/lint-js.js $(PARALLEL_ARGS) -f tap -o test-eslint.tap \
$(LINT_JS_TARGETS)
@if [ -x $(NODE) ]; then \
$(NODE) tools/lint-js.js $(PARALLEL_ARGS) -f tap -o test-eslint.tap \
$(LINT_JS_TARGETS); \
else \
node tools/lint-js.js $(PARALLEL_ARGS) -f tap -o test-eslint.tap \
$(LINT_JS_TARGETS); \
fi

jslint-ci: lint-js-ci
@echo "Please use lint-js-ci instead of jslint-ci"

LINT_CPP_ADDON_DOC_FILES = $(wildcard test/addons/??_*/*.cc test/addons/??_*/*.h)
LINT_CPP_EXCLUDE ?=
LINT_CPP_EXCLUDE += src/node_root_certs.h
LINT_CPP_EXCLUDE += $(wildcard test/addons/??_*/*.cc test/addons/??_*/*.h)
LINT_CPP_EXCLUDE += $(LINT_CPP_ADDON_DOC_FILES)
LINT_CPP_EXCLUDE += $(wildcard test/addons-napi/??_*/*.cc test/addons-napi/??_*/*.h)
# These files were copied more or less verbatim from V8.
LINT_CPP_EXCLUDE += src/tracing/trace_event.h src/tracing/trace_event_common.h
Expand All @@ -1019,11 +1037,19 @@ LINT_CPP_FILES = $(filter-out $(LINT_CPP_EXCLUDE), $(wildcard \
tools/icu/*.h \
))

# Code blocks don't have newline at the end,
# and the actual filename is generated so it won't match header guards
ADDON_DOC_LINT_FLAGS=-whitespace/ending_newline,-build/header_guard

lint-cpp:
@echo "Running C++ linter..."
@$(PYTHON) tools/cpplint.py $(LINT_CPP_FILES)
@$(PYTHON) tools/check-imports.py

lint-addon-docs: test/addons/.docbuildstamp
@echo "Running C++ linter on addon docs..."
@$(PYTHON) tools/cpplint.py --filter=$(ADDON_DOC_LINT_FLAGS) $(LINT_CPP_ADDON_DOC_FILES)

cpplint: lint-cpp
@echo "Please use lint-cpp instead of cpplint"

Expand All @@ -1033,9 +1059,10 @@ lint:
$(MAKE) lint-js || EXIT_STATUS=$$? ; \
$(MAKE) lint-cpp || EXIT_STATUS=$$? ; \
$(MAKE) lint-md || EXIT_STATUS=$$? ; \
$(MAKE) lint-addon-docs || EXIT_STATUS=$$? ; \
exit $$EXIT_STATUS
CONFLICT_RE=^>>>>>>> [0-9A-Fa-f]+|^<<<<<<< [A-Za-z]+
lint-ci: lint-js-ci lint-cpp lint-md
lint-ci: lint-js-ci lint-cpp lint-md lint-addon-docs
@if ! ( grep -IEqrs "$(CONFLICT_RE)" benchmark deps doc lib src test tools ) \
&& ! ( find . -maxdepth 1 -type f | xargs grep -IEqs "$(CONFLICT_RE)" ); then \
exit 0 ; \
Expand Down Expand Up @@ -1115,6 +1142,7 @@ endif
test-ci \
test-ci-js \
test-ci-native \
test-doc \
test-gc \
test-gc-clean \
test-hash-seed \
Expand Down
2 changes: 1 addition & 1 deletion doc/api/addons.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1095,7 +1095,7 @@ static void at_exit_cb1(void* arg) {
Isolate* isolate = static_cast<Isolate*>(arg);
HandleScope scope(isolate);
Local<Object> obj = Object::New(isolate);
assert(!obj.IsEmpty()); // assert VM is still alive
assert(!obj.IsEmpty()); // assert VM is still alive
Copy link
Contributor

Choose a reason for hiding this comment

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

is this a lint issue?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, needs two spaces here

assert(obj->IsObject());
at_exit_cb1_called++;
}
Expand Down