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

mu: enable AOT native-comp for emacs lisp #194281

Merged
merged 1 commit into from
Apr 19, 2023

Conversation

jian-lin
Copy link
Contributor

@jian-lin jian-lin commented Oct 3, 2022

Description of changes
Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 22.11 Release Notes (or backporting 22.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@peterhoeg
Copy link
Member

peterhoeg commented Oct 4, 2022 via email

@jian-lin
Copy link
Contributor Author

jian-lin commented Oct 4, 2022

any chance we can move this out into a separate hook and just call that?

Good idea, I will wait for #194279 to be merged first.

I’m sure there are other emacs packages that would benefit from this.

Are there? To the best of my knowledge, mu is the only package. Could you list a few?

@jian-lin
Copy link
Contributor Author

jian-lin commented Oct 5, 2022

Are there?

Indeed, there are many packages like mu4e.

rg emacs -t nix --glob '!pkgs/applications/editors/emacs' --glob '
!pkgs/build-support/emacs' --glob '!nixos' --glob '!test'

I am working on it.

@peterhoeg
Copy link
Member

Even our regular emacs packages (under emacsPackages) require nativecomp so they should simply use the same hook/helper.

@jian-lin
Copy link
Contributor Author

jian-lin commented Oct 6, 2022

Even our regular emacs packages (under emacsPackages) require nativecomp

They have infra for native comp since a7cb8e3, not a hook though.

they should simply use the same hook/helper

Yeah, that is also a good idea.

I will mark this as draft to prevent it from being merged.

@jian-lin jian-lin marked this pull request as draft October 6, 2022 02:28
@jian-lin jian-lin force-pushed the mu4e-enable-native-comp branch from de8ce54 to c11682b Compare October 9, 2022 21:03
@jian-lin jian-lin marked this pull request as ready for review October 9, 2022 21:04
@jian-lin
Copy link
Contributor Author

jian-lin commented Oct 9, 2022

I give up making a setup hook for native-comp because I cannot figure out how to put everything needed by native-comp into a hook, e.g. these lines. I will leave that for future contributors.

I make native-comp conditionally depend on emacs.nativeComp.

I suggest we merge this for now and transfer it to a hook when the hook is implemented.

@jian-lin jian-lin force-pushed the mu4e-enable-native-comp branch from c11682b to 15ebd7c Compare October 9, 2022 21:11
@jian-lin
Copy link
Contributor Author

Any updates?

@chvp
Copy link
Member

chvp commented Oct 19, 2022

I give up making a setup hook for native-comp because I cannot figure out how to put everything needed by native-comp into a hook, e.g. these lines. I will leave that for future contributors.

Is this not already solved in a general way by the current emacs infrastructure/emacs-overlay. All my emacs packages (except mu4e) are AOT-compiled already, and I can't imagine someone added AOT compilation to all those packages seperately.

@jian-lin
Copy link
Contributor Author

jian-lin commented Oct 19, 2022

Is this not already solved in a general way by the current emacs infrastructure/emacs-overlay.

No and emacs-overlay is not involved here.

All my emacs packages (except mu4e) are AOT-compiled already

Let me explain some details.

Currently, only elisp packages, which are built using elpaBuild, melpaBuild or trivialBuild, enable AOT native-comp after my pr. This is because only these three builders run native-comp in the postInstall. As a result, mu4e does not do AOT native-comp.

I can't imagine someone added AOT compilation to all those packages seperately.

Yeah, the infra of emacs needs some improvement to do it the right way. One possible way is to make native-comp a hook, as @peterhoeg says.

Unfortunately, it seems few people care about native-comp in emacs. Almost all recent native-comp related prs are all from me. As I said, I tried to make native-comp a hook, but failed. I would appreciate it if someone could improve the emacs infra. mu4e is the last piece I care for native-comp, so here is this pr.

@jian-lin
Copy link
Contributor Author

It seems this pr does not reach the bar for maintainers to merge it and there is also nothing more I can do. Closing.

@jian-lin jian-lin closed this Nov 29, 2022
@jian-lin jian-lin deleted the mu4e-enable-native-comp branch November 29, 2022 14:36
@peterhoeg
Copy link
Member

I am truly sorry to leave you hanging like this. I absolutely think there is value in your work - I just haven't had tiem to do anything about it.

@jian-lin jian-lin restored the mu4e-enable-native-comp branch November 30, 2022 11:44
@jian-lin
Copy link
Contributor Author

Oh, I misunderstood. Let's reopen it and give it some more time.

@jian-lin jian-lin reopened this Nov 30, 2022
@LiGoldragon
Copy link

Unless emacs *absolutely requires' the elisp files, the nativeComp outputs should go in their own derivations. Lean emacs deploys. Easier to think about and modularize. Just a thought. I might implement something and will let you know if I do.

@peterhoeg
Copy link
Member

peterhoeg commented Jan 27, 2023 via email

@bhankas
Copy link
Contributor

bhankas commented Apr 4, 2023

Just so it doesn't get forgotten to sands of time, @peterhoeg can we do something about this? Anybody else who might be able to review if you're busy?

@peterhoeg
Copy link
Member

peterhoeg commented Apr 4, 2023 via email

@LiGoldragon
Copy link

In principle that makes sense, but is there really a usecase for emacs without the natively compiles files?

I was thinking more about emacs without the elisp files

@bhankas
Copy link
Contributor

bhankas commented Apr 19, 2023

ping @peterhoeg ! Apologies if I'm being annoying, but I'd quite like to see this merged. If Peter is busy, can @adisbladis take a look?

@adisbladis adisbladis merged commit 8af8e36 into NixOS:master Apr 19, 2023
@jian-lin jian-lin deleted the mu4e-enable-native-comp branch April 19, 2023 10:07
@adisbladis
Copy link
Member

While it would indeed be better to solve in a hook or other central location it's OK to have a workaround until then.

@jian-lin
Copy link
Contributor Author

jian-lin commented Sep 5, 2023

FYI, I made a follow-up PR to package mu4e as a separate Emacs lisp package.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants