neovim: Build and install tree-sitter parsers #24120

thestr4ng3r · 2024-05-24T18:59:26Z

Description

I consider this patch as some kind of WIP proposal. Perhaps things like the path to the cmake executable or its args should be filled dynamically from macports defaults. But I do not know the best practices for this specific case so I feel free to just push on top of this if it needs refinement.

neovim needs a number of prebuilt parsers in /lib/nvim/parser/ or else it will throw errors when opening e.g. ":help". USE_BUNDLED is also not a valid option for the root cmake build system anymore, but applies to the one in cmake.deps/.

Closes: https://trac.macports.org/ticket/70072

Type(s)

bugfix
enhancement
security fix

Tested on

macOS 15.5
Xcode 15.3 / Command Line Tools 15.3.0

Verification

Have you

followed our Commit Message Guidelines?
squashed and minimized your commits?
checked that there aren't other open pull requests for the same change?
referenced existing tickets on Trac with full URL in commit message?
checked your Portfile with port lint --nitpick?
tried existing tests with sudo port test?
tried a full install with sudo port -vst install? (Did it without trace mode because of https://trac.macports.org/ticket/66358)
tested basic functionality of all binary files?
checked that the Portfile's most important variants haven't been broken?

macportsbot · 2024-05-24T18:59:31Z

Notifying maintainers:
@judaew for port neovim.
@raimue for port neovim.
@l2dy for port neovim.

ryandesign · 2024-05-24T20:30:00Z

editors/neovim/Portfile

+ system -W ${worksrcpath} "cmake -S cmake.deps -B .deps -G \"Unix Makefiles\" -D CMAKE_BUILD_TYPE=Release -DUSE_BUNDLED=OFF -DUSE_BUNDLED_TS_PARSERS=ON"
+ system -W ${worksrcpath} "cmake --build .deps"


This doesn't make use of all of the MacPorts settings that we expect ports to make use of. For example, it doesn't build for the user's desired build_arch or universal_archs or with the MacPorts default optimization flags. All of those details are handled in the cmake portgroups, but those are designed for ports that do only that and can't be easily mixed with ports that also need to do other build tasks.

The desired number of parallel build jobs is another MacPorts setting that the above does not make use of.

I see that this port already uses the cmake portgroup. cmake has extensive capabilities for building subprojects as a part of a larger build. Why hasn't neovim made use of those facilities so that everything gets built using just the original top-level cmake invocation that the port already uses?

If the tree-sitter parsers will forever remain a separate build, can they be built in a separate portfile that the neovim port depends on? That separate portfile could then simply use the cmake portgroup as intended to get all expected MacPorts behaviors for free.

Exactly my thoughts. I wasn't able to find any reasoning behind this kind of design yet, so I opened an issue there. Let's see what they think: neovim/neovim#29042
Separate port could work, but it seems unnecessarily complicated to have an additional package that only a single other package ever uses and depends on. If this will be the way to go, I also wonder if this could be done as a subport.

Quick summary of the discussion with upstream: An attempt to move all deps into the toplevel cmake was made in neovim/neovim#22054, but dismissed because of technical reasons.
For the parsers specifically, there is a slight chance it might happen in the future though.
But for the current release, we have to work around it of course. I see the following possibilities:

Use an approach similar to what is already implemented in the PR, using a pre-configure step and try to shove all the cmake args from the cmake port group in there manually as good as possible

Split neovim in two packages. Debian uses a similar approach (neovim + neovim-runtime): https://salsa.debian.org/vim-team/neovim/-/tree/debian/experimental/debian Though they have a lot of custom logic there as well and splitting packages like this is also more common in debian.

If possible without too much effort, try to patch neovim's toplevel cmake to also include the parsers.

It would be good to know which approach macports maintainers would prefer before I move forward. I am currently leaning a bit towards the third option (patching), assuming the patch can be made small and clear, since (I think) this may be easier to revert than a split package in case it becomes better integrated upstream in the future.

I'd prefer the second option, but if you can make a clean and easy to adapt patch, option 3 is acceptable.

@ryandesign @l2dy I pushed a possible solution with a patch, which applies to both the release and neovim-devel. Whether it is clean is subjective I would say. If you don't like it I will attempt to split split the package.

editors/neovim/Portfile

l2dy · 2024-06-05T10:46:37Z

editors/neovim/files/embed-parsers-build.diff

+ ${EXTERNALPROJECT_OPTIONS})
+ endfunction()
+
+set(ALL_BUNDLED_TS_PARSER_TARGETS c lua vim vimdoc query python bash markdown)


Where is the list of languages from?

From the original code lines directly below. I have now added a comment to clarify this.

l2dy · 2024-06-05T10:53:17Z

editors/neovim/files/embed-parsers-build.diff

+- add_custom_command(TARGET nvim_runtime_deps COMMAND ${CMAKE_COMMAND} -E ${COPY_DIRECTORY} ${DEPS_PREFIX}/lib/nvim/parser ${BINARY_LIB_DIR}/parser)
+-endif()
+add_dependencies(nvim_runtime_deps ${ALL_BUNDLED_TS_PARSER_TARGETS})
+add_custom_command(TARGET nvim_runtime_deps COMMAND ${CMAKE_COMMAND} -E ${COPY_DIRECTORY} ${DEPS_INSTALL_DIR}/lib/nvim/parser ${BINARY_LIB_DIR}/parser)


Have you ran otool -L on the produced binaries? If it's referencing other dynamic libraries via relative path or path not under the MacPorts prefix, we should use the separate Portfile approach instead.

Do you know what level of optimization (e.g. -O1) is applied? It's best to follow MacPorts configuration configure.optflags, but a sane default LGTM.

They only use /usr/lib/libSystem.B.dylib:

florian-macbook:neovim florian$ port contents neovim | grep parser | xargs -n 1 otool -L /opt/local/lib/nvim/parser/bash.so: /usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1345.100.2) /opt/local/lib/nvim/parser/c.so: /usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1345.100.2) /opt/local/lib/nvim/parser/lua.so: /usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1345.100.2) /opt/local/lib/nvim/parser/markdown.so: /usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1345.100.2) /opt/local/lib/nvim/parser/markdown_inline.so: /usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1345.100.2) /opt/local/lib/nvim/parser/python.so: /usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1345.100.2) /opt/local/lib/nvim/parser/query.so: /usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1345.100.2) /opt/local/lib/nvim/parser/vim.so: /usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1345.100.2) /opt/local/lib/nvim/parser/vimdoc.so: /usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1345.100.2)

Regarding optimization it was using the default flags from cmake when CMAKE_BUILD_TYPE is not set. I have now pushed a change to pass through CMAKE_BUILD_TYPE and CMAKE_C_FLAGS, which will then come from the cmake port group.
For example, for me macports now runs the toplevel cmake like this:

DEBUG: CFLAGS='-pipe -Os -DNDEBUG -I/opt/local/include -isysroot/Library/Developer/CommandLineTools/SDKs/MacOSX14.sdk' /opt/local/bin/cmake -G "CodeBlocks - Unix Makefiles" -DCMAKE_BUILD_TYPE=Release -DCMAKE_INSTALL_PREFIX="/opt/local" -DCMAKE_INSTALL_NAME_DIR="/opt/local/lib" -DCMAKE_SYSTEM_PREFIX_PATH="/opt/local;/usr" -DCMAKE_C_COMPILER="$CC" -DCMAKE_CXX_COMPILER="$CXX" -DCMAKE_OBJC_COMPILER="$CC" -DCMAKE_OBJCXX_COMPILER="$CXX" -DCMAKE_POLICY_DEFAULT_CMP0025=NEW -DCMAKE_POLICY_DEFAULT_CMP0060=NEW -DCMAKE_VERBOSE_MAKEFILE=ON -DCMAKE_COLOR_MAKEFILE=ON -DCMAKE_FIND_FRAMEWORK=LAST -DCMAKE_EXPORT_COMPILE_COMMANDS=ON -DCMAKE_MAKE_PROGRAM=/usr/bin/make -DCMAKE_MODULE_PATH="/opt/local/share/cmake/Modules" -DCMAKE_PREFIX_PATH="/opt/local/share/cmake/Modules" -DCMAKE_BUILD_WITH_INSTALL_RPATH:BOOL=ON -DCMAKE_INSTALL_RPATH="/opt/local/lib" -Wno-dev -DLUA_PRG=/opt/local/bin/luajit -DCMAKE_OSX_ARCHITECTURES="arm64" -DCMAKE_OSX_DEPLOYMENT_TARGET="14.0" -DCMAKE_OSX_SYSROOT="/Library/Developer/CommandLineTools/SDKs/MacOSX14.sdk" /opt/local/var/macports/build/_Users_florian_dev_ports-dev_editors_neovim/neovim/work/neovim-0.10.0

and an ExternalProject for a parser is configured like this:

/opt/local/bin/cmake -D CMAKE_C_COMPILER=/usr/bin/clang -D CMAKE_C_STANDARD=99 -D "CMAKE_GENERATOR=Unix Makefiles" -D CMAKE_GENERATOR_PLATFORM= -D BUILD_SHARED_LIBS=OFF -D CMAKE_POSITION_INDEPENDENT_CODE=ON -D CMAKE_INSTALL_PREFIX=/opt/local/var/macports/build/_Users_florian_dev_ports-dev_editors_neovim/neovim/work/build/usr -D CMAKE_FIND_FRAMEWORK=LAST -D CMAKE_POLICY_DEFAULT_CMP0092=NEW -D CMAKE_BUILD_TYPE=Release -D "CMAKE_C_FLAGS=-pipe -Os -DNDEBUG -I/opt/local/include -isysroot/Library/Developer/CommandLineTools/SDKs/MacOSX14.sdk" -D PARSERLANG=c "-GCodeBlocks - Unix Makefiles" -S /opt/local/var/macports/build/_Users_florian_dev_ports-dev_editors_neovim/neovim/work/build/build/src/treesitter_c -B /opt/local/var/macports/build/_Users_florian_dev_ports-dev_editors_neovim/neovim/work/build/build/src/treesitter_c-build

neovim needs a number of prebuilt parsers in <prefix>/lib/nvim/parser/ or else it will throw errors when opening e.g. ":help". USE_BUNDLED is also not a valid option for the root cmake build system anymore, but applies to the one in cmake.deps/. Closes: https://trac.macports.org/ticket/70072

l2dy

LGTM. Let's see what others think.

l2dy · 2024-06-08T05:47:41Z

Merged. Thank you!

neovim needs a number of prebuilt parsers in <prefix>/lib/nvim/parser/ or else it will throw errors when opening e.g. ":help". USE_BUNDLED is also not a valid option for the root cmake build system anymore, but applies to the one in cmake.deps/. Closes: https://trac.macports.org/ticket/70072

macportsbot assigned judaew, raimue and l2dy May 24, 2024

macportsbot added type: bugfix maintainer: open Affects an openmaintainer port labels May 24, 2024

ryandesign reviewed May 24, 2024

View reviewed changes

editors/neovim/Portfile Show resolved Hide resolved

thestr4ng3r mentioned this pull request May 27, 2024

Integrate tree-sitter parsers into the toplevel build system neovim/neovim#29042

Open

thestr4ng3r force-pushed the neovim-ts-parsers branch from 904b1a7 to 5a71c5b Compare June 4, 2024 17:04

l2dy reviewed Jun 5, 2024

View reviewed changes

thestr4ng3r force-pushed the neovim-ts-parsers branch from 5a71c5b to 9d748f8 Compare June 5, 2024 12:15

l2dy approved these changes Jun 5, 2024

View reviewed changes

l2dy merged commit 77f8d99 into macports:master Jun 8, 2024
4 checks passed

thestr4ng3r deleted the neovim-ts-parsers branch June 8, 2024 06:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

neovim: Build and install tree-sitter parsers #24120

neovim: Build and install tree-sitter parsers #24120

thestr4ng3r commented May 24, 2024 •

edited

macportsbot commented May 24, 2024

ryandesign May 24, 2024

ryandesign May 24, 2024

thestr4ng3r May 27, 2024

thestr4ng3r May 31, 2024

l2dy Jun 3, 2024

thestr4ng3r Jun 4, 2024

l2dy Jun 5, 2024

thestr4ng3r Jun 5, 2024

l2dy Jun 5, 2024

thestr4ng3r Jun 5, 2024 •

edited

l2dy left a comment

l2dy commented Jun 8, 2024

		system -W ${worksrcpath} "cmake -S cmake.deps -B .deps -G \"Unix Makefiles\" -D CMAKE_BUILD_TYPE=Release -DUSE_BUNDLED=OFF -DUSE_BUNDLED_TS_PARSERS=ON"
		system -W ${worksrcpath} "cmake --build .deps"

neovim: Build and install tree-sitter parsers #24120

neovim: Build and install tree-sitter parsers #24120

Conversation

thestr4ng3r commented May 24, 2024 • edited

Description

Type(s)

Tested on

Verification

macportsbot commented May 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thestr4ng3r Jun 5, 2024 • edited

Choose a reason for hiding this comment

l2dy left a comment

Choose a reason for hiding this comment

l2dy commented Jun 8, 2024

thestr4ng3r commented May 24, 2024 •

edited

thestr4ng3r Jun 5, 2024 •

edited