darwin.libffi: match upstream configuration on x86_64-darwin#383376
darwin.libffi: match upstream configuration on x86_64-darwin#383376reckenrode merged 1 commit intoNixOS:stagingfrom
Conversation
github-actions
bot
added
10.rebuild-darwin: 5001+
There was a problem hiding this comment.
Perhaps we should just install darwin/include wholesale rather than (or in addition to) the upstream headers? It has an ffitarget.h that conditionally includes the header for the various supported platforms and sets things appropriately. That way we don’t have to worry about keeping in sync; I’d worry about other differences in the configuration given that we’ve already had to replace one header and patch another.
There was a problem hiding this comment.
I opted to copy the platform-specific headers into the target config folder created by the configure script. That makes sure they’re consistent with the build, and lets the standard install phase handle copying them into place.
reckenrode
force-pushed
the
push-kqrzruxywypy
branch
from
48c6f13 to
d89016a
Compare
This ensures that the installed headers match those shipped with the system libffi, which may enable or disable certain features depending on the platform (such as the legacy closure API on x86_64-darwin).
reckenrode
force-pushed
the
push-kqrzruxywypy
branch
from
d89016a to
31d699a
Compare
| echo '#define FFI_TRAMPOLINE_WHOLE_DYLIB 1' >> aarch64-apple-darwin/fficonfig.h | ||
| postConfigure = '' | ||
| # Use Apple’s configuration instead of the one generated by the `configure` script. | ||
| cp darwin/include/fficonfig_${stdenv.hostPlatform.darwinArch}.h ${stdenv.hostPlatform.config}/fficonfig.h |
There was a problem hiding this comment.
Is the missing include/ here correct? I guess so, going by https://github.com/apple-oss-distributions/libffi/blob/83edf1120fbb1d9aace9b9eb87ac22198b1bc2e8/darwin/README?
Also, does tramp.h matter? Do we need to move that into place too?
We could potentially run https://github.com/apple-oss-distributions/libffi/blob/83edf1120fbb1d9aace9b9eb87ac22198b1bc2e8/generate-darwin-source-and-headers.py ourselves if it’d make anything easier/cleaner…
There was a problem hiding this comment.
The missing include/ is intentional. The build system for libffi creates a folder for the target platform’s triple then generates <triple>/fficonfig.h, <triple>/include/ffi.h, and <triple>/include/ffitarget.h. I replace those in this derivation with Apple’s generated headers. Notably, fficonfig.h is not installed. It’s only used for building libffi. That’s why it’s not copied to <triple>/include.
Regarding tramp.h, it’s not a generated header. The build system doesn’t even copy it to <triple>. The one included with Apple’s headers is the same as the upstream one.
We could potentially run https://github.com/apple-oss-distributions/libffi/blob/83edf1120fbb1d9aace9b9eb87ac22198b1bc2e8/generate-darwin-source-and-headers.py ourselves if it’d make anything easier/cleaner…
Running generate-darwin-source-and-headers.py would create a circular dependency between libffi and Python. Either python3-minimal would have to be changed to build against libffiReal, the first build of libffi in the bootstrap would have to not run it, or the stdenv bootstrap would need to build another Python just for building darwin.libffi. I don’t think any of those would make the build easier or cleaner.
The cleanest thing to do would be to get the trampoline PR finally merged upstream and work out whatever configure flags are needed to duplicate Apple’s configuration. I’m not exactly familiar with aarch64 assembly, nor do I have a lot of bandwidth, so I’m probably not be the best person to try to do that.
There was a problem hiding this comment.
Thanks. This all seems reasonable to me then.
3 participants
The headers for the system libffi enable the legacy closure API by default, which some packages (such as Python) expect.
Note: I only confirmed that Python built in the bootstrap.
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.