Skip to content

build: more portable Makefile - #28108

Closed
refack wants to merge 4 commits into
nodejs:masterfrom
refack:more-portable-Makefile
Closed

build: more portable Makefile#28108
refack wants to merge 4 commits into
nodejs:masterfrom
refack:more-portable-Makefile

Conversation

@refack

Copy link
Copy Markdown
Contributor
  • Make some $(shell) calls lazy
  • $(wildcard) instead of ls
  • $(info) instead of echo
  • $? instead of duplicated file lists

Biggest benefit: smoother experience with MSYS make (can be added to "Git Bash", or vendored)

/CC @nodejs/build-files @nodejs/platform-windows

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

* Make some `$(shell)` calls lazy
* `$(wildcard)` instead of `ls`
* `$(info)` instead of `echo`
* `$?` instead of duplicated file lists
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-botnodejs-github-bot added the build Issues and PRs related to build files or the CI. label Jun 6, 2019

@sam-githubsam-github left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good to me

@sam-github

Copy link
Copy Markdown
Contributor

Assuming it passes ci ... :-)

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Comment threadMakefile
@$(call available-node,$(run-lint-doc-md))
@touch $@
tools/.docmdlintstamp: AVALIBLE_NODE := $(available-node-shell)
tools/.docmdlintstamp: $(wildcard doc/*.md doc/**/*.md)

@richardlaurichardlauJun 13, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

doc/**/*.md doesn't appear to pick up all subdirectories under doc with wildcard.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

5 mins of searching, and I couldn't even find a definition of what pattern means for wildcard() in GNU make, much less generally. I didn't think ** was supported unless the shell supported it, and I just tried on AIX, which is agressively POSIX, and it didn't expand.

@BridgeAR

Copy link
Copy Markdown
Member

@refack this needs a rebase

@BridgeAR

Copy link
Copy Markdown
Member

Closing, since there was no follow up for a long time.
@refack please reopen in case you would like to work on this again!

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

buildIssues and PRs related to build files or the CI.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@refack@nodejs-github-bot@sam-github@BridgeAR@richardlau