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

jakecastelli · 2024-05-28T01:31:35Z

github-actions · 2024-05-28T01:40:09Z

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

nodejs-github-bot · 2024-05-28T01:46:53Z

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

tniessen

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.

jakecastelli · 2024-05-28T08:31:36Z

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.

tniessen · 2024-05-28T08:53:14Z

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

jakecastelli · 2024-05-28T09:01:11Z

I am on MacOS m1

targos

This is not the right fix as discussed in #53176

Waiting for discussion to settle

jakecastelli · 2024-05-28T14:16:58Z

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

tniessen

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

jakecastelli · 2024-05-28T15:17:53Z

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'

tniessen · 2024-05-28T15:41:48Z

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

anonrig · 2024-05-28T15:46:36Z

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 :-)

jakecastelli · 2024-05-28T15:53:32Z

OHHH I took this one seriously 😂

jakecastelli · 2024-05-28T16:05:28Z

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

nodejs-github-bot · 2024-06-08T00:39:42Z

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

nodejs-github-bot · 2024-06-08T01:08:23Z

Landed in 5a446cc

PR-URL: #53181Fixes: #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#53181Fixes: 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: #53181Fixes: #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 pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels May 28, 2024

github-actionsBot 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 requested changes May 28, 2024
View reviewed changes

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

jakecastelli changed the title ~~build: passing -j number into ninja~~build: fix the extra whitespace before NINJIA_ARGSMay 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 a6ba4cbCompare 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:mainJun 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

Uh oh!

Uh oh!

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-actionsBot 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

jakecastelli commented May 28, 2024•
edited
Loading

jakecastelli commented May 28, 2024•
edited
Loading