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.

Sign up for GitHub

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

Jump to bottom

apple_sdk_11_0: generate frameworks.nix #196197

Merged
merged 4 commits into from
Mar 18, 2023
Merged

apple_sdk_11_0: generate frameworks.nix #196197

merged 4 commits into from
Mar 18, 2023

Conversation

stephank
Copy link
Contributor

@stephank stephank commented Oct 15, 2022 •
edited
Loading

Description of changes

This is an attempt to generate frameworks.nix using the Swift compiler. Via swiftc -scan-dependencies, the compiler outputs JSON we can easily consume. A script runs this on a very simple Swift module that does just import SomeFramework, for every framework in the input SDK.

This PR is currently split in four commits:

  • The first commit adds just the script.
  • The second commit replaces frameworks.nix with a generated version. Dependencies that would've been removed in this process were added back as manual overrides in apple_sdk.nix.
  • The third commit fixes circular references in the Nix expression by removing some of the manual overrides.
  • The fourth commit slightly changes the way we unpack the macOS SDK, so that we preserve all files. This is really just SDKSettings.{plist,json}, there wasn't much else missing, but those files happen to be essential for the Swift compiler to work with the SDK as downloaded by Nix.

This successfully builds stdenv. I also tested swift, in order to run the script itself. As an extra check, I also built mpv, which worked after a partial revert of #211864 (currently only on staging), and after adding an Accelerate framework dependency (but it appears mpv is similarly broken on master). Especially mpv also uses rust, gfortran, python and a whole bunch of things in its build closure, so I think that makes this decently tested?

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.

@ofborg ofborg bot added 10.rebuild-darwin: 501+ 10.rebuild-darwin: 1001-2500 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Oct 15, 2022
Atemu
Atemu reviewed Oct 16, 2022
View reviewed changes
pkgs/os-specific/darwin/apple-sdk-11.0/apple_sdk.nix Outdated
Comment on lines 166 to 213
in {
# Below this comment are entries migrated from before the generator was
# added. If, for a given framework, you are able to reverify the extra
# deps are really necessary on top of the generator deps, move it above
# this comment (and maybe document your findings).
AVFoundation = { inherit ApplicationServices AVFCapture AVFCore; };
Accelerate = { inherit CoreWLAN IOBluetooth; };
AddressBook = { inherit AddressBookCore ContactsPersistence libobjc; };
AppKit = { inherit AudioToolbox AudioUnit UIFoundation; };
AudioToolbox = { inherit AudioToolboxCore; };
AudioUnit = { inherit Carbon CoreAudio; };
Carbon = { inherit IOKit QuartzCore libobjc; };
CoreAudio = { inherit IOKit; };
CoreFoundation = { inherit libobjc; };
CoreGraphics = { inherit Accelerate IOSurface SystemConfiguration; };
CoreMIDIServer = { inherit CoreMIDI; };
CoreMedia = { inherit ApplicationServices AudioToolbox AudioUnit; };
CoreServices = { inherit CoreAudio CoreData NetFS OpenDirectory ServiceManagement; };
CoreWLAN = { inherit SecurityFoundation; };
DiscRecording = { inherit IOKit libobjc; };
Foundation = { inherit SystemConfiguration libobjc; };
GameKit = { inherit GameCenterFoundation GameCenterUI GameCenterUICore ReplayKit; };
ICADevices = { inherit Carbon libobjc; };
IOBluetooth = { inherit CoreBluetooth; };
JavaScriptCore = { inherit libobjc; };
Kernel = { inherit IOKit; };
LinkPresentation = { inherit URLFormatting; };
MediaToolbox = { inherit AudioUnit; };
MetricKit = { inherit SignpostMetrics; };
Network = { inherit libnetwork; };
PCSC = { inherit CoreData; };
PassKit = { inherit PassKitCore; };
QTKit = { inherit CoreMedia CoreMediaIO MediaToolbox VideoToolbox; };
Quartz = { inherit QTKit; };
QuartzCore = { inherit ApplicationServices CoreImage CoreVideo Metal OpenCL libobjc; };
Security = { inherit IOKit libDER; };
TWAIN = { inherit Carbon; };
VideoDecodeAcceleration = { inherit CoreVideo; };
WebKit = { inherit ApplicationServices Carbon libobjc; };
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are quite a lot. How were these figured out?

Could this at least be partially automated somehow?

Copy link
Contributor Author

@stephank stephank Oct 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are the difference between the generated set and old set. So everything in the old frameworks.nix the generator could not find. (I did this manually because we only have to do it once, in theory.)

I think each of those requires some investigation to see if they are still needed. But for now I thought let's not remove anything to prevent breakage elsewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To add to this, I think when we add other SDK versions, we should maybe try starting without these manual dependencies. For the 11.0 SDK, that could be trouble because it's the base for aarch64-darwin stdenv. But any other SDK version will likely be a per-package opt-in situation, and we can reintroduce manual dependencies as we find them.

@stephank stephank force-pushed the feat/darwin-gen-frameworks branch from 86a6825 to b9c6525 Compare January 17, 2023 14:18
@stephank stephank force-pushed the feat/darwin-gen-frameworks branch from b9c6525 to d3ebc95 Compare January 31, 2023 15:30
@stephank stephank marked this pull request as ready for review January 31, 2023 19:08
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/3032/1958

@SuperSandro2000 SuperSandro2000 requested a review from a team March 17, 2023 09:22
viraptor
viraptor approved these changes Mar 17, 2023
View reviewed changes
Copy link
Contributor

@viraptor viraptor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense to me

pkgs/os-specific/darwin/gen-frameworks.py Outdated
]
frameworks.sort()

# Determine the longest ame for padding output.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# Determine the longest ame for padding output.
# Determine the longest name for padding output.

@lovesegfault lovesegfault requested a review from Atemu March 17, 2023 16:12
Atemu
Atemu approved these changes Mar 17, 2023
View reviewed changes
Copy link
Member

@Atemu Atemu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Diff LGTM

stephank added 4 commits March 17, 2023 21:30
@stephank
apple_sdk_11_0: script to generate frameworks.nix
7f90949
@stephank
apple_sdk_11_0: generate frameworks.nix
b9e6d1d
@stephank
apple_sdk_11_0: fix circular framework references
0c3aaf3
@stephank
apple_sdk_11_0: simplify SDK unpacking
a0537d6 
The SDK was missing SDKSettings files. This is usually not a problem for
Nix builds, because we generate our own fake SDK structure when
necessary (in xcbuild), but not having these files blocks using the
upstream Apple SDK in tooling such as gen-frameworks.py.
@stephank stephank force-pushed the feat/darwin-gen-frameworks branch from d3ebc95 to a0537d6 Compare March 17, 2023 20:31
@stephank
Copy link
Contributor Author

Thanks for the reviews! I fixed the typo and rebased on staging, but no other changes.

@viraptor
Copy link
Contributor

I think it's good enough for staging. If there's a massive breakage, we can still spot it in Hydra and fix up/revert.

@viraptor viraptor merged commit 65188d6 into NixOS:staging Mar 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Atemu Atemu Atemu approved these changes

@viraptor viraptor viraptor approved these changes

@berryp berryp berryp approved these changes

Assignees
No one assigned
Labels
10.rebuild-darwin: 501+ 10.rebuild-darwin: 5001+ 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@stephank @nixos-discourse @viraptor @berryp @Atemu