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

Support specifying extra emacs pkgs from nixpkgs #12

Merged
merged 8 commits into from
Jun 6, 2024
Merged

Support specifying extra emacs pkgs from nixpkgs #12

merged 8 commits into from
Jun 6, 2024

Conversation

schwanberger
Copy link
Contributor

@schwanberger schwanberger commented Jun 1, 2024

The following checks succeed:

  • nix build .\#checks.x86_64-linux.minimalExtraPackages
    • (NEW) should likely be tweaked with a call to elisp list-packages to verify that vterm is external. What do you think?
  • nix build .\#checks.x86_64-linux.minimal

I have also bumped my local doom-emacs using my config and the changes in this fork and can anecdotally attest that it works as expected for my (yet undocumented) use-case.

*EDIT: Updating checkbox status.

default.nix Outdated Show resolved Hide resolved
home-manager.nix Outdated
Comment on lines 106 to 108
To let nix handle a doom dependency '(package! ...)' we can leverage the ':built-in t' argument
Consider the following example for 'vterm' in the doom config packages.el:
(package! vterm :built-in t)
Copy link
Owner

Choose a reason for hiding this comment

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

Using vterm as an example is a little strange: Unstraightened should already be installing epkgs.vterm if you enable the vterm module. It'll be the pinned version, but if you want to unpin it I'd argue (unpin! vterm) in packages.el makes more sense than :built-in t + extraPackages.

Your treesit-grammars.with-all-grammars example makes sense because you can't express that using (package!), but since Doom+Unstraightened aren't trying to install that one you don't need the :built-in t either.

You'd really only need :built-in t to stop Unstraightened from installing something it would normally install, if you want to further customize it in a way that cannot be expressed in packages.el. I can't think of great examples off the top of my head, but applying patches to an ELisp package would be one. Can you tweak the wording a bit to make it more clear :built-in t is only needed when replacing packages, not adding extra ones?

I'd also drop epkgs.vterm from the example as I think it's less realistic.

Copy link
Contributor Author

@schwanberger schwanberger Jun 2, 2024

Choose a reason for hiding this comment

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

I've updated the description to be more precise. See cdd7af0

@marienz
Copy link
Owner

marienz commented Jun 2, 2024

should likely be tweaked with a call to elisp list-packages to verify that vterm is external. What do you think?

I had the same thought. Rather than list-packages, I'd pick a package that isn't built-in (and isn't likely to become a dependency of Doom's core anytime soon... vterm would do fine for test purposes, even if it doesn't make as much sense in documentation because in normal use you'd just enable Doom's vterm module) and (require 'that-package) from the test. No need to duplicate the "not built-in" half of test-external-org for this.

@schwanberger
Copy link
Contributor Author

schwanberger commented Jun 2, 2024

should likely be tweaked with a call to elisp list-packages to verify that vterm is external. What do you think?

I had the same thought. Rather than list-packages, I'd pick a package that isn't built-in (and isn't likely to become a dependency of Doom's core anytime soon... vterm would do fine for test purposes, even if it doesn't make as much sense in documentation because in normal use you'd just enable Doom's vterm module) and (require 'that-package) from the test. No need to duplicate the "not built-in" half of test-external-org for this.

I've implemented a doomTest "extraPackages" that succeeds if extraPackages = epkgs: [ epkgs.vterm ]; and fails otherwise.
See 4df8b7b

Make it explicit that is now possible to use the home-manager option `extraPackages`
Eluding to likely upcoming support for native TS,
at least as an opt-in and eluding to the issue for unstraightened
not likely to be chased any further.
This is in line with the discussion in the issue..
@schwanberger
Copy link
Contributor Author

@marienz Seeing as my todo list for the PR is now complete I was wondering if you have any comments or new changes that you'd like. Please feel free to change/delete any changes I've made to README.md without soliciting an opinion from me 👍

@marienz
Copy link
Owner

marienz commented Jun 6, 2024

Looks great! Thanks for including comprehensive documentation.

@marienz marienz merged commit 5cd1844 into marienz:main Jun 6, 2024
2 checks passed
@schwanberger schwanberger deleted the feature/extra_packages branch June 8, 2024 10:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants