Skip to content

build: opt-in to delegate building from Makefile to ninja - #27504

Merged
refack merged 1 commit into
nodejs:masterfrom
refack:auto-support-ninja
May 3, 2019
Merged

build: opt-in to delegate building from Makefile to ninja#27504
refack merged 1 commit into
nodejs:masterfrom
refack:auto-support-ninja

Conversation

@refack

@refackrefack commented Apr 30, 2019•
edited
Loading

Copy link
Copy Markdown
Contributor

This captures the intent to build with ninja from configure --ninja to be used in the Makefile.

/CC @nodejs/build-files
/CC @joyeecheung@ryzokuken@sam-github (who expressed interest in the past)

Refs: https://mobile.twitter.com/refack/status/1118484079077482498

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

@nodejs-github-bot

nodejs-github-bot commented Apr 30, 2019

Copy link
Copy Markdown
Collaborator

Lite-CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/3405

@nodejs-github-botnodejs-github-bot added the build Issues and PRs related to build files or the CI. label Apr 30, 2019
@refackrefack self-assigned this Apr 30, 2019
@sam-github

sam-github commented Apr 30, 2019

Copy link
Copy Markdown
Contributor

Will it cause make test to use the ninja-built node? If so, this might be good for me, but I'm not sure if the impact on everyone else is excessive. fwiw, I never build with Make anymore. I run test.py directly, and make lint directly. Besides the issues in #25135, ninja is faster and gives a better progess indicator (in fairness, make doesn't even try to give progress indication).

@refack

refack commented Apr 30, 2019

Copy link
Copy Markdown
ContributorAuthor

Will it cause make test to use the ninja-built node?

Yes. make test delegates to make node or make node_g which are the targets I've patched.
BTW: this will use ninja iff configure was run with --ninja

Trott
Trott reviewed Apr 30, 2019
View reviewed changes
Comment threadconfigure.py Outdated

@TrottTrottApr 30, 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.

Stray whitespace change?

@refackrefackApr 30, 2019

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

No. I meant to better delineate the lines that generate config.mk.
Optimally I'd wrap this code into a function, but that will just conflict with #26725

@sam-github

sam-github commented Apr 30, 2019

Copy link
Copy Markdown
Contributor

BTW: this will use ninja iff configure was run with --ninja

That's a really important point. I'll give it a dry-run tomorrow (no time today)

richardlau
richardlau reviewed Apr 30, 2019
View reviewed changes
Comment threadMakefile Outdated
@joyeecheung

joyeecheung commented May 1, 2019•
edited
Loading

Copy link
Copy Markdown
Member

+100

The default is make, it seems. Can we get CI coverage for this? I assume this just needs a custom freestyle build with --use_ninja as CONFIG_FLAGS? (But does any of the machine have ninja? Or maybe the one that builds the V8 CI should have it?)

@refack

refack commented May 1, 2019

Copy link
Copy Markdown
ContributorAuthor

Can we get CI coverage for this?

I'll think of a way to cover this without adding bloat to the CI matrix 🤔 ...

@refackrefack changed the title build: delegate building from Makefile to ninjabuild: opt-in to delegate building from Makefile to ninjaMay 1, 2019
sam-github
sam-github reviewed May 1, 2019
View reviewed changes
Comment threadMakefile Outdated

@sam-githubsam-githubMay 1, 2019

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.

Perhaps the -jN flag, if it's present in MAKEFLAGS, should be copied into NINJA_ARGS? So that make -j2 ends up calling ninja -j2.

@refackrefackMay 2, 2019•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Perhaps the -jN flag, if it's present in MAKEFLAGS, should be copied into NINJA_ARGS? So that make -j2 ends up calling ninja -j2.

Added support via a JOBS env var.

FTR: By default ninja runs in multiproc mode so adding -j is used to change the default or limit the number of parallel processes.
Also make or even gmake do not readily expose the value that was passed to them.

@refackrefackMay 2, 2019

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Added $(filter -j%,$(MAKEFLAGS))

sam-github
sam-github approved these changes May 1, 2019
View reviewed changes

@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.

Worked for me when I tried it, seems pretty reasonable.

ryzokuken
ryzokuken approved these changes May 1, 2019
View reviewed changes

@ryzokukenryzokuken 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.

Can't thank you enough for this.

LGTM w/ @sam-github's concern taken care of.

@sam-github

sam-github commented May 1, 2019

Copy link
Copy Markdown
Contributor

IMO, If it doesn't work, those of using ninja will notice pretty quickly, I don't know that it needs ci coverage.

@refack
refackforce-pushed the auto-support-ninja branch from 76b89aa to 821f143CompareMay 2, 2019 20:15
@nodejs-github-bot

nodejs-github-bot commented May 2, 2019

Copy link
Copy Markdown
Collaborator

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

@refack
refackforce-pushed the auto-support-ninja branch from 858adb2 to f45e940CompareMay 2, 2019 20:26
@nodejs-github-bot

nodejs-github-bot commented May 2, 2019

Copy link
Copy Markdown
Collaborator

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

@nodejs-github-bot

nodejs-github-bot commented May 3, 2019

Copy link
Copy Markdown
Collaborator

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

@nodejs-github-bot

nodejs-github-bot commented May 3, 2019

Copy link
Copy Markdown
Collaborator

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

@refack
build: delegate building from Makefile to ninja
46eb532
PR-URL: nodejs#27504
Refs: https://mobile.twitter.com/refack/status/1118484079077482498
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
@refack
refackforce-pushed the auto-support-ninja branch from f45e940 to 46eb532CompareMay 3, 2019 01:05
@refack
refack merged commit 46eb532 into nodejs:masterMay 3, 2019
@refack
refack deleted the auto-support-ninja branch May 3, 2019 01:06
targos pushed a commit that referenced this pull request May 4, 2019
@refack@targos
build: delegate building from Makefile to ninja
22a5a05
PR-URL: #27504
Refs: https://mobile.twitter.com/refack/status/1118484079077482498
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
@targostargos mentioned this pull request May 6, 2019
Merged
This was referenced May 7, 2019
Closed
Closed
Closed
This was referenced May 7, 2019
Closed
Closed
Closed
Closed
Closed
Closed
Closed
@refackrefack mentioned this pull request May 27, 2019
Closed
@refackrefack removed their assignment Jun 6, 2019
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@TrottTrottTrott left review comments
@richardlaurichardlaurichardlau left review comments
+2 more reviewers
@sam-githubsam-githubsam-github approved these changes
@ryzokukenryzokukenryzokuken approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

buildIssues 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.

7 participants

@refack@nodejs-github-bot@sam-github@joyeecheung@Trott@richardlau@ryzokuken