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

build: add crypto check to markdown lint target #21326

Closed
wants to merge 6 commits into from
Closed

build: add crypto check to markdown lint target #21326

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6a1a47c
build: add crypto check to markdown lint target
danbev Jun 14, 2018
043bcaa
squash: rename HAS_CRYPTO to NODE_USE_OPENSSL
danbev Jun 14, 2018
ea20291
squash: fix if statement
danbev Jun 14, 2018
d8aa677
squash: add default value for NODE_USE_OPENSSL
danbev Jun 14, 2018
030a1ee
squash: remove NODE_USE_OPENSSL variable
danbev Jun 15, 2018
5df7691
squash: check process.versions.openssl
danbev Jun 15, 2018
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
8 changes: 8 additions & 0 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1048,9 +1048,13 @@ LINT_MD_DOC_FILES = $(shell ls doc/**/*.md)
run-lint-doc-md = tools/remark-cli/cli.js -q -f $(LINT_MD_DOC_FILES)
# Lint all changed markdown files under doc/
tools/.docmdlintstamp: $(LINT_MD_DOC_FILES)
ifeq ($(NODE_USE_OPENSSL),true)
Copy link
Contributor

@refack refack Jun 14, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Since this step anyway requires an existing node binary, why not simply test @$(call available-node,"-p require('crypto')"). Then you can remove all the other "breadcrumbs".

Copy link
Contributor

Choose a reason for hiding this comment

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

Or -p process.config.variables.node_use_openssl

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, it would be safer to do that as the existing node binary might have been compiled without openssl (might not be the common case but could happen). I'll take a look and update. Thanks

@echo "Running Markdown linter on docs..."
@$(call available-node,$(run-lint-doc-md))
@touch $@
else
@echo "Skipping Markdown linter on docs (no crypto)"
endif

LINT_MD_TARGETS = src lib benchmark tools/doc tools/icu
LINT_MD_ROOT_DOCS := $(wildcard *.md)
Expand All @@ -1059,9 +1063,13 @@ LINT_MD_MISC_FILES := $(shell find $(LINT_MD_TARGETS) -type f \
run-lint-misc-md = tools/remark-cli/cli.js -q -f $(LINT_MD_MISC_FILES)
# Lint other changed markdown files maintained by us
tools/.miscmdlintstamp: $(LINT_MD_MISC_FILES)
ifeq ($(NODE_USE_OPENSSL),true)
@echo "Running Markdown linter on misc docs..."
@$(call available-node,$(run-lint-misc-md))
@touch $@
else
@echo "Skipping Markdown linter on misc docs (no crypto)"
endif

tools/.mdlintstamp: tools/.miscmdlintstamp tools/.docmdlintstamp

Expand Down
1 change: 1 addition & 0 deletions configure
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1517,6 +1517,7 @@ config = {
'BUILDTYPE': 'Debug' if options.debug else 'Release',
'PYTHON': sys.executable,
'NODE_TARGET_TYPE': variables['node_target_type'],
'NODE_USE_OPENSSL': variables['node_use_openssl'],
}

if options.prefix:
Expand Down