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.

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

Passing -j32 for ninja no longer works #53176

Closed
anonrig opened this issue May 27, 2024 · 24 comments · Fixed by #53181 or #53187
Closed

Passing -j32 for ninja no longer works #53176

anonrig opened this issue May 27, 2024 · 24 comments · Fixed by #53181 or #53187
Labels
build Issues and PRs related to build files or the CI. macos Issues and PRs related to the macOS platform / OSX.

Comments

@anonrig
Copy link
Member

anonrig commented May 27, 2024

Running ./configure --ninja -C and later running -make -j32 no longer works on main.

Possibly related to. 19f0bca

cc @tniessen

node on  main via ⬢ v22.2.0
❯ make -j32
ninja -C out/Release  -j
ninja: option requires an argument -- j
usage: ninja [options] [targets...]

if targets are unspecified, builds the 'default' target (see manual).

options:
  --version      print ninja version ("1.12.1")
  -v, --verbose  show all command lines while building
  --quiet        don't show progress status, just command output

  -C DIR   change to DIR before doing anything else
  -f FILE  specify input build file [default=build.ninja]

  -j N     run N jobs in parallel (0 means infinity) [default=12 on this system]
  -k N     keep going until N jobs fail (0 means infinity) [default=1]
  -l N     do not start new jobs if the load average is greater than N
  -n       dry run (don't run commands but act like they succeeded)

  -d MODE  enable debugging (use '-d list' to list modes)
  -t TOOL  run a subtool (use '-t list' to list subtools)
    terminates toplevel options; further flags are passed to the tool
  -w FLAG  adjust warnings (use '-w list' to list warnings)
make: *** [node] Error 1
@anonrig anonrig changed the title Passing ninja job no longer works Passing -j32 for ninja no longer works May 27, 2024
@tniessen
Copy link
Member

@anonrig What OS distribution is this, and what flavor of make? For my GNU make, 19f0bca appears to have fixed the handling of -j. Maybe this is due to some difference between different make implementations?

@tniessen
Copy link
Member

@anonrig @jakecastelli This Dockerfile demonstrates that passing -j from make to ninja works on Ubuntu on the main branch:

FROM ubuntu:latest

RUN apt-get update && apt-get install -y git build-essential ninja-build python3

WORKDIR /nodejs
RUN git clone --depth=1 https://github.com/nodejs/node.git
WORKDIR /nodejs/node
RUN ./configure --ninja
RUN make -j3
$ docker build --progress=plain .
...
#10 [7/7] RUN make -j3
#10 0.519 ninja -C out/Release  -j3
#10 0.521 ninja: Entering directory `out/Release'

@targos
Copy link
Member

targos commented May 28, 2024

I'm on macOS and it doesn't work:

$ make -v
GNU Make 3.81
Copyright (C) 2006  Free Software Foundation, Inc.
This is free software; see the source for copying conditions.
There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A
PARTICULAR PURPOSE.

This program built for i386-apple-darwin11.3.0

@tniessen tniessen added build Issues and PRs related to build files or the CI. macos Issues and PRs related to the macOS platform / OSX. labels May 28, 2024
@jakecastelli
Copy link
Member

I am on MacOS as well, exactly the same GNU Make as yours @targos - the current work around for me is to use make JOBS=n

@tniessen
Copy link
Member

On Linux, it seems that GNU Make 3.81 was obsolete in 2010 and last distributed in Ubuntu 14 — a decade ago... is that the most recent official release for macOS?

I found this StackOverflow thread that seems related, and which is also about macOS. It also mentions differences in how MAKEFLAGS is passed between make 3.x and 4.x.

@targos
Copy link
Member

targos commented May 28, 2024

The default make version available on macOS probably won't change in the near future, but it's possible to install the latest using homebrew: https://formulae.brew.sh/formula/make#default

@jakecastelli
Copy link
Member

I upgraded my make to v4.4.1 and I can verify it works on main (-jn can be properly propagated).

$ make -v
GNU Make 4.4.1
Built for aarch64-apple-darwin22.3.0
Copyright (C) 1988-2023 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <https://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.

I think your fix 19f0bca is legit! @tniessen we might need to update the README for mac users.

@jakecastelli
Copy link
Member

I think in conclusion for mac users there are two options:

  1. use make JOBS=n instead of make -jn if you are using the macOS default make.
  2. upgrade make to v4.x through homebrew and run make -jn.

Are you guys happy for me to update the BUILDING.md in a separate PR?

@tniessen
Copy link
Member

@jakecastelli Thank you so much for confirming! Let me try to add a workaround for ancient versions of GNU make to our Makefile.

@targos Since you have access to macOS and are still using GNU Make 3.81, could you please test if this patch fixes the propagation?

diff --git a/Makefile b/Makefile
index 5187d6a950..8f579bada1 100644
--- a/Makefile
+++ b/Makefile
@@ -147,7 +147,11 @@ ifdef JOBS
        NINJA_ARGS := $(NINJA_ARGS) -j$(JOBS)
 else
        IMMEDIATE_NINJA_ARGS := $(NINJA_ARGS)
-       NINJA_ARGS = $(IMMEDIATE_NINJA_ARGS) $(filter -j%,$(MAKEFLAGS))
+       ifneq (3.81,$(MAKE_VERSION))
+               NINJA_ARGS = $(IMMEDIATE_NINJA_ARGS) $(filter -j%,$(MAKEFLAGS))
+       else
+               NINJA_ARGS := $(IMMEDIATE_NINJA_ARGS) $(filter -j%,$(MAKEFLAGS))
+       endif
 endif
 $(NODE_EXE): config.gypi out/Release/build.ninja
        $(NINJA) -C out/Release $(NINJA_ARGS)

If it does, I can open a PR and add an explanation to the Makefile.

@targos
Copy link
Member

targos commented May 28, 2024

I notice one change of behavior with make v4. When configuring with --debug, it now runs the ninja commands for both Release and Debug builds in parallel (with v3, it was in sequence):

$ make -j6
ninja -C out/Release  -j6
ninja -C out/Debug  -j6
ninja: Entering directory `out/Release'
ninja: Entering directory `out/Debug'

@targos
Copy link
Member

targos commented May 28, 2024

@tniessen I wouldn't say it fixes the propagation, but at least it goes back to the previous "working" behavior that doesn't propagate the -j flag.

@jakecastelli
Copy link
Member

jakecastelli commented May 28, 2024

If I remember correctly with make v3 it builds the release and then the debug build separately right?

edit: sorry you have already mentioned here

with v3, it was in sequence

@targos
Copy link
Member

targos commented May 28, 2024

Yes. I would prefer it to only build the debug build but maybe there's a reason the release one is needed.

@tniessen
Copy link
Member

I wouldn't say it fixes the propagation, but at least it goes back to the previous "working" behavior that doesn't propagate the -j flag.

Oh, my assumption was that, since @anonrig was using -j32, propagation worked on GNU make 3.81 before 19f0bca.

If it never worked on any version of GNU make, then I am out of ideas for the macOS version.

@jakecastelli
Copy link
Member

TBH I think it might've never worked on macOS (at least with GNU make v3.81 on arm) with -jn, I use make JOBS=4 sometimes just to make sure my laptop does not freeze.

@jakecastelli
Copy link
Member

with GNU make v3.81 on previous "working version"

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

Screenshot 2024-05-28 at 7 13 13 pm

it does not look like -j4 works.

➜  node ✗ make JOBS=4
ninja -C out/Release  -j4
ninja: Entering directory `out/Release'

Screenshot 2024-05-28 at 7 13 50 pm

Correct me if I am wrong @targos

@targos
Copy link
Member

targos commented May 28, 2024

That's my experience too

@jakecastelli
Copy link
Member

jakecastelli commented May 28, 2024

with this patch from @tniessen :

-       NINJA_ARGS = $(IMMEDIATE_NINJA_ARGS) $(filter -j%,$(MAKEFLAGS))
+       ifneq (3.81,$(MAKE_VERSION))
+               NINJA_ARGS = $(IMMEDIATE_NINJA_ARGS) $(filter -j%,$(MAKEFLAGS))
+       else
+               NINJA_ARGS := $(IMMEDIATE_NINJA_ARGS) $(filter -j%,$(MAKEFLAGS))
+       endif

it falls back to the old behaviour which ignores the -jn propagation but build still works (feels like its been everyone's old behaviour).

Do you guys have any suggestion here? shall I apply the patch to #53181 (adding tniessen as co-author) and update the BUILDING.md regards this comment OR keep the current behaviour and only update the BUILDING.md.

@tniessen
Copy link
Member

it falls back to the old behaviour which ignores the -jn propagation but build still works (feels like its been everyone's old behaviour).

Do you guys have any suggestion here? shall I apply the patch to #53181

I don't think the old behavior is reasonable: if -jn was ignored anyway, affected contributors can stop passing -jn if they are using super old versions of GNU make.

Let's wait to hear from @anonrig since he opened the issue and apparently has been using -jn with ninja.

If we don't find a solution for ancient GNU make versions, then updating building-node-with-ninja.md is probably a good idea.

@joyeecheung
Copy link
Member

FWIW I have never used the -jn arguments when using ninja. I just BUILD_WITH=ninja make node and ninja will use all the available cores automatically.

@anonrig
Copy link
Member Author

anonrig commented May 28, 2024

Updating make fixes the issue, but there is an unnecessary space before -j32

❯ make -j32
ninja -C out/Release  -j32

@jakecastelli
Copy link
Member

jakecastelli commented May 28, 2024

I just BUILD_WITH=ninja make node and ninja will use all the available cores automatically.

Thanks for the suggestion, I think -jn usually only needed when you don't wanna ninja to use all your cores 😄.

Updating make fixes the issue, but there is an unnecessary space before -j32

surely I can fix that 😁

@tniessen
Copy link
Member

@jakecastelli Would you like to add a note to building-node-with-ninja.md about this issue with ancient make versions?

@jakecastelli
Copy link
Member

Would you like to add a note to building-node-with-ninja.md about this issue with ancient make versions?

Sure, I can do that 👍

nodejs-github-bot pushed a commit that referenced this issue May 30, 2024
PR-URL: #53187
Fixes: #53176
Refs: #53176
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
targos pushed a commit that referenced this issue Jun 1, 2024
PR-URL: #53187
Fixes: #53176
Refs: #53176
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
nodejs-github-bot pushed a commit that referenced this issue Jun 8, 2024
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>
targos pushed a commit that referenced this issue Jun 20, 2024
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>
sophoniie pushed a commit to sophoniie/node that referenced this issue Jun 20, 2024
PR-URL: nodejs#53187
Fixes: nodejs#53176
Refs: nodejs#53176
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
sophoniie pushed a commit to sophoniie/node that referenced this issue Jun 20, 2024
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>
bmeck pushed a commit to bmeck/node that referenced this issue Jun 22, 2024
PR-URL: nodejs#53187
Fixes: nodejs#53176
Refs: nodejs#53176
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
bmeck pushed a commit to bmeck/node that referenced this issue Jun 22, 2024
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>
marco-ippolito pushed a commit that referenced this issue Jul 19, 2024
PR-URL: #53187
Fixes: #53176
Refs: #53176
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
marco-ippolito pushed a commit that referenced this issue Jul 19, 2024
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>
marco-ippolito pushed a commit that referenced this issue Jul 19, 2024
PR-URL: #53187
Fixes: #53176
Refs: #53176
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
marco-ippolito pushed a commit that referenced this issue Jul 19, 2024
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. macos Issues and PRs related to the macOS platform / OSX.
Projects
None yet
5 participants