Skip to content

cc-wrapper, clang-tools: move libcxx before cflags#185569

Closed
tjni wants to merge 1 commit intoNixOS:stagingfrom
tjni:libstd-prefix-staging
Closed

cc-wrapper, clang-tools: move libcxx before cflags#185569
tjni wants to merge 1 commit intoNixOS:stagingfrom
tjni:libstd-prefix-staging

Conversation

@tjni
Copy link
Contributor

@tjni tjni commented Aug 7, 2022

This is #169962 rebased against the staging branch.


This fixes #163052, where the xcbuild build fails on Darwin after
libSystem is added as a dependency of IOSurface.

Even though cc-wrapper adds libc to NIX_CFLAGS_COMPILE using the
-idirafter flag, ensuring it comes after the C++ standard library,
and the clang-tools wrapper respects -idirafter when it parses
arguments, it appears that "-isystem libc" can make its way onto
NIX_CFLAGS_COMPILE too.

In honesty, I have not been able to figure out how NIX_CFLAGS_COMPILE
inherits "-isystem libSystem", or how it influences the xcbuild build
itself. However:

  1. This change does appear to fix the xcbuild build.

  2. If NIX_CFLAGS_COMPILE does have "-isystem libSystem" added to it
    somehow, it should probably come after the C++ standard library.

Description of changes
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/)
  • 22.11 Release Notes (or backporting 22.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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

This fixes #163052, where the xcbuild build fails on Darwin after
libSystem is added as a dependency of IOSurface.

Even though cc-wrapper adds libc to NIX_CFLAGS_COMPILE using the
-idirafter flag, ensuring it comes after the C++ standard library,
and the clang-tools wrapper respects -idirafter when it parses
arguments, it appears that "-isystem libc" can make its way onto
NIX_CFLAGS_COMPILE too.

In honesty, I have not been able to figure out how NIX_CFLAGS_COMPILE
inherits "-isystem libSystem", or how it influences the xcbuild build
itself. However:

1. This change does appear to fix the xcbuild build.

2. If NIX_CFLAGS_COMPILE does have "-isystem libSystem" added to it
   somehow, it _should_ probably come after the C++ standard library.
@tjni tjni requested a review from Ericson2314 as a code owner August 7, 2022 15:55
@ofborg ofborg bot added 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild on Darwin and must target a staging branch. 10.rebuild-linux-stdenv This PR causes stdenv to rebuild on Linux and must target a staging branch. 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. labels Aug 7, 2022
@tjni
Copy link
Contributor Author

tjni commented Aug 9, 2022

@toonn, would you be willing to skim this and share your thoughts or suggestions?

My aim ultimately is to fix cargo-watch on my M1 Mac.

@toonn
Copy link
Contributor

toonn commented Aug 26, 2022

I'm looking into this, xcbuild indeed builds with these commits. Now I'm verifying it indeed doesn't without them. (Apologies for not getting around to it sooner but I was away for a couple weeks and then nixpkgs-review gave me a hard time, so I'm checking things out manually for now.)

@toonn
Copy link
Contributor

toonn commented Aug 26, 2022

Ok, the build did not fail for me without this change but I just noticed this is specific to aarch64-darwin... I'll ask whether someone can test this.

@malob
Copy link
Member

malob commented Aug 26, 2022

Ok, the build did not fail for me without this change but I just noticed this is specific to aarch64-darwin... I'll ask whether someone can test this.

@toonn, I'm running the build for xcbuild at this commit now on aarch64-darwin.

@malob
Copy link
Member

malob commented Aug 26, 2022

Alright I ran,

~/C/nixpkgs on libstd-prefix-stagingnix build -f . xcbuild

which ran for about 45 minutes then failed when trying to build dejagnu. I ran the same command again and had the same issue.

I then tried again with sandboxing enabled,

~/C/nixpkgs on libstd-prefix-stagingnix --option sandbox true build -f . xcbuild

and the build picked up where it left off, and finished after another couple of hours.

