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

darwin.apple_sdk_11_0: add Security dependency on xpc #207123

Merged
merged 1 commit into from
Jan 11, 2023

Conversation

divanorama
Copy link
Contributor

@divanorama divanorama commented Dec 21, 2022

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 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 #203519

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

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
@uri-canva
Copy link
Contributor

@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?

@stephank
Copy link
Contributor

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 modules list. The entries of this are pairs, where the first is an identifier ({ clang: "foo" } or { swift: "foo"} to distinguish Clang / Swift modules.) and the second metadata. Security is a Clang module only (no Swift overload), and has XPC listed in directDependencies.

@uri-canva
Copy link
Contributor

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.

@uri-canva uri-canva merged commit ea9c9ba into NixOS:staging Jan 11, 2023
@stephank
Copy link
Contributor

stephank commented Jan 17, 2023

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 bazel-watcher and bazel_6 on aarch64-darwin, using current staging with this change reverted, and both work.

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.

@uri-canva
Copy link
Contributor

Is there a package that is broken by this change?

@uri-canva
Copy link
Contributor

I can see bazel-watcher is fine on master and that doesn't have this change: https://hydra.nixos.org/job/nixpkgs/trunk/bazel-watcher.aarch64-darwin https://hydra.nixos.org/job/nixpkgs/trunk/bazel-watcher.x86_64-darwin.

@stephank
Copy link
Contributor

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.

@uri-canva
Copy link
Contributor

Ok let's go ahead and revert this, we can always try again it's a fairly small change if we need it!

@uri-canva
Copy link
Contributor

I'll let you do it since you're looking at it.

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.

3 participants