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

feat: package for luarocks #356

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Conversation

mrcjkb
Copy link

@mrcjkb mrcjkb commented Nov 21, 2024

Closes #217

  • Adds support for building with luarocks.
  • Adds some busted tests to make sure the rust library can be called from lua
    (based on this guide).
  • Runs the nix build (which also runs the tests) on PR.
    We could also potentially change this to a workflow that doesn't use nix (see the Makefile and test.sh).
  • Updates the nix build to build a luarocks package/neovim plugin.

This PR falls back the old behaviour of downloading pre-built binaries if the rust module is not found, so that it can still be used without luarocks.

TODO:

  • luarocks-tag-release workflow for publishing releases to luarocks.org (in a follow-up PR)
  • release-please workflow for generating semver tags based on conventional commits (follow-up PR, if wanted?)
  • The location of the rust sources has changed. I suppose the scripts for building the pre-built binaries will need to be updated.

@mrcjkb mrcjkb force-pushed the mj/push-mxkzxtywqxvw branch from 8ffbcc1 to 8246a17 Compare November 21, 2024 00:49
@Saghen Saghen force-pushed the mj/push-mxkzxtywqxvw branch from 4134c23 to c3f041a Compare November 21, 2024 02:43
@Saghen
Copy link
Owner

Saghen commented Nov 21, 2024

Tysm! Would it also be possible to ship prebuilt binaries through luarocks via the luarocks-tag-release? It might be helpful on some systems although it's minor since the build is so seamless. To clarify, does the luarocks build handle installing the nightly rust toolchain?

I'll take a look at the failing nix macos build soon and I think I'll pass on the release-please workflow for now.

@mrcjkb
Copy link
Author

mrcjkb commented Nov 21, 2024

Would it also be possible to ship prebuilt binaries through luarocks via the luarocks-tag-release?

We could add pre-packaged builds that include the binaries to the neorocks rocks-binaries server, which checks for updates twice per day and builds for x86_64-linux, x86_64-darwin, aarch64-darwin and x86_64-windows.
Afaik, both rocks.nvim and lazy.nvim configure luarocks to pull from the rocks-binaries server.

I'd suggest waiting with merging this until I have a draft for rocks-binaries workflows set up and until I have a luarocks-tag-release PR stacked on this one (I'll do it later today or tomorrow). Otherwise lazy.nvim users might complain.

To clarify, does the luarocks build handle installing the nightly rust toolchain?

No, it tries to use whatever cargo executable it finds on the PATH.

I'll take a look at the failing nix macos build soon

Looks like the busted tests are failing for some reason, but curiously, without any output. 🤔
The luarocks make command to build the binary seems to have run fine.

@mrcjkb mrcjkb force-pushed the mj/push-mxkzxtywqxvw branch 2 times, most recently from 82eb558 to e5320c8 Compare November 21, 2024 09:06
@mrcjkb
Copy link
Author

mrcjkb commented Nov 21, 2024

I've added some notes to the readme

@mrcjkb mrcjkb force-pushed the mj/push-mxkzxtywqxvw branch from e5320c8 to d66b41c Compare November 22, 2024 18:06
@mrcjkb
Copy link
Author

mrcjkb commented Nov 22, 2024

Just figured out that it's the call to require("blink_cmp_fuzzy") that is failing on darwin.
I guess something is wrong with the darwin cargo build 🤔

@mrcjkb mrcjkb force-pushed the mj/push-mxkzxtywqxvw branch from d66b41c to a1772af Compare November 22, 2024 19:30
@mrcjkb
Copy link
Author

mrcjkb commented Nov 22, 2024

I just realised I can't stack PRs from a fork 😅
So I've added the luarocks workflow to this PR.

Some notes on that:

  • To be able to publish, you will need a luarocks account and a LUAROCKS_API_KEY that must be added to this repo's GitHub Actions secrets.
  • If you create releases using a GitHub Actions workflow, you might have to create a personal access token, in order for the luarocks workflow to be triggered automatically.

