build: fix the extra whitespace before NINJIA_ARGS by jakecastelli · Pull Request #53181 · nodejs/node · GitHub

Fast-track has been requested by @anonrig. Please 👍 to approve.

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

Is this effectively reverting 19f0bca? If so, then it'd be cleaner to actually git revert that commit.

That'd break building with ninja for me again, though, so we should really figure out a solution that works for everyone.

Hi @tniessen, do you want me to revert this commit in this PR?

I have been spending the past few hours trying to get the ninja build working as it is breaking for me as well (I am not an expert on either Makefiles or ninja and just trying to dig and learn ;)) I think I am getting close to get a fix out. Will ping you once I open that PR.

@jakecastelli Let's continue the discussion in #53176. Are you on macOS by any chance?

I am on MacOS m1

This is not the right fix as discussed in #53176

Waiting for discussion to settle

Turned out this PR I only fixed an unnecessary whitespace, but I've learned a lot ❤️ thanks everyone!

The space is intentional — I'm pretty sure this change will break make -jn V=1.

The space is intentional — I'm pretty sure this change will break make -jn V=1.

oh good catch! @tniessen what if we change the position:

 150⋮    │    NINJA_ARGS = $(IMMEDIATE_NINJA_ARGS)$(filter -j%,$(MAKEFLAGS))
    ⋮ 150│    NINJA_ARGS = $(filter -j%,$(MAKEFLAGS))$(IMMEDIATE_NINJA_ARGS)

