Skip to content

Comments

darwin: check apple headers#109368

Merged
veprbl merged 15 commits intoNixOS:stagingfrom
holymonson:check-headers
Feb 1, 2021
Merged

darwin: check apple headers#109368
veprbl merged 15 commits intoNixOS:stagingfrom
holymonson:check-headers

Conversation

@holymonson
Copy link
Contributor

Motivation for this change

We assemble Libsystem by borrowing headers for other lib packages, and when those packages update some headers may be dropped by APPLE. In order to updating without losing header files, we maintain a header list for each of them to reduce missing risk.

This could also benefit knowing where a header file comes from, because some of them may overlap in separated packages.

Things done

Tested building all apple packages, but not for bootstrap.

  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@ofborg ofborg bot added the 6.topic: darwin Running or building packages on Darwin label Jan 14, 2021
@ofborg ofborg bot added 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild on Darwin and must target a staging branch. 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-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. labels Jan 14, 2021
@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/445

@holymonson
Copy link
Contributor Author

@veprbl could you review this?

@veprbl
Copy link
Member

veprbl commented Jan 17, 2021

Strict header check diminishes the possibility that certain header file functionality will be lost. This, however, adds some burden to maintain those lists in the nixpkgs tree, as new headers may be added. Maybe I'm not aware of some common case that this is supposed to catch, but unless there is one, this would be better (IMO) implemented as an external experimental tool first.

@SuperSandro2000 SuperSandro2000 added the 2.status: merge conflict This PR has merge conflicts with the target branch label Jan 18, 2021
@holymonson
Copy link
Contributor Author

Libsystem is assembled by borrowing headers for many apple packages, trying to make up a header stub imitating /Library/Developer/CommandLineTools/usr/include on our local macOS. We are pouring all headers to Libsystem, some are extra and some are missing comparing to the origin macOS, usually we don't care about the extra, but we need to make sure no file got lost after update.

The problem is, we don't know the exact header list and which package a header comes from. For example, NSSystemDirectories.h exists in an old Libc but not in the new one, so we need to borrow it from the old while others from the new. Same as libproc.h, but it could also be found in newer xnu, so we can change the source. However, these information is not on any record.

Apple is not much nice to opensource so we can't get the those headers from a tarball or a tool, and it's risky to update because apple may drop headers (they do!) in new release. But the headers we are using is several years old and it start to block normal use (#101229).

This PR could make it easy to find the missing headers during updating. Since we are assembling Libsystem inside nixpkgs (mainly in the installPhase of Libsystem), I think put the header list here should be proper.

Alternatively, we move all the headers to https://github.com/NixOS/darwin-stubs . (But this will take much more work comparing to this PR and #108590).

@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Jan 19, 2021
@holymonson
Copy link
Contributor Author

cc @matthewbauer

@holymonson holymonson force-pushed the check-headers branch 2 times, most recently from 992f862 to e3748ab Compare January 23, 2021 13:56
@veprbl veprbl added the 2.status: merge conflict This PR has merge conflicts with the target branch label Jan 29, 2021
@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Jan 30, 2021
Copy link
Contributor

@r-burns r-burns left a comment

Choose a reason for hiding this comment

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

Built stdenv, CoreFoundation, and libutil

@veprbl veprbl merged commit f602c10 into NixOS:staging Feb 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.topic: darwin Running or building packages on Darwin 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: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux.

Projects

No open projects
Archived in project

Development

Successfully merging this pull request may close these issues.

5 participants