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

Have IOSurface depend on Libsystem rather than XPC #163052

Closed
toonn opened this issue Mar 6, 2022 · 6 comments
Closed

Have IOSurface depend on Libsystem rather than XPC #163052

toonn opened this issue Mar 6, 2022 · 6 comments
Labels
0.kind: bug 6.topic: darwin Running or building packages on Darwin
Projects

Comments

@toonn
Copy link
Contributor

toonn commented Mar 6, 2022

Describe the bug

The IOSurface framework as defined by the apple-source-releases depends on XPC. This causes redefinition errors because other frameworks get these definitions from Libsystem. There was an attempt to fix it in PR #161561, but this causes build problems due to Libsystem and libc++ being incompatible currently, so it was reverted in PR #162681.

Steps To Reproduce

The first problem:

  1. Build something that depends on Libsystem and IOSurface, like mac-notification-sys.

The second problem:

  1. Apply the patch having IOSurface depend on Libsystem rather than XPC.
  2. Build something depending on IOSurface and libc++, like xcbuild.

Expected behavior

The incompatibility between Libsystem and libc++ should be fixed so we can have IOSurface properly depend on Libsystem without causing problems.

@toonn toonn added 0.kind: bug 6.topic: darwin Running or building packages on Darwin labels Mar 6, 2022
@toonn toonn changed the title Have IOSurface depend on Libsystem rathen than XPC Have IOSurface depend on Libsystem rather than XPC Mar 6, 2022
@veprbl veprbl added this to Package pain points in Darwin Mar 7, 2022
@tjni
Copy link
Contributor

tjni commented Apr 15, 2022

This issue is blocking me, so I started to investigate it, but I need some help.

With PR #161561 applied, I can reproduce the xcbuild build error. By adding some debugging print statements, we can observe that the arguments to clang++ include:

-isystem /nix/store/czzxpxnigp3r0zb4sklwksv5zahsgkdp-libSystem-11.0.0/include
...
-isystem /nix/store/wlp2bii0r52zh2p7bkp84v0jwqj22ya9-libcxx-11.1.0-dev/include/c++/v1

Because libSystem is added to -isystem before libcxx, the cmath file from libcxx includes math.h from libSystem instead of from libcxx, compilation breaks. libcxx needs its own math.h, because it redefines macros such as signbit as functions and then refers to them as such (e.g. ::signbit).

I'm not very experienced with C/C++, so I could use some opinions on how this conflict should be handled. Thank you!

/cc @toonn

@tjni
Copy link
Contributor

tjni commented Apr 15, 2022

Perhaps it would make sense to remove the path to libSystem (in general, the path to glibc) from CPLUS_INCLUDE_PATH in the clang wrapper. Or, alternatively, add libcxx to the beginning of CPLUS_INCLUDE_PATH instead of to the end.

/cc @cmm @Mic92, I saw both of you have worked on resolving these sorts of issues before, and I'd love your thoughts.

@toonn
Copy link
Contributor Author

toonn commented Apr 22, 2022

Good digging. I'm not sure where the include order is determined. Would changing this order be a fix or would other programs expect libSystem's math.h?

@tjni
Copy link
Contributor

tjni commented Apr 22, 2022

In theory, including libc++'s math.h before libSystem's math.h should be safe, because libc++'s is compatible:

#include_next <math.h>

// redefines some macros from math.h as functions

However, I would be more surprised if something doesn't break after making this change either.

I am trying to dig into this more today, but my lack of experience with debugging and developing in nixpkgs is showing. My plan is to connect to the Matrix channel and ask for some advice there.

tjni added a commit to tjni/nixpkgs that referenced this issue Aug 7, 2022
This fixes NixOS#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 added a commit to tjni/nixpkgs that referenced this issue Aug 7, 2022
This fixes NixOS#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.
@stephank
Copy link
Contributor

stephank commented Oct 2, 2022

I think this can be closed now that #191235 is merged?

@toonn
Copy link
Contributor Author

toonn commented Oct 3, 2022

Yes, thank you for pointing this out!

Fixed in #191235

@toonn toonn closed this as completed Oct 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0.kind: bug 6.topic: darwin Running or building packages on Darwin
Projects
Darwin
  
Package pain points
Development

Successfully merging a pull request may close this issue.

3 participants