➜  node ✗ make -j4 V=1
ninja -C out/Release -j4 -v 
ninja: Entering directory `out/Release'

Eh, I don't know — why are we trying to get rid of a single space in the output of make?

Eh, I don't know — why are we trying to get rid of a single space in the output of make?

Nit: My right eye twitches :-)

OHHH I took this one seriously 😂

why are we trying to get rid of a single space in the output of make?

For the couple of hours I spent on learning how Makefile works, I think I deserve a green tick 😉 just joking, I am happy to close this PR if you are object to it.

Not sure what this is accomplishing but I suppose it's not harmful either

Commit Queue failed

- Loading data for nodejs/node/pull/53181
✔  Done loading data for nodejs/node/pull/53181
----------------------------------- PR info ------------------------------------
Title      build: fix the extra whitespace before NINJIA_ARGS (#53181)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     jakecastelli:fix-passing-j32-to-ninja -> nodejs:main
Labels     build, author ready, needs-ci
Commits    1
 - build: fix spacing before NINJA_ARGS
Committers 1
 - jakecastelli <959672929@qq.com>
PR-URL: https://github.com/nodejs/node/pull/53181
Fixes: https://github.com/nodejs/node/issues/53176
Refs: https://github.com/nodejs/node/issues/53176
Reviewed-By: Yagiz Nizipli 
Reviewed-By: Michaël Zasso 
Reviewed-By: Luigi Pinca 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/53181
Fixes: https://github.com/nodejs/node/issues/53176
Refs: https://github.com/nodejs/node/issues/53176
Reviewed-By: Yagiz Nizipli 
Reviewed-By: Michaël Zasso 
Reviewed-By: Luigi Pinca 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last approving review:
   ⚠  - build: fix spacing before NINJA_ARGS
   ℹ  This PR was created on Tue, 28 May 2024 01:31:35 GMT
   ✔  Approvals: 3
   ✔  - Yagiz Nizipli (@anonrig): https://github.com/nodejs/node/pull/53181#pullrequestreview-2081601232
   ✔  - Michaël Zasso (@targos) (TSC): https://github.com/nodejs/node/pull/53181#pullrequestreview-2082953839
   ✔  - Luigi Pinca (@lpinca): https://github.com/nodejs/node/pull/53181#pullrequestreview-2083105432
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2024-05-28T01:46:53Z: https://ci.nodejs.org/job/node-test-pull-request/59471/
   ⚠  Commits were pushed after the last Full PR CI run:
   ⚠  - build: fix spacing before NINJA_ARGS
- Querying data for job/node-test-pull-request/59471/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/9424757149

Landed in 5a446cc

PR-URL: #53181 Fixes: #53176 Refs: #53176 Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>

PR-URL: nodejs#53181 Fixes: nodejs#53176 Refs: nodejs#53176 Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>

PR-URL: #53181 Fixes: #53176 Refs: #53176 Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>

nodejs-github-bot added build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run. labels May 28, 2024

anonrig approved these changes May 28, 2024

View reviewed changes

anonrig added fast-track PRs that do not need to wait for 48 hours to land. author ready PRs that have at least one approval, no outstanding review comments, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels May 28, 2024

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 28, 2024

jakecastelli marked this pull request as ready for review May 28, 2024 03:32

tniessen reviewed May 28, 2024

View reviewed changes

tniessen removed the fast-track PRs that do not need to wait for 48 hours to land. label May 28, 2024

jakecastelli mentioned this pull request May 28, 2024

Passing -j32 for ninja no longer works #53176

Closed

lpinca previously approved these changes May 28, 2024

View reviewed changes

targos suggested changes May 28, 2024

View reviewed changes

jakecastelli force-pushed the fix-passing-j32-to-ninja branch from ed72a00 to b8f6690 Compare May 28, 2024 14:14

jakecastelli changed the title build: passing -j number into ninja build: fix the extra whitespace before NINJIA_ARGS May 28, 2024

targos approved these changes May 28, 2024

View reviewed changes

lpinca approved these changes May 28, 2024

View reviewed changes

tniessen previously requested changes May 28, 2024

View reviewed changes

build: fix spacing before NINJA_ARGS

a6ba4cb

jakecastelli force-pushed the fix-passing-j32-to-ninja branch from b8f6690 to a6ba4cb Compare May 28, 2024 15:54

H4ad added the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 8, 2024

nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Jun 8, 2024

H4ad approved these changes Jun 8, 2024

View reviewed changes

H4ad added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Jun 8, 2024

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 8, 2024

nodejs-github-bot merged commit 5a446cc into nodejs:main Jun 8, 2024

targos mentioned this pull request Jun 25, 2024

v22.4.0 release proposal #53583

Merged

Karliz24 mentioned this pull request Jul 9, 2024

## 2024-07-02, Version 22.4.0 (Current), @targos Karliz24/Marley-#16

Open

marco-ippolito mentioned this pull request Jul 19, 2024

v20.16.0 proposal #53945

Merged

build: fix the extra whitespace before NINJIA_ARGS by jakecastelli · Pull Request #53181 · nodejs/node · GitHub

Uh oh!

Uh oh!

build: fix the extra whitespace before NINJIA_ARGS - #53181

build: fix the extra whitespace before NINJIA_ARGS#53181nodejs-github-bot merged 1 commit intonodejs:mainnodejs/node:mainfrom jakecastelli:fix-passing-j32-to-ninjajakecastelli/node:fix-passing-j32-to-ninjaCopy head branch name to clipboard

Conversation

jakecastelli commented May 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented May 28, 2024

Uh oh!

nodejs-github-bot commented May 28, 2024

Uh oh!

tniessen left a comment

Choose a reason for hiding this comment

Uh oh!

jakecastelli commented May 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tniessen commented May 28, 2024

Uh oh!

jakecastelli commented May 28, 2024

Uh oh!

targos left a comment

Choose a reason for hiding this comment

Uh oh!

jakecastelli commented May 28, 2024

Uh oh!

tniessen left a comment

Choose a reason for hiding this comment

Uh oh!

jakecastelli commented May 28, 2024

Uh oh!

tniessen commented May 28, 2024

Uh oh!

anonrig commented May 28, 2024

Uh oh!

jakecastelli commented May 28, 2024

Uh oh!

jakecastelli commented May 28, 2024

Uh oh!

nodejs-github-bot commented Jun 8, 2024

Uh oh!

nodejs-github-bot commented Jun 8, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

build: fix the extra whitespace before NINJIA_ARGS#53181
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
jakecastelli:fix-passing-j32-to-ninja

jakecastelli commented May 28, 2024 •
edited
Loading

jakecastelli commented May 28, 2024 •
edited
Loading