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

refactor(build): include lpeg as a library #23216

Merged
merged 1 commit into from Apr 27, 2023
Merged

Conversation

bfredl
Copy link
Member

@bfredl bfredl commented Apr 20, 2023

This reduces the build time required luarocks packages to zero. (luarocks is now only needed for testing).

alternative: vendor lpeg as src/lpeg just like src/mpack. This avoids most extra cmake code, but vendors the lpeg c code (~5000 lines). Not sure which one will make distros recoil in horror the most.

@github-actions github-actions bot added build building and installing Neovim using the provided scripts dependencies build dependencies (LuaJIT, LibUV, etc.) refactor changes that are not features or bugfixes labels Apr 20, 2023
@clason
Copy link
Member

clason commented Apr 20, 2023

Do we want to make changes to lpeg for any reason? That is usually the deciding factor on whether to vendor something.

On the other hand, would vendoring allow keeping lpeg.lua out of runtime (and hence the grubby mitts of stylua)? Vendoring would also be more robust against unreachable hosts (as lpeg seems to be hosted in the private user directory on a single university server).

@bfredl bfredl force-pushed the lpeg branch 5 times, most recently from c2cdb79 to 61a9908 Compare April 26, 2023 08:36
runtime/doc/lua.txt Outdated Show resolved Hide resolved
@bfredl bfredl force-pushed the lpeg branch 2 times, most recently from 15c627e to c8cf2be Compare April 26, 2023 09:01
@bfredl bfredl marked this pull request as ready for review April 26, 2023 09:01
runtime/doc/lua.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
cmake/FindLpeg.cmake Outdated Show resolved Hide resolved
cmake/FindLpeg.cmake Outdated Show resolved Hide resolved
Copy link
Member

@dundargoc dundargoc left a comment

Choose a reason for hiding this comment

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

The cmake part looks fine apart from some nitpicks.

@clason

This comment was marked as resolved.

@bfredl bfredl force-pushed the lpeg branch 2 times, most recently from a4f8270 to 45bcf83 Compare April 27, 2023 09:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build building and installing Neovim using the provided scripts dependencies build dependencies (LuaJIT, LibUV, etc.) refactor changes that are not features or bugfixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants