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

bash: disable parallel building on darwin #245066

Closed

Conversation

risicle
Copy link
Contributor

@risicle risicle commented Jul 23, 2023

Description of changes

On darwin with many CPUs building bash will sometimes fail with:

make[2]: warning: -j8 forced in submake: resetting jobserver mode.
cp ./libgnuintl.h.in libgnuintl.h
In file included from unwind_prot.c:51:
In file included from ./bashintl.h:30:
./include/gettext.h:27:11: fatal error: 'libintl.h' file not found
# include <libintl.h>
          ^~~~~~~~~~~
1 error generated.
clang  -DPROGRAM='"bash"' -DCONF_HOSTTYPE='"x86_64"' -DCONF_OSTYPE='"darwin19.6.0"' -DCONF_MACHTYPE='"x86_64-apple-darwin19.6.0"' -DCONF_VENDOR='"apple"' -DLOCALEDIR='"/nix/store/mfgsqk1kbd3faz18plkmjj5ng81spww6-bash-5.2-p15/share/locale"' -DPACKAGE='"bash"' -DSHELL -DHAVE_CONFIG_H -DMACOSX   -I.  -I. -I./include -I./lib -I./lib/intl -I/private/tmp/nix-build-bash-5.2-p15.drv-0/bash-5.2/lib/intl -Wno-parentheses -Wno-format-security   -g -O2 -c dispose_cmd.c
make: *** [Makefile:106: unwind_prot.o] Error 1
make: *** Waiting for unfinished jobs....
cmp libgnuintl.h libintl.h || cp libgnuintl.h libintl.h
cmp: libintl.h: No such file or directory
make[2]: Leaving directory '/private/tmp/nix-build-bash-5.2-p15.drv-0/bash-5.2/lib/intl

where it can be clearly seen that libintl.h is being put in place too late. Disabling parallelism resolves the issue.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.11 Release Notes (or backporting 23.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Copy link
Contributor

@tjni tjni left a comment

Choose a reason for hiding this comment

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

LGTM. I've also seen this before locally.

@SuperSandro2000
Copy link
Member

@trofi can you help out here?

@trofi
Copy link
Contributor

trofi commented Jul 24, 2023

The change looks good. I was not able to reproduce parallel build failure locally on x86_64-darwin VM. Not sure if I'm holding it wrong:

$ while :; do nix build --impure --expr 'with import ./. {}; bash.overrideAttrs (oa: { makeFlags = "--shuffle"; nativeBuildInputs = oa.nativeBuildInputs ++ [ gnumake ]; })' --cores 8 --rebuild || break; echo again; done
again
again
again
again
again
again
again
again
again
again
again
again
again
again
again
...

Looking at your error snippet I think it's a matter of missing dependency on (transitive) libintl.h by unwind_prot.c:

I wonder if something like this is enough for you to fix parallel failures:

--- a/Makefile.in
+++ b/Makefile.in
@@ -1432,6 +1432,7 @@ siglist.o: bashintl.h ${LIBINTL_H} $(BASHINCDIR)/gettext.h
 subst.o: bashintl.h ${LIBINTL_H} $(BASHINCDIR)/gettext.h
 test.o: bashintl.h ${LIBINTL_H} $(BASHINCDIR)/gettext.h
 trap.o: bashintl.h ${LIBINTL_H} $(BASHINCDIR)/gettext.h
+unwind_prot.o: bashintl.h ${LIBINTL_H} $(BASHINCDIR)/gettext.h
 variables.o: bashintl.h ${LIBINTL_H} $(BASHINCDIR)/gettext.h
 version.o: bashintl.h ${LIBINTL_H} $(BASHINCDIR)/gettext.h
 xmalloc.o: bashintl.h ${LIBINTL_H} $(BASHINCDIR)/gettext.h

Reproduced manually on a bash git checkout as:

$ nix develop nixpkgs#bash
$$ ./configure
$$ make unwind_prot.o
...
In file included from unwind_prot.c:51:
In file included from ./bashintl.h:30:
./include/gettext.h:27:11: fatal error: 'libintl.h' file not found
# include <libintl.h>
          ^~~~~~~~~~~
1 error generated.
make: *** [Makefile:106: unwind_prot.o] Error 1

Looks like a legit upstream bug.

@trofi
Copy link
Contributor

trofi commented Jul 24, 2023

Proposed the fix upstream as https://savannah.gnu.org/patch/index.php?10373

trofi added a commit to trofi/nixpkgs that referenced this pull request Jul 24, 2023
As reported by Robert Scott in NixOS#245066
without the change `make -j8` build of `make` occasionally fails to
buildin parallel. The simplest reproducer is:

    $$ ./configure
    $$ make unwind_prot.o
    ...
    In file included from unwind_prot.c:51:
    In file included from ./bashintl.h:30:
    ./include/gettext.h:27:11: fatal error: 'libintl.h' file not found
    # include <libintl.h>
              ^~~~~~~~~~~
    1 error generated.
    make: * [Makefile:106: unwind_prot.o] Error 1

The change adds missing ttransitive `${LIBINTL_H}` dependency for
unwind_prot.o.
@trofi
Copy link
Contributor

trofi commented Jul 24, 2023

Also proposed it for nixpkgs as #245252 if you think it's a reasonable fix.

@vcunat
Copy link
Member

vcunat commented Aug 11, 2023

This PR should be superseded by #245252

@vcunat vcunat closed this Aug 11, 2023
tm-drtina pushed a commit to awakesecurity/nixpkgs that referenced this pull request Jul 4, 2024
As reported by Robert Scott in NixOS#245066
without the change `make -j8` build of `make` occasionally fails to
buildin parallel. The simplest reproducer is:

    $$ ./configure
    $$ make unwind_prot.o
    ...
    In file included from unwind_prot.c:51:
    In file included from ./bashintl.h:30:
    ./include/gettext.h:27:11: fatal error: 'libintl.h' file not found
    # include <libintl.h>
              ^~~~~~~~~~~
    1 error generated.
    make: * [Makefile:106: unwind_prot.o] Error 1

The change adds missing ttransitive `${LIBINTL_H}` dependency for
unwind_prot.o.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants