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

Fix color-theme-solarized #68768

Merged
merged 1 commit into from Nov 17, 2019
Merged

Fix color-theme-solarized #68768

merged 1 commit into from Nov 17, 2019

Conversation

samuelrivas
Copy link
Contributor

@samuelrivas samuelrivas commented Sep 14, 2019 •
edited

I suspect there is something wrong with the elpa package generator. Emacs'
setup-hook adds the site-lisp directory to load-path, but elpa packages end
up having their code in `site-lisp/elpa/package-version, and thus emacs cannot
load them.

If that worked, adding color-theme as builInput might be better than
explicitly adding its site-lisp to the load-path in the buildPhase

Motivation for this change
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)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Notify maintainers

cc @

@samuelrivas samuelrivas force-pushed the fix-color-theme-solarized branch 2 times, most recently from 6441663 to d97a7a5 Compare September 14, 2019 05:35
@samuelrivas
color-theme-solarized: update and fix
fdb00fd 
I suspect there is something wrong with the elpa package generator. Emacs'
setup-hook adds the `site-lisp` directory to `load-path`, but elpa packages end
up having their code in `site-lisp/elpa/package-version, and thus emacs cannot
load them.

If that worked, adding `color-theme` as `builInput` might be better than
explicitly adding its `site-lisp` to the `load-path` in the `buildPhase`
@samuelrivas samuelrivas mentioned this pull request Sep 16, 2019
Merged
10 tasks
@Lassulus
Copy link
Member

hmm, how to test something like this? nix-review says no changes :D

@samuelrivas
Copy link
Contributor Author

What is nix-review supposed to check? There are changes in the PR at least :)

@samuelrivas
Copy link
Contributor Author

The way I use this is adding it to emacsWithPackages and then loading it like here:

https://github.com/samuelrivas/monorepo/blob/526fa7cb14b9d9f09112b3e28f238640ac53bdc8/src/elisp/emacs-config/src/emacs-config.el#L20

It is dirty, but this PR is just to preserve previous behaviour. The package broke because the new way of creating packages for melpa puts elisp code in site-elisp/elpa/<package> and this derivation was assuming that they would be in site-elisp.

I think there might be something wrong with the setup hook as it doesn't seem that elisp code is properly accessible even when you add the libraries as build dependencies, but I haven't had the time to dig into this to write a proper package for this color theme

@samuelrivas
Copy link
Contributor Author

So what is the state of this PR? Can anyone reject or approve it?

Lassulus
Lassulus approved these changes Nov 17, 2019
View reviewed changes
Copy link
Member

@Lassulus Lassulus left a comment

Choose a reason for hiding this comment

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

LGTM

@Lassulus Lassulus merged commit 0b504bc into NixOS:master Nov 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Lassulus Lassulus Lassulus approved these changes

@adisbladis adisbladis Awaiting requested review from adisbladis

Assignees
No one assigned
Labels
6.topic: emacs 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
2 participants
@samuelrivas @Lassulus