d12frosted / homebrew-emacs-plus

Emacs Plus formulae for the Homebrew package manager
MIT License
2.24k stars 175 forks source link

Initial implementation of `--native-compilation` options, enabling AOT or disabling native compilation #667

Closed stradicat closed 11 hours ago

stradicat commented 4 months ago

I've implemented and tested a rudimentary way of exposing --native-compilation=aot to compile Emacs' .elc files at build time, as previously asked in https://github.com/d12frosted/homebrew-emacs-plus/issues/556#issuecomment-1565423124, by passing the new --with-native-comp-aot argument I've inserted in the formulae.

I've probably repeated myself in Ruby, as I'm not a Ruby dev, and it could surely welcome some deduplication in further commits.

However, AOT compilation works.

d12frosted commented 4 months ago

Ah, it would be also nice to consider https://github.com/d12frosted/homebrew-emacs-plus/issues/635 while working on AOT.

This sounds like there are 3 options that Emacs can work with: no native compilation, native compilation, native compilation with AOT.

Homebrew only allows flags as options, meaning that we can't easily encode 3 states :) We can go with one opt out option called --without-native-comp and opt-in/opt-out --with[out]-native-comp-aot. Enabling second one requires the first one enabled (i.e. use odie).

stradicat commented 4 months ago

Good point! I understand why some users may still want Emacs' plain elisp byte-compile to .elc without native compilation (e.g. myself on my i386 Pentium with 2Gb of RAM), to avoid the overhead.

I'll take the opportunity to include such an option as well, while I'm at it. (I'll modify the PR title)

(And I'll also brush up on Ruby, to avoid this commit's duplication pitfalls in the near future, if were to come up with new changes)

devcexx commented 4 months ago

Hi,

I just saw this PR and tried locally for building my own Emacs 29 installation. However, just after building it I got this error when I tried to run emacs --version:

Error using execdir /usr/local/Cellar/emacs-plus@29/29.2/Emacs.app/Contents/MacOS/:
emacs: dlopen(/usr/local/Cellar/emacs-plus@29/29.2/Emacs.app/Contents/MacOS/../native-lisp/29.2-f364439d/preloaded/register-80045398-3789d1c4.eln, 0x0001): tried: '/usr/local/Cellar/emacs-plus@29/29.2/Emacs.app/Contents/MacOS/../native-lisp/29.2-f364439d/preloaded/register-80045398-3789d1c4.eln' (no such file), '/System/Volumes/Preboot/Cryptexes/OS/usr/local/Cellar/emacs-plus@29/29.2/Emacs.app/Contents/MacOS/../native-lisp/29.2-f364439d/preloaded/register-80045398-3789d1c4.eln' (no such file), '/usr/local/Cellar/emacs-plus@29/29.2/Emacs.app/Contents/MacOS/../native-lisp/29.2-f364439d/preloaded/register-80045398-3789d1c4.eln' (no such file)

I built it with the following command:

git clone https://github.com/stradicat/homebrew-emacs-plus/
cd homebrew-emacs-plus
brew install --build-from-source Formula/emacs-plus@29.rb --with-native-comp-aot --with-modern-black-variant-icon

For fixing the error I needed to manually copy the prebuilt eln files into the .app bundle by just doing:

cp -r /usr/local/Cellar/emacs-plus@29/29.2/lib/emacs/29.2/native-lisp /usr/local/Cellar/emacs-plus@29/29.2/Emacs.app/Contents/

Maybe some extra changes in the Formula needs to be done in order to copy that folder to the .app bundle? Or did I do something wrong during the build process?

stradicat commented 4 months ago

Thanks for bringing it up! I'll test again in the same way you did.

stradicat commented 4 months ago

My Mac just died yesterday, so please hold this PR until I can repair the machine and continue testing to ensure that it works 100% despite having passed all checks.

stradicat commented 4 months ago

Hm, I had saved a warning I got last time:

Error: Failed changing dylib ID of /usr/local/Cellar/emacs-plus@29/29.2/Emacs.app/Contents/native-lisp/29.2-8ccb7056/c-ts-mode-a80983a9-e27a6682.eln
  from c-ts-mode-a80983a9-e27a6682.eln
    to /usr/local/opt/emacs-plus@29/Emacs.app/Contents/native-lisp/29.2-8ccb7056/c-ts-mode-a80983a9-e27a6682.eln

Error: Failed to fix install linkage

The formula built, but you may encounter issues using it or linking other
formulae against it.

This looks odd.

Edit: Seems like it was a thing

stradicat commented 3 months ago

Alright, got the machine running again.

Regarding

Error: Failed changing dylib ID of /usr/local/Cellar/emacs-plus@29/29.2/Emacs.app/Contents/native-lisp/29.2-8ccb7056/c-ts-mode-a80983a9-e27a6682.eln
  from c-ts-mode-a80983a9-e27a6682.eln
    to /usr/local/opt/emacs-plus@29/Emacs.app/Contents/native-lisp/29.2-8ccb7056/c-ts-mode-a80983a9-e27a6682.eln
Error: Failed to fix install linkage
The formula built, but you may encounter issues using it or linking other
formulae against it.

I tried copying /usr/local/Cellar/emacs-plus@29/29.2/Emacs.app/Contents/native-lisp/29.2-8ccb7056/c-ts-mode-a80983a9-e27a6682.eln to /usr/local/opt/emacs-plus@29/Emacs.app/Contents/native-lisp/29.2-8ccb7056/, but the file was already there ([files] are identical (not copied)).

Therefore, the warnings about missing .eln files may be a false positive. However, I agree that this may look confusing.

stradicat commented 3 months ago

Thanks a lot for the PR.

I wonder - does it even make sense to have non-aot native compilation?

Just came across across this comment by Eli Zaretskii (GNU Emacs maintainer):

"Why are you compiling all of your Lisp files ahead of time? That is not how native compilation is supposed to be used. You are supposed to let Emacs compile each file in the background if and when it is loaded. This way, you don't need to wait for the compilation, and also, you never compile files you don't use."

The man knows his beast. Perhaps I should just close this PR and avoid the hassle.

devcexx commented 3 months ago

In my particular case, I used this PR for building my emacs installation because I was having huge performance issues while the native compilation was happening asynchronously. For me it was just a way of offloading this extra process that was impacting my emacs usage to the build time.

Anyway at the end of the day the good thing about this PR is that it adds an optional feature anyway, it won't impact users that doesn't use it. I see it as a nice to have.

stradicat commented 3 months ago

In that case, I'll do some more testing.