More details in this guide

@Saghen
Copy link
Owner

Saghen commented Nov 22, 2024

No, it tries to use whatever cargo executable it finds on the PATH.

We could add pre-packaged builds that include the binaries to the neorocks rocks-binaries server, which checks for updates twice per day and builds for x86_64-linux, x86_64-darwin, aarch64-darwin and x86_64-windows.

For releases, would it be possible to manually trigger these builds, perhaps by webhook, or build it ourselves and push to the rocks-binaries server? I'm looking to avoid making rust nightly a requirement for the stable releases. Thanks again for all your effort here!

@Saghen
Copy link
Owner

Saghen commented Nov 22, 2024

Perhaps it would make sense for blink.cmp to continue serving its own prebuilt binaries and only fallback to building through luarocks when on main. As per #145, we also want to support aarch64-linux-android for example. Interested in your thoughts though

@mrcjkb
Copy link
Author

mrcjkb commented Nov 22, 2024

For releases, would it be possible to manually trigger these builds, perhaps by webhook, or build it ourselves and push to the rocks-binaries server? I'm looking to avoid making rust nightly a requirement for the stable releases. Thanks again for all your effort here!

I am looking into adding a blink.cmp to the existing rocks-binaries workflows.
It requires the package to be available on luarocks though, as it works as follows:

  • Install toolchains needed to build binary packages
  • luarocks install <package> - installs the package from luarocks.org
  • luarocks pack <package> - packs the pre-built package so it can be uploaded to rocks-binaries

Here's the current workflow file for Linux, for example: https://github.com/nvim-neorocks/rocks-binaries/blob/main/.github/workflows/build-linux-x86_64.yml

@mrcjkb
Copy link
Author

mrcjkb commented Nov 23, 2024

I managed to set up a darwin nix-community builder.
Here's the error:

┃        > blink.cmp scm-1 depends on lua 5.1 (5.1-1 provided by VM: success)
┃        > Auto configuration failed
┃        > 8268074816:error:02FFF001:system library:func(4095):Operation not permitted:/AppleInternal/Library/BuildRoots/289ffcb4-455d-11ef-953d-e2437461156c/Library/Caches/com.apple.xbs/Sources/libressl/libressl-3.3/crypto/bio/bss_file.c:122:fopen('/private/etc/ssl/openssl.cnf', 'rb')
┃        > 8268074816:error:20FFF002:BIO routines:CRYPTO_internal:system lib:/AppleInternal/Library/BuildRoots/289ffcb4-455d-11ef-953d-e2437461156c/Library/Caches/com.apple.xbs/Sources/libressl/libressl-3.3/crypto/bio/bss_file.c:127:
┃        > 8268074816:error:0EFFF002:configuration file routines:CRYPTO_internal:system lib:/AppleInternal/Library/BuildRoots/289ffcb4-455d-11ef-953d-e2437461156c/Library/Caches/com.apple.xbs/Sources/libressl/libressl-3.3/crypto/conf/conf_def.c:202:
┃        >
┃        > Error: Build error: Failed building.

@mrcjkb
Copy link
Author

mrcjkb commented Nov 23, 2024

Potentially related?

It succeeds with --option sandbox false.

@Saghen
Copy link
Owner

Saghen commented Nov 24, 2024

I am looking into adding a blink.cmp to the existing rocks-binaries workflows.
It requires the package to be available on luarocks though, as it works as follows:

Sounds good but just to clarify

We could add pre-packaged builds that include the binaries to the neorocks rocks-binaries server, which checks for updates twice per day and builds for x86_64-linux, x86_64-darwin, aarch64-darwin and x86_64-windows.

How can we ensure that there is always a binary available for the latest release? It sounds like for up to 12 hours after a release goes out, there won't be any prebuilt binaries available.

I'm also curious about the targets, since we support quite a few at this point: https://github.com/Saghen/blink.cmp/releases/tag/v0.6.1 (x86_64/aarch64-linux-musl/gnu/android)

@mrcjkb
Copy link
Author

mrcjkb commented Nov 25, 2024

How can we ensure that there is always a binary available for the latest release? It sounds like for up to 12 hours after a release goes out, there won't be any prebuilt binaries available.

To be able to trigger workflows between different GitHub repos, you would need access to the repo whose workflow you want to trigger.
I would suggest a separate workflow for blink.cmp that runs more frequently. I could potentially see if it's possible to generate an access token for just that workflow, but I don't know if that's possible.

I'm also curious about the targets, since we support quite a few at this point

The current rocks-binaries setup only builds for the targets supported by GitHub actions runners.
It may be possible to use nix to cross-build for other Linux targets via KVM.

@Saghen
Copy link
Owner

Saghen commented Nov 25, 2024

Gotcha, in that case, perhaps we leave the rust build and binary part out and only use luarocks for packaging the lua code. Nix handles the rust toolchain setup and provides a reproducible environment, so I'd prefer pointing users that build from source there. Non-nix users can either use the rock or clone the repo directly and the binary will be downloaded on first run, or setup rust nightly themselves as per usual.

@mrcjkb
Copy link
Author

mrcjkb commented Nov 25, 2024

Hmm, from a nixpkgs maintenance perspective, it would be nice to have the test suite run in the checkPhase.
What would you think of the idea of PRing the luarocks-build-rust-mlua build backend with an option to only build the rust binary if the correct toolchain is detected, and to just install the lua modules (printing a warning) if it isn't?

@simifalaye
Copy link

@Saghen @mrcjkb Any news on this?

@Saghen
Copy link
Owner

Saghen commented Dec 10, 2024

I'll take another look closer to 1.0 but I think we'll end up packaging only the lua/rust code, without a build step

@milanglacier
Copy link
Contributor

milanglacier commented Dec 22, 2024

I agree that we should avoid integrating the build step into Rockspec. This
decision is not only due to the limited scope of supported targets but also
because of how lazy.nvim manages rockspec files.

Currently, lazy.nvim utilizes luarocks to build a Neovim package only if
the rockspec includes a complex build step — as would be the case with blink,
which relies on the Rust toolchain. If blink provides a rockspec requiring a
Rust-based build, lazy.nvim will attempt to use luarocks to handle the
process. However, this approach feels unnecessary. Since blink already ships
with precompiled binaries and can alternatively use cargo build --release to
build the package, there is no need to introduce luarocks, especially
considering that users of lazy.nvim may not prefer luarocks for package
installation.

Additionally, lazy.nvim has a default behavior where, if luarocks is
required but not installed, it will invoke hererocks to install luarocks
and proceed to install the package. For the average user using lazy.nvim
with its default settings, this adds complexity that could be avoided. Instead,
blink could simply provide binaries for download from its releases as it is now.

While the default implementation of lazy.nvim’s luarocks support might seem
"hack", however lazy.nvim is the most popular package manager within the
Neovim ecosystem. There may be worth considering adjustments to accommodate
its design.

Another way is not including the rockspec file in the blink-cmp repository
and we use another repository for build rockspec for blink. This ensures that users of
lazy.nvim can add Saghen/blink.cmp to their package specifications without
concern for luarocks. Meanwhile, those who prefer using luarocks to manage
Neovim packages can just install the prebuilt blink-cmp package from the
luarocks server as usual.

My personal opnion is that, unless neovim incorporates luarocks as a core
component, we should avoid forcing users to install luarocks unless it is
absolutely essential for running a package — for instance, when the plugin
requires a Lua package.

@mrcjkb
Copy link
Author

mrcjkb commented Dec 22, 2024

@milanglacier That's a problem that would be easy to solve: If a lazy.lua file is present in the project root, lazy.nvim will ignore any rockspecs (please correct me if I'm wrong about this).

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.

Publish to luarocks with automated semver versioning/releases
4 participants