Skip to content

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

Closed
tjni wants to merge 154 commits intoNixOS:masterfrom
tjni:libstd-prefix
Closed

cc-wrapper, clang-tools: move libcxx before cflags#169962
tjni wants to merge 154 commits intoNixOS:masterfrom
tjni:libstd-prefix

Conversation

@tjni
Copy link
Contributor

@tjni tjni commented Apr 23, 2022

Description of changes

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.

Notes

  1. I am not fully happy with my level of understanding of how this problem manifests, which also means I'm not certain that this is the right way to fix this issue. But I have reached a point where I can't do more without guidance. (For instance, the successful build of xcbuild took an hour and a half on my computer, because I changed cc-wrapper, and I can't learn by trial and error with that sort of build time.) I'd be happy to take advice though.

  2. If this approach looks sound, I then both want to trigger more builds to check what effect this has (how?) and wary of doing so because it might use up a lot of build infrastructure. What should I do?

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.05 Release Notes (or backporting 21.11 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.

@tjni tjni requested a review from Ericson2314 as a code owner April 23, 2022 15:24
@tjni
Copy link
Contributor Author

tjni commented Apr 23, 2022

This looks safe to do if the C++ library used is libc++, as I notice in many of its headers that it can handle libc on the path, e.g, for math.h:

#include_next <math.h>

// redefines some macros from math.h as functions

I am not sure about other C++ standard libraries.

@tjni
Copy link
Contributor Author

tjni commented Apr 23, 2022

I'll need some time to set up an x86_64 Linux box to try to reproduce the build failure in libuv.

@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 Apr 25, 2022
@tjni
Copy link
Contributor Author

tjni commented Apr 26, 2022

I'll need some time to set up an x86_64 Linux box to try to reproduce the build failure in libuv.

I set one up and couldn't reproduce the libuv build (which is actually a test) failure. I triggered a couple new builds, and they went away, so it looks flaky. I think this is ready for review, unless the hanging Darwin tests could somehow be triggered to further verify this.

@tjni
Copy link
Contributor Author

tjni commented Apr 28, 2022

/cc @Mic92 @cmm

This touches the clang wrapper as well, so please take a look if you've got some time. Thank you!

@tjni
Copy link
Contributor Author

tjni commented May 3, 2022

@Ericson2314 would you be able to either take a look or point me in a direction?

I'm currently looking for next steps, whether that is to revise the approach, do more testing, come back to this later (such as after the current release window), or something else. Thank you!

bootstrap-prime and others added 18 commits July 23, 2022 20:35
@bootstrap-prime
libfido2: 1.10.0 -> 1.11.0
a79c8da
@bootstrap-prime
polkit: use systemdMinimal to avoid dep cycle
56d0dd7
@bootstrap-prime
pcsclite: use systemdMinimal to avoid dep cycle
dad5575
@bootstrap-prime
bluez: use systemdMinimal to avoid dep cycle
3b9048a 
also make test output unconditional
@bootstrap-prime
dbus: use systemdMinimal to avoid dep cycle
5f43dbe
@bootstrap-prime
python-dbusmock: refactor check, avoid dep cycle
aa5659c 
python-dbusmock relies on bluez for it's checks. Which shouldn't be a
problem, and isn't one normally. However, checkPhase contained a string
substitution that was always constructed regardless of the value of
doCheck (and simply not used if doCheck was false), and so bluez, which
was not supposed to be a dependency, was used as and registered as one
and caused a dependency cycle when trying to add pcsclite as a
dependency of libfido2.

The string substitution has been removed in favor of something that can
remain disabled when doCheck is false.
@bootstrap-prime
libfido2: depend on pcsclite to support nfc keys
e384d91
@SuperSandro2000
python310Packages.python-dbusmock: 0.28.3 -> 0.28.4
a885d2a
@wamserma
freeglut: 3.2.1 -> 3.2.2
43f0842
@stigtsp
gnupg: 2.3.6 -> 2.3.7
bf81738 
- Release announcement: https://dev.gnupg.org/T5947
- Removed CVE-2022-34903 patch which is included in 2.3.7
@r-ryantm
ell: 0.51 -> 0.52
d338b73
@SuperSandro2000
python310Packages.urllib3: 1.26.10 -> 1.26.11
4c1987d
@r-ryantm
libass: 0.15.2 -> 0.16.0
51d1d82
@trofi
windows.mingw_w64: enable stripping
d82ce04 
With 0507725 "setup-hooks/strip.sh: run RANLIB on static
archives after stripping" it should now be safe to run strip
on wider range of .a files.

mingw-w64 is a good example where a reference was preserved
to gcc from intermediate stage. Enabling stripping by default
decreases `pkgsCross.mingw32` closure for 20%:

Before:

    $ nix path-info -rsSh $(nix-build -A pkgsCross.mingw32.stdenv)
    ...
    /nix/store/qzhkidff0wxhqf2gi97ng6qismzvpnbp-stdenv-linux 43.6K    1.0G

After:

    $ nix path-info -rsSh $(nix-build -A pkgsCross.mingw32.stdenv)
    ...
    /nix/store/fj4dv1n3sa3jgcb1j3nwn6njsf943n48-stdenv-linux 43.6K  792.4M
@r-ryantm
patchelf: 0.14.5 -> 0.15.0
c826b18
@SuperSandro2000
Merge pull request #183169 from stigtsp/package/gnupg-2.3.7-staging
6bbcbc0 
gnupg: 2.3.6 -> 2.3.7
@github-actions
Merge staging-next into staging
466b084
@SuperSandro2000
gnupg: remove ? null from inputs, remove with lib
32904b7
@github-actions github-actions bot added 6.topic: golang Go is a high-level general purpose programming language that is statically typed and compiled. 6.topic: python Python is a high-level, general-purpose programming language. 6.topic: stdenv Standard environment 6.topic: vim Advanced text editor labels Aug 7, 2022
@tjni
Copy link
Contributor Author

tjni commented Aug 7, 2022

I am going to close this PR in favor of a new one that tracks staging. I tried to rebase it and it looks confused.

@tjni tjni closed this Aug 7, 2022
@tjni tjni mentioned this pull request Aug 7, 2022
Closed
13 tasks
@tjni
Copy link
Contributor Author

tjni commented Aug 7, 2022

The replacement PR for this is #185569.

@winterqt
Copy link
Member

winterqt commented Aug 7, 2022

For what it's worth @tjni, you can change the base branch for a PR without having to make a new one. Just so you know for the future :)

@ofborg ofborg bot added 8.has: clean-up This PR removes packages or removes other cruft 8.has: package (new) This PR adds a new package labels Aug 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

6.topic: golang Go is a high-level general purpose programming language that is statically typed and compiled. 6.topic: python Python is a high-level, general-purpose programming language. 6.topic: stdenv Standard environment 6.topic: vim Advanced text editor 8.has: clean-up This PR removes packages or removes other cruft 8.has: package (new) This PR adds a new package 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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Have IOSurface depend on Libsystem rather than XPC