-
-
Notifications
You must be signed in to change notification settings - Fork 14.6k
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
darwin.apple_sdk_11_0: add Security dependency on xpc #207123
Conversation
Some packages like `bazel-watcher` seems to have broken after 9dc3b14 Where `xpc` was removed from `IOSurface` dependencies. `CoreServices` were pulling `xpc` via `IOSurface` and so `Security` didn't break. Now explicit dependency on `xpc` is needed to avoid errors like ``` In file included from __main__/external/com_github_fsnotify_fsevents/go_1_10_after.go:6: In file included from /nix/store/2k3mdkl9jvwwzpbfaqhchfiqjq64046b-apple-framework-CoreServices-11.0.0/Library/Frameworks/CoreServices.framework/Headers/CoreServices.h:39: In file included from /nix/store/2k3mdkl9jvwwzpbfaqhchfiqjq64046b-apple-framework-CoreServices-11.0.0/Library/Frameworks/CoreServices.framework/Frameworks/LaunchServices.framework/Headers/LaunchServices.h:23: In file included from /nix/store/2k3mdkl9jvwwzpbfaqhchfiqjq64046b-apple-framework-CoreServices-11.0.0/Library/Frameworks/CoreServices.framework/Frameworks/LaunchServices.framework/Headers/IconsCore.h:23: In file included from /nix/store/2k3mdkl9jvwwzpbfaqhchfiqjq64046b-apple-framework-CoreServices-11.0.0/Library/Frameworks/CoreServices.framework/Frameworks/OSServices.framework/Headers/OSServices.h:29: In file included from /nix/store/2k3mdkl9jvwwzpbfaqhchfiqjq64046b-apple-framework-CoreServices-11.0.0/Library/Frameworks/CoreServices.framework/Frameworks/OSServices.framework/Headers/CSIdentity.h:43: In file included from /nix/store/dg51rm1bapffbqvn46bh43km4dhcsy9p-apple-framework-Security-11.0.0/Library/Frameworks/Security.framework/Headers/Security.h:87: /nix/store/dg51rm1bapffbqvn46bh43km4dhcsy9p-apple-framework-Security-11.0.0/Library/Frameworks/Security.framework/Headers/SecCode.h:35:10: fatal error: 'xpc/xpc.h' file not found ^~~~~~~~~~~ ``` Should help with NixOS#203519
@toonn @stephank Could you help me look at this? I'm not familiar enough with the staging workflow and testing low level changes, but could like to be. I have read the manual section on the staging workflow https://nixos.org/manual/nixpkgs/unstable/#id-1.6.4.8.3 so I understand the theory behind it, I'm just not sure about the practical aspects. Do you know of any other documentation I could refer to? |
Tbh, workflow is a little bit of a mystery to me too. 🙂 Hydra doesn't build staging, so testing is manual and something you have to arrange for yourself, I believe? We recently had a community M1 Mac Mini set up which I've been using for large testing builds. You can ask Winter in #macos:nixos.org for access to that, I think yay or nay is reputation-based. (I believe there are also Linux community boxes, but don't use them or know who manages those.) Staging-next is tested by Hydra. There is always a PR open for staging-next while it's active, and you'll likely be pinged for issues related to your change, but it's also recommended you try monitor these builds yourself. I'm not sure how averse we are to reverting changes. (Ie. if it's okay to land small changes on staging without manual testing, then see how they fare on Hydra in staging-next and revert if necessary.) As for this PR, I think it looks good on the surface. Security does have a dependency on XPC, so this makes sense. You can verify this with something like: swiftc -scan-dependencies - <<< "import Security" This spits out a structure with a |
Ok, sounds good. Since we built various packages that depended on this change when we mistakenly merged it before I'd say it's been tested locally. I'll merge this and monitor the staging-next PR. |
I'd like to bring this up again. In the macOS 11 SDK, the xpc package contains only headers, and those same headers are already part of libsystem / stdenv. So I believe this shouldn't change anything? I tried builds of The issue I have is that this is introducing two sets of xpc headers in include paths, and that's an error for Swift modules. |
Is there a package that is broken by this change? |
I can see |
Swift support for Darwin was recently merged into staging (#189977), and I'm fixing merge issues. The Swift compiler itself doesn't build after the merge because of the two xpc modules in the include path. |
Ok let's go ahead and revert this, we can always try again it's a fairly small change if we need it! |
I'll let you do it since you're looking at it. |
Description of changes
recreated #207104 - changing target branch from master to staging exploded there because it can't be done atomically with rebase in github UI and picks up lots of master commits, adds lots of codeowners etc.
Some packages like
bazel-watcher
seems to have broken after 9dc3b14Where
xpc
was removed fromIOSurface
dependencies.CoreServices
were pullingxpc
viaIOSurface
and soSecurity
didn't break. Now explicit dependency onxpc
is needed to avoid errors likeShould help with #203519
Things done
sandbox = true
set innix.conf
? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)nixos/doc/manual/md-to-db.sh
to update generated release notes