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

Jump to bottom

macvim: 7.4.909 -> 8.1.1517 #62934

Merged
merged 1 commit into from Jun 16, 2019
Merged

macvim: 7.4.909 -> 8.1.1517 #62934

merged 1 commit into from Jun 16, 2019

Conversation

lilyball
Copy link
Member

Motivation for this change

Fix up the macvim package to build again, with the latest snapshot. The patchfile has been recreated by manually reapplying all of the changes from the old patchfile, and the other changes in here were figured out by trial and error (such as the need to unset LD).

Also tweak the package to use python37 by default, and add an option to go back to python27 if desired. I'm not actually sure what the most common python configuration is these days, so this was chosen mostly because python3 is supposed to be the future.

Disable Sparkle so the user isn't prompted to update a readonly package.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 nix-review --run "nix-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)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

This build is impure. It relies on accessing a few tools from /usr/bin (including xcodebuild), and it sets the minimum deployment target to the current OS version. I tried patching the latter but that resulted in a bunch of warnings during the build process, so I left it alone. This impurity exists for the current broken package too, of course.

CC @cstrahan

@ofborg ofborg bot added the 6.topic: vim label Jun 10, 2019
@lilyball
Copy link
Member Author

I tested some basic functionality, such as :py3 print("hello") and :ruby print("hello"). I also verified that it builds with usePython27 set (and that :py print("hello") works there). I was somewhat tempted to just enable both python2 and python3 simultaneously, which apparently works with some limitations, but decided it was better not to.

@ofborg ofborg bot requested a review from cstrahan June 10, 2019 06:47
@lilyball lilyball force-pushed the macvim branch 2 times, most recently from 86298b2 to 559a67f Compare June 11, 2019 18:16
@lilyball lilyball changed the title macvim: 7.4.909 -> 8.1.950 macvim: 7.4.909 -> 8.1.1517 Jun 11, 2019
@lilyball
Copy link
Member Author

Updated to the just-released snapshot-156 which includes a fix for the recent Vim CVE (relating to modelines).

@lilyball
macvim: 7.4.909 -> 8.1.1517
95bfb99 
Fix up the macvim package to build again, with the latest snapshot. The
patchfile has been recreated by manually reapplying all of the changes
from the old patchfile, and the other changes in here were figured out
by trial and error (such as the need to unset `LD`).

Also tweak the package to use python37 by default, and add an option to
go back to python27 if desired.

Disable Sparkle so the user isn't prompted to update a readonly package.
@lilyball
Copy link
Member Author

I've also gone ahead and added myself as a maintainer.

@nixos-discourse
Copy link

This pull request has been mentioned on Nix community. There might be relevant details there:

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

@matthewbauer matthewbauer merged commit 486626b into NixOS:master Jun 16, 2019
matthewbauer
matthewbauer reviewed Jun 16, 2019
View reviewed changes
pkgs/applications/editors/vim/macvim.nix
# Full path to xcodebuild
substituteInPlace src/Makefile --replace "xcodebuild" "/usr/bin/xcodebuild"
for nib in MainMenu Preferences; do
/usr/bin/ibtool --compile src/MacVim/English.lproj/$nib.nib/keyedobjects.nib src/MacVim/English.lproj/$nib.nib
Copy link
Member

Choose a reason for hiding this comment

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

This could cause issues when /usr/bin/ibtool doesn't exist. Probably we should follow the Hydra builds to see. I think a darwin.ibtool might be useful to give a nice error message when this doesn't exist.

Copy link
Member Author

Choose a reason for hiding this comment

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

What version of macOS does Hydra run on, and what's the minimum version of macOS that Nix supports? /usr/bin/ibtool should exist on any version of macOS from the past few years at least. I suppose this could be /usr/bin/xcrun ibtool instead if we need to support a version of macOS that predates the introduction of the full set of build tools into /usr/bin.

@lilyball lilyball deleted the macvim branch June 16, 2019 22:10
@lilyball lilyball mentioned this pull request Jul 15, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matthewbauer matthewbauer matthewbauer left review comments

@cstrahan cstrahan Awaiting requested review from cstrahan

Assignees
No one assigned
Labels
1.severity: security 6.topic: vim 10.rebuild-darwin: 1-10 10.rebuild-linux: 0 11.by: package-maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@lilyball @nixos-discourse @matthewbauer @samueldr