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.

Sign up for GitHub

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

melpaPackages: match version of magit-popup to magit #33191

Merged
merged 1 commit into from Dec 30, 2017
Merged

melpaPackages: match version of magit-popup to magit #33191

merged 1 commit into from Dec 30, 2017

Conversation

benley
Copy link
Member

@benley benley commented Dec 29, 2017

Related to magit/magit#3286

Motivation for this change

I was getting weird errors trying to use magit features that involved magit-popup

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-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 nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

benley
benley commented Dec 29, 2017
View reviewed changes
pkgs/applications/editors/emacs-modes/melpa-packages.nix Outdated
# version of magit-popup needs to match magit
# https://github.com/magit/magit/issues/3286
magit = super.magit.override {
inherit (self.melpaPackages) magit-popup
Copy link
Member Author

Choose a reason for hiding this comment

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

oops, I see the missing semicolon. Fix coming in a sec.

@benley benley requested a review from ttuegel December 29, 2017 23:43
@lukateras
Copy link
Member

@GrahamcOfBorg eval

@lukateras lukateras merged commit 823e191 into NixOS:master Dec 30, 2017
@ttuegel
Copy link
Member

ttuegel commented Dec 30, 2017

A word of warning: this only works as long as the user doesn't install magit-popup explicitly; if they do that, they will get the incompatible version. Actually, Emacs will try to install both versions simultaneously. That's impossible, so I'm not sure what will happen.

@lukateras
Copy link
Member

I've accepted this because melpaPackages already has similar overrides in a few other places:

nixpkgs/pkgs/applications/editors/emacs-modes/melpa-packages.nix

Line 46 in f0f290e

inherit (self.melpaPackages) powerline;

nixpkgs/pkgs/applications/editors/emacs-modes/melpa-packages.nix

Line 69 in f0f290e

inherit (self.melpaPackages) easy-kill;

nixpkgs/pkgs/applications/editors/emacs-modes/melpa-packages.nix

Line 82 in f0f290e

inherit (self.melpaPackages) ess ctable popup;

@ttuegel
Copy link
Member

ttuegel commented Dec 30, 2017

I've accepted this because melpaPackages already has similar overrides in a few other places:

You were right to; this is the best we can do for now!

@benley
Copy link
Member Author

benley commented Dec 30, 2017

A word of warning: this only works as long as the user doesn't install magit-popup explicitly; if they do that, they will get the incompatible version. Actually, Emacs will try to install both versions simultaneously. That's impossible, so I'm not sure what will happen.

I've experimented with that scenario, and it appears that the older incompatible version wins out. It effectively just negates this workaround, I think.

@benley benley deleted the magit-popup-version branch December 30, 2017 20:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lukateras lukateras lukateras approved these changes

@ttuegel ttuegel Awaiting requested review from ttuegel

Assignees
No one assigned
Labels
10.rebuild-darwin: 0 10.rebuild-linux: 0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@benley @lukateras @ttuegel @GrahamcOfBorg