Here's the log of the failed build for dejagnu when sandboxing wasn't enabled (unfortunately it's not really that helpful):

Failed dejagnu build log
@nix { "action": "setPhase", "phase": "unpackPhase" }
unpacking sources
unpacking source archive /nix/store/lhfgbqdaizrv41549mm458r900qqya8r-dejagnu-1.6.3.tar.gz
source root is dejagnu-1.6.3
setting SOURCE_DATE_EPOCH to timestamp 1623898533 of file dejagnu-1.6.3/doc/stamp-vti
@nix { "action": "setPhase", "phase": "patchPhase" }
patching sources
@nix { "action": "setPhase", "phase": "updateAutotoolsGnuConfigScriptsPhase" }
updateAutotoolsGnuConfigScriptsPhase
Updating Autotools / GNU config script to a newer upstream version: ./config.sub
Updating Autotools / GNU config script to a newer upstream version: ./config.guess
@nix { "action": "setPhase", "phase": "configurePhase" }
configuring
configure flags: --disable-dependency-tracking --prefix=/nix/store/aprcyq4m92gg3zzs1mz9da5xn901mak8-dejagnu-1.6.3
checking for a BSD-compatible install... /nix/store/z3d2skwvafb2rdfhbwivkp4lfhr21njp-coreutils-9.1/bin/install -c
checking whether build environment is sane... yes
checking for a thread-safe mkdir -p... /nix/store/z3d2skwvafb2rdfhbwivkp4lfhr21njp-coreutils-9.1/bin/mkdir -p
checking for gawk... gawk
checking whether make sets $(MAKE)... yes
checking whether make supports nested variables... yes
checking whether to enable maintainer-specific portions of Makefiles... no
checking whether make sets $(MAKE)... (cached) yes
checking for gawk... (cached) gawk
checking for gcc... clang
checking whether the C compiler works... yes
checking for C compiler default output file name... a.out
checking for suffix of executables... 
checking whether we are cross compiling... no
checking for suffix of object files... o
checking whether we are using the GNU C compiler... yes
checking whether clang accepts -g... yes
checking for clang option to accept ISO C89... none needed
checking whether clang understands -c and -o together... yes
checking for style of include used by make... GNU
checking dependency style of clang... none
checking whether we are using the GNU C++ compiler... yes
checking whether clang++ accepts -g... yes
checking dependency style of clang++... none
checking that generated files are newer than configure... done
configure: creating ./config.status
config.status: creating Makefile
config.status: executing depfiles commands
@nix { "action": "setPhase", "phase": "buildPhase" }
building
build flags: SHELL=/nix/store/zmfjjgbm4bjxwx46i4pdic51jg64lgjq-bash-5.1-p16/bin/bash
Done. Now run 'make install'.
@nix { "action": "setPhase", "phase": "checkPhase" }
running tests
Done. Now run 'make install'.
make  unit
make[1]: Entering directory '/private/tmp/nix-build-dejagnu-1.6.3.drv-0/dejagnu-1.6.3/build'
clang++ -DPACKAGE_NAME=\"GNU\ DejaGnu\" -DPACKAGE_TARNAME=\"dejagnu\" -DPACKAGE_VERSION=\"1.6.3\" -DPACKAGE_STRING=\"GNU\ DejaGnu\ 1.6.3\" -DPACKAGE_BUGREPORT=\"bug-dejagnu@gnu.org\" -DPACKAGE_URL=\"http://www.gnu.org/software/dejagnu/\" -DPACKAGE=\"dejagnu\" -DVERSION=\"1.6.3\" -I. -I..    -I.. -g -g -O2 -c -o testsuite/libdejagnu/unit.o ../testsuite/libdejagnu/unit.cc
clang++ -I.. -g -g -O2   -o unit testsuite/libdejagnu/unit.o  
make[1]: Leaving directory '/private/tmp/nix-build-dejagnu-1.6.3.drv-0/dejagnu-1.6.3/build'
make  check-DEJAGNU
make[1]: Entering directory '/private/tmp/nix-build-dejagnu-1.6.3.drv-0/dejagnu-1.6.3/build'
Making a new site.exp file ...
srcdir='..'; export srcdir; \
EXPECT=expect; export EXPECT; \
if /nix/store/zmfjjgbm4bjxwx46i4pdic51jg64lgjq-bash-5.1-p16/bin/bash -c "../runtest --version" > /dev/null 2>&1; then \
  exit_status=0; l='launcher libdejagnu report-card runtest'; for tool in $l; do \
    if ../runtest  --tool $tool --srcdir $srcdir ; \
    then :; else exit_status=1; fi; \
  done; \
else echo "WARNING: could not find '../runtest'" 1>&2; :;\
fi; \
exit $exit_status
Using ../testsuite/lib/launcher.exp as tool init file.
Test run by nix-build-daemon on Fri Aug 26 22:08:16 2022
Native configuration is aarch64-apple-darwin22.1.0

		=== launcher tests ===

Schedule of variations:
    unix

Running target unix
Using ../baseboards/unix.exp as board description file for target.
Using ../config/unix.exp as generic interface file for target.
Using ../testsuite/../config/unix.exp as tool-and-target-specific interface file.
Running ../testsuite/launcher.all/command.exp ...
Running ../testsuite/launcher.all/help.exp ...
Running ../testsuite/launcher.all/interp.exp ...
Running ../testsuite/launcher.all/verbose.exp ...

		=== launcher Summary ===

# of expected passes		54
Using ../testsuite/lib/libdejagnu.exp as tool init file.
Test run by nix-build-daemon on Fri Aug 26 22:08:19 2022
Native configuration is aarch64-apple-darwin22.1.0

		=== libdejagnu tests ===

Schedule of variations:
    unix

Running target unix
Using ../baseboards/unix.exp as board description file for target.
Using ../config/unix.exp as generic interface file for target.
Using ../testsuite/../config/unix.exp as tool-and-target-specific interface file.
Running ../testsuite/libdejagnu/tunit.exp ...

		=== libdejagnu Summary ===

# of expected passes		5
Using ../testsuite/lib/report-card.exp as tool init file.
Test run by nix-build-daemon on Fri Aug 26 22:08:19 2022
Native configuration is aarch64-apple-darwin22.1.0

		=== report-card tests ===

Schedule of variations:
    unix

Running target unix
Using ../baseboards/unix.exp as board description file for target.
Using ../config/unix.exp as generic interface file for target.
Using ../testsuite/../config/unix.exp as tool-and-target-specific interface file.
Running ../testsuite/report-card.all/onetest.exp ...
Running ../testsuite/report-card.all/passes.exp ...

		=== report-card Summary ===

# of expected passes		245
Using ../testsuite/lib/runtest.exp as tool init file.
Test run by nix-build-daemon on Fri Aug 26 22:08:20 2022
Native configuration is aarch64-apple-darwin22.1.0

		=== runtest tests ===

Schedule of variations:
    unix

Running target unix
Using ../baseboards/unix.exp as board description file for target.
Using ../config/unix.exp as generic interface file for target.
Using ../testsuite/../config/unix.exp as tool-and-target-specific interface file.
Running ../testsuite/runtest.libs/libs.exp ...
Running ../testsuite/runtest.libs/clone_output.test ...
Running ../testsuite/runtest.libs/config.test ...
Running ../testsuite/runtest.libs/remote.test ...
Running ../testsuite/runtest.libs/target.test ...
Running ../testsuite/runtest.libs/testcase_group.test ...
Running ../testsuite/runtest.libs/testsuite_can.test ...
Running ../testsuite/runtest.libs/testsuite_file.test ...
Running ../testsuite/runtest.libs/utils.test ...
Running ../testsuite/runtest.main/error.exp ...
Running ../testsuite/runtest.main/options.exp ...
Running ../testsuite/runtest.main/pr42399.exp ...
Running ../testsuite/runtest.main/pr48155.exp ...
Running ../testsuite/runtest.main/stats.exp ...

		=== runtest Summary ===

# of expected passes		298
# of unresolved testcases	2
DejaGnu version	1.6.3
Expect version	5.45.4
Tcl version	8.6

make[1]: *** [Makefile:1097: check-DEJAGNU] Error 1
make[1]: Leaving directory '/private/tmp/nix-build-dejagnu-1.6.3.drv-0/dejagnu-1.6.3/build'
make: *** [Makefile:1307: check-am] Error 2

@malob
Copy link
Member

malob commented Aug 27, 2022

Building xcbuild also succeeded on aarch64-darwin using the previous commit. Sandboxing was also required to get dejagnu to build.

@winterqt
Copy link
Member

xcbuild currently builds on aarch64-darwin. Is this only supposed to fix the build when applied along with #161561, or am I missing something here?

@tjni
Copy link
Contributor Author

tjni commented Aug 28, 2022

I have been preoccupied these last two days with both work and personal matters, so I'm sorry to all I have not been able to review your comments and feedback. I will as soon as I can next week. Thank you for your attention on this already.

xcbuild currently builds on aarch64-darwin. Is this only supposed to fix the build when applied along with #161561, or am I missing something here?

Yes, you're exactly right.

@tjni
Copy link
Contributor Author

tjni commented Sep 2, 2022

I had time today to revisit this issue. TLDR: I think the right thing to do is close this and pursue a different approach.

Today, cc-wrapper by default will add libstdc++/libc++ and libc together to the include search path in the right order. I think this is supposed to mimic how a non-nixpkgs gcc or clang normally finds these libraries in standard system directories.

Within nixpkgs, dependencies are added to the include path using -isystem (maybe through cc-wrapper's setup hook?). This means that they will be found before the standard system directories. Current code emulates this by adding libcstdc++/libc++ to the include path as the very last -isystem entry, which I don't think should change.

Now, #161561 adds an explicit dependency on libc, which is added to the include path using -isystem. This in turn breaks an assumption that libc must come after libstdc++/libc++.

I happen to like explicit dependencies, including on standard libraries, but I think the assumption is that the C and C++ standard libraries are provided implicitly unless the build takes over including both in the right order, for which packages are not set up. My current thinking is that it would be better to bring back #161561 by just removing xdc and relying on cc-wrapper to provide Libsystem implicitly. What does everyone else think?

@tjni tjni closed this Sep 4, 2022
@toonn
Copy link
Contributor

toonn commented Sep 5, 2022

Sounds like a reasonable (though unfortunate) compromise to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild on Darwin and must target a staging branch. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. 10.rebuild-linux-stdenv This PR causes stdenv to rebuild on Linux and must target a staging branch.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants