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

modules/performance: add byteCompileLua option #1887

Merged
merged 4 commits into from
Jul 31, 2024

Conversation

stasjok
Copy link
Contributor

@stasjok stasjok commented Jul 18, 2024

This PR implements a second item from this comment.

It can be merged only on top of #1886.

It introduces an option performance.byteCompileLua, that, when enabled, byte compiles lua files. It can be configured what to compile. By default it's user configs. But also plugins and neovim itself can be compiled.

@stasjok
Copy link
Contributor Author

stasjok commented Jul 19, 2024

It looks like a lot of code that I traced to find any parts generating config files would be changed by #1889. Only three last commits beginning with "modules/performance: add byteCompileLua option" is related to this PR, but it'll definitely will conflict (as is the second my PR). So I guess I'll have to re-implement things.

wrappers/_shared.nix Outdated Show resolved Hide resolved
wrappers/_shared.nix Outdated Show resolved Hide resolved
@stasjok stasjok force-pushed the byte-compile-lua-pr branch 2 times, most recently from 9cd6533 to 3caca7c Compare July 25, 2024 16:03
@stasjok
Copy link
Contributor Author

stasjok commented Jul 25, 2024

Rebased and changed implementation for byte compiling extraFiles. Other implementations stayed as it was (in particularly init.lua).

I hope this time it's alright, because it'll be frustrating if it needs to be rewritten again.

Copy link
Member

@traxys traxys left a comment

Choose a reason for hiding this comment

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

A few question, but I'm mostly ok with the code

lib/builders.nix Show resolved Hide resolved
modules/files.nix Outdated Show resolved Hide resolved
@MattSturgeon
Copy link
Member

MattSturgeon commented Jul 25, 2024

> Running test for default
> ERROR: Error detected while processing /nix/store/21ahy9d40fhvzijkdvdascnz60d4py71-nvim-config/plugin/test.lua:
> E5113: Error while calling lua chunk: ...y9d40fhvzijkdvdascnz60d4py71-nvim-config/plugin/test.lua:16: plugin/file_text.lua is expected to be byte compiled, but it's not

file_text.lua is expected to be byte compiled, but it's not

Looks like it only affects Darwin

modules/files.nix Outdated Show resolved Hide resolved
modules/performance.nix Outdated Show resolved Hide resolved
@stasjok stasjok force-pushed the byte-compile-lua-pr branch 2 times, most recently from 0bae1ff to 0ba8d21 Compare July 26, 2024 09:47
Copy link
Member

@GaetanLepage GaetanLepage left a comment

Choose a reason for hiding this comment

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

Sorry for the late review.
I had an overall look at the PR and trust @MattSturgeon and @traxys for checking the details.

Thank you very much @stasjok for this amazing work. Also the effort you have put in documentation and testing is truly appreciable !

@stasjok
Copy link
Contributor Author

stasjok commented Jul 31, 2024

Will it get merged?

@GaetanLepage
Copy link
Member

GaetanLepage commented Jul 31, 2024

Sure ! Let's give this a shot. Thanks for the hard work !

@Mergifyio queue

@GaetanLepage

This comment was marked as outdated.

This comment was marked as outdated.

@MattSturgeon
Copy link
Member

(I'm going to re-queue this so it merges after #1956, since that PR doesn't need rebasing)

@MattSturgeon

This comment was marked as outdated.

This comment was marked as outdated.

@MattSturgeon

This comment was marked as outdated.

Copy link
Contributor

mergify bot commented Jul 31, 2024

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at 9314cd4

* add `performance.byteCompileLua.enable` toggle to enable or disable
  all byte compiling features
* add `performance.byteCompileLua.initLua` toggle to enable or
  disable byte compiling of init.lua
* add `writeByteCompiledLua` helper for saving byte compiled lua source
  code to the nix store
* `nixvim-print-init` utility is always pointed to uncompiled init.lua
* add tests
This commit adds support for byte compiling lua configuration files.
It's enabled by default (if byte compiling is enabled at all) and can be
disabled with `performance.byteCompileLua.configs` toggle.

To implement this feature `extraFiles.<name>.finalSource` internal
read-only option is introduced. `source` option cannot be used because
it's user configurable. In order to access the values of the
`performance.byteCompileLua` options, parent config is added to
specialArgs of extraFiles submodule. Then the usages of `source` option
changed to `finalSource` in all relevant places (filesPlugin and
wrappers).

Added more helpers for various cases of byte compiling:

* `byteCompileLuaFile` byte compiles lua file
* `byteCompileLuaHook` is a setup hook that byte compiles all lua files
* `byteCompileLuaDrv` overrides derivation by adding byteCompileLuaHook
  to it

Added tests to validate that extraFiles specified by various methods are
handled correctly. Added a separate home-manager test, that is intended
to validate that extraFiles propagated to wrapper modules are correctly
byte compiled.
This commit adds `performance.byteCompileLua.plugins` toggle that, if
enabled, byte compiles all lua files in plugins
This commit adds `performance.byteCompileLua.nvimRuntime` toggle that,
if enabled, byte compiles all lua files in Nvim runtime directory.
@MattSturgeon
Copy link
Member

@stasjok thanks again for all your hard work, persistence and dedication!

@mergify mergify bot merged commit 9314cd4 into nix-community:main Jul 31, 2024
4 checks passed
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.

4 participants