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

macvim: 8.2.319 -> 8.2.539 #85321

Merged
merged 3 commits into from May 3, 2020
Merged

macvim: 8.2.319 -> 8.2.539 #85321

merged 3 commits into from May 3, 2020

Conversation

@lilyball
Copy link
Member

lilyball commented Apr 15, 2020

Motivation for this change

https://github.com/macvim-dev/macvim/releases/tag/snapshot-163

This also includes a fix for compatibility with Xcode 11.4. Xcode 11.4 has an updated sys/_types/_fd_def.h header that references a new symbol from libSystem. This is a problem because we're using /usr/bin/clang to compile the non-Xcode portion, and this
pulls in headers from Xcode's SDK. Somehow it's still linking to the Nix libraries. The end result is we get a linker error where this new symbol can't be found at link time, even though it's a weak import and isn't required at runtime.

Ideally we'd provide a full 10.12 SDK to /usr/bin/clang, but we can't do that because even the DevSDK package we use for our 10.12 SDK doesn't contain everything (in particular it's missing nearly all dylibs) so we just get linker errors if we do that.

Instead we'll just strip the LDFLAGS post-configure so it no longer references the Nix versions of OS libraries. This means we're not linking against Nix libSystem anymore, and that fixes the link error. This also prevents this issue from cropping up again in the future.

Things done
  • 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.
lilyball added 2 commits Apr 15, 2020
Xcode 11.4 has an updated sys/_types/_fd_def.h header that references a
new symbol from libSystem. This is a problem because we're using
`/usr/bin/clang` to compile the non-Xcode portion, and this pulls in
headers from Xcode's SDK. Somehow it's still linking to the Nix
libraries (I can't figure out where configure finds these to put into
`LDFLAGS` as we're not using the cc-wrapper). The end result is we get a
linker error where this new symbol can't be found at link time, even
though it's a weak import and isn't required at runtime.

Ideally we'd provide a full 10.12 SDK to `/usr/bin/clang`, but we can't
do that because even the DevSDK package we use for our 10.12 SDK doesn't
contain everything (in particular it's missing nearly all dylibs) so we
just get linker errors if we do that.

Instead we'll just do a horrible hack and provide an `-isystem` path to
a folder structure that contains only the 10.12 `sys/_types/_fd_def.h`
header. This avoids the new symbol without causing all the errors that
happen if we pull in the entire `${darwin.Libsystem}/include`.
@lilyball
Copy link
Member Author

lilyball commented Apr 15, 2020

I took another look at the built product and the Vim binary links against Nix libSystem but the MacVim binary (compiled with Xcode) is linking against the OS-provided libraries. And I guess it's been this way all along.

I don't know if we should just give up and figure out how to make Vim use the system libSystem, or if we should fixup MacVim to link against Nix libSystem, or just ignore it since it doesn't really seem to matter.

Eventully we should look into using xcbuild to compile this stuff instead, now that we're no longer invoking ibtool, but I don't know if it will work.

@lilyball
Copy link
Member Author

lilyball commented Apr 16, 2020

I dug into xcbuild and I'm getting an opaque failure in actool. I don't know what's wrong, and xcbuild doesn't really seem to be actively developed anymore.

@lilyball
Copy link
Member Author

lilyball commented Apr 19, 2020

Since xcbuild has too many issues to use, I went ahead and pushed a commit that gets rid of the fake libSystem header and instead just strips all the Nix linker paths for OS libraries out of LDFLAGS. This means we're doubling down on using Xcode's SDK to compile, but that's what we've mostly been doing this whole time, and this should prevent future Xcode versions from breaking the package again.

@nixos-discourse
Copy link

nixos-discourse commented Apr 23, 2020

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

https://discourse.nixos.org/t/prs-ready-for-review-may-2019/3032/146

MacVim compiles the Vim part using `/usr/bin/clang` and the GUI part
using Xcode. The Xcode portion always uses Xcode's own SDK and we have
no workable alternative. The Vim portion so far has been compiling using
a hybrid compilation environment, where it uses the SDK for most stuff
but picks up a bunch of library linker paths (including libSystem) by
virtue of Ruby's LDFLAGS. This hybrid compilation environment meant that
if the SDK headers referenced a symbol that the library itself didn't
have, this could produce link errors.

Previously we attempted to fix this by synthesizing an include path that
contained just the one header from Nix's Libsystem that referenced the
missing symbol, to get rid of the reference and allow linking to work
again, but this was very hacky and runs the risk of future Xcode SDK
changes producing the same errors with different headers, or of future
SDK versions expecting the intercepted header to contain a definition
that Nix's doesn't.

This new approach is to just clean up the compilation environment such
that the Vim portion is compiling against the Xcode SDK as well, by
sanitizing the LDFLAGS produced by the configure script so it stops
referencing Nix's versions of OS libraries. This means the resulting Vim
binary no longer depends at runtime on Nix for anything except the
scripting language support, but that's how it's been for the MacVim
binary all along anyway, and this approach should keep us insulated
against future Xcode SDK changes.
@lilyball lilyball force-pushed the lilyball:macvim branch from 8f1b934 to ee28064 Apr 28, 2020
@lilyball
Copy link
Member Author

lilyball commented Apr 28, 2020

I changed the postConfigure error into a warning instead.

@nixos-discourse
Copy link

nixos-discourse commented Apr 30, 2020

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

https://discourse.nixos.org/t/prs-ready-for-review-may-2019/3032/152

@veprbl veprbl merged commit 20eff68 into NixOS:master May 3, 2020
13 checks passed
13 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ee28064"; rev="ee28064389194bb614e5d34118e8fd51aa0f7bf9"; } ./pkgs/t
Details
grahamcofborg-eval-lib-tests nix-build --arg pkgs import ./. {} ./lib/tests/release.nix
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ee28064"; rev="ee28064389194bb614e5d34118e8fd51aa0f7bf9"; } ./nixos/
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ee28064"; rev="ee28064389194bb614e5d34118e8fd51aa0f7bf9"; } ./nixos/
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ee28064"; rev="ee28064389194bb614e5d34118e8fd51aa0f7bf9"; } ./nixos/
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ee28064"; rev="ee28064389194bb614e5d34118e8fd51aa0f7bf9"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ee28064"; rev="ee28064389194bb614e5d34118e8fd51aa0f7bf9"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ee28064"; rev="ee28064389194bb614e5d34118e8fd51aa0f7bf9"; } ./pkgs/t
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
@veprbl
Copy link
Member

veprbl commented May 3, 2020

@lilyball Sorry, can't really review this. Merging assuming this is working well for you.

@lilyball lilyball deleted the lilyball:macvim branch May 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.