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.Libsystem: add missing modulemaps #186251

Merged
merged 1 commit into from
Aug 13, 2022

Conversation

stephank
Copy link
Contributor

Description of changes

These define the Darwin module, which I don't think is really used in ObjC, but is the primary means with which Swift code interacts with low-level libsystem API.

I've done a stdenv rebuild on aarch64-darwin and confirmed my build issue goes away.

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 6.topic: darwin Running or building packages on Darwin 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 Aug 12, 2022
@stephank
Copy link
Contributor Author

cc @NixOS/darwin-maintainers, don't know who to tag for this. 🙃

@viraptor
Copy link
Contributor

This makes sense to me. I learned what the modules/modulemaps do from here: https://clang.llvm.org/docs/Modules.html

Since the extra files are copied from the sdk, I wonder if a separate package would provide the same benefits and avoid a rebuild. On the other hand, we're probably not too many years away from something using the same modules from C, so being in the default package makes sense.

I'll try to give it a go on Intel tomorrow.

@uri-canva
Copy link
Contributor

This looks good to me at a high level, and I think it's better to change existing libSystem than introducing a new one. Let's not make the interface to darwin more complicated to save a rebuild, the cost of a rebuild is one and done, but the extra complexity will harm both maintainers and casual users for a long time going forward.

I have two high level questions which I'd like confirmation on, but if no one knows then I'm happy to go ahead based on my assumptions:

  1. There's a couple of extra modulemaps here and there, for example include/AppleArchive/module.modulemap,
    include/CommonCrypto/module.modulemap,
    include/EndpointSecurity/module.modulemap, I see some are being copied, and some aren't. Since the logic is in sync with the headers, I assume those are already the correct set of include/ subdirectories.
  2. Did you confirm all the symbols declared in the module maps are defined in the libraries we are copying? I assume since we're copying the modulemaps that correspond to the headers we've already been copying and using everything is ok.

@stephank
Copy link
Contributor Author

stephank commented Aug 13, 2022

There's a couple of extra modulemaps here and there, for example include/AppleArchive/module.modulemap,
include/CommonCrypto/module.modulemap,
include/EndpointSecurity/module.modulemap, I see some are being copied, and some aren't. Since the logic is in sync with the headers, I assume those are already the correct set of include/ subdirectories.

Those subdirectories are copied in full, so I assume they're already fine. (May also be stuff that is already used from ObjC.)

Did you confirm all the symbols declared in the module maps are defined in the libraries we are copying? I assume since we're copying the modulemaps that correspond to the headers we've already been copying and using everything is ok.

The modulemaps don't declare symbols, they reference header files. If any of the header files reference symbols elsewhere, that'd already be a problem, I think?

But what I found is that not all header files referenced are present. I came up with this cludge:

$ awk '/header/ { for (i=1; i<=NF; ++i) { if ($i ~ "\"") { gsub(/"/, "", $(i)); print $(i) } } }' *.modulemap \
  | xargs ls 2>&1 | grep 'No such file'
ls: _structs.h: No such file or directory
ls: krb5/krb5.h: No such file or directory
ls: krb5/locate_plugin.h: No such file or directory
ls: krb5/preauth_plugin.h: No such file or directory
ls: stdarg.h: No such file or directory
ls: stdatomic.h: No such file or directory
ls: stdbool.h: No such file or directory

All of these issues come from module.modulemap:

  • The _structs.h is an exclude header in module Darwin, so I'm not sure if that's an issue.

  • The krb5/* headers are part of module krb5, so at least my testing with module Darwin wouldn't have picked up any issues with this.

  • The missing std*.h headers are part of module Darwin and listed as "supplied by the compiler". Curiously, other headers that have that note are present.

I think the krb5 one is interesting. There are several modules like this, like SQLite, zlib, etc, and headers are already present in our libsystem, but we package all of those separately. So we are already packaging headers here for which symbols are not present.

@uri-canva
Copy link
Contributor

I see, so we should be changing a couple of derivations here so that the headers are together with their respective libraries, but it doesn't seem to cause any issues so far. Let's merge this as is, and fix up the headers and modulemaps being in the wrong derivation later.

@uri-canva uri-canva self-requested a review August 13, 2022 23:01
@uri-canva
Copy link
Contributor

@viraptor I've tried to build this myself but it takes way too much disk space. It should be pretty safe since it only adds files, it doesn't modify them or remove them, so I'll go ahead and merge, but if you manage to build it after all leave a comment, we can always revert if there's issues.

@uri-canva uri-canva merged commit 1681ca4 into NixOS:staging Aug 13, 2022
@viraptor
Copy link
Contributor

Yeah, that makes sense. The change should be very safe.

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 10.rebuild-darwin: 501+ 10.rebuild-darwin: 1001-2500 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants