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(registry): add drools-lsp #898

Merged
merged 2 commits into from
Jan 13, 2023
Merged

feat(registry): add drools-lsp #898

merged 2 commits into from
Jan 13, 2023

Conversation

errantepiphany
Copy link
Contributor

@errantepiphany errantepiphany commented Jan 13, 2023

This PR adds support for Red Hat's Drools Rule Language (ft=drools, ext=.drl). The drools-lsp server config was recently merged into nvim-lspconfig (also written by me). The generated doc link in nvim-lspconfig is here.

The companion PR in mason-lspconfig.nvim is here.

Thank you so much!

Signed-off-by: David Ward dward@redhat.com

@errantepiphany
Copy link
Contributor Author

errantepiphany commented Jan 13, 2023

I recognize that having a separate file (drools_lsp/util.lua) in addition to the standard init file (drools_lsp/init.lua) is "new", since no other server in the registry does that. But it's the best way I could think of for a user to easily configure the path to the installed jar file. Here are all the options I could think of, in decreasing preference order:

-- option 1: util.lua
require('lspconfig').drools_lsp.setup {
  settings = {
    drools = {
      jar = require('mason-registry.drools-lsp.util').get_drools_jar(),
    },
  },
}

-- option 2: mason-core.path.package_prefix
require('lspconfig').drools_lsp.setup {
  settings = {
    drools = {
      jar = (function()
        local path = require 'mason-core.path'
        local pkg = path.package_prefix 'drools-lsp'
        return path.concat { pkg, 'drools-lsp-server-jar-with-dependencies.jar' }
      end)(),
    },
  },
}

-- option 3: vim.fn.stdpath
require('lspconfig').drools_lsp.setup {
  settings = {
    drools = {
      jar = (function()
        local path = require 'mason-core.path'
        local pkg = path.concat { vim.fn.stdpath 'data', 'mason', 'packages', 'drools-lsp' }
        return path.concat { pkg, 'drools-lsp-server-jar-with-dependencies.jar' }
      end)(),
    },
  },
}

-- option 4: hardcoded path
require('lspconfig').drools_lsp.setup {
  settings = {
    drools = {
      jar = vim.fn.expand '$HOME/.local/share/nvim/mason/packages/drools-lsp/drools-lsp-server-jar-with-dependencies.jar',
    },
  },
}

All those options work, but the first option seems to me to be the "cleanest". I thought this would be okay, considering each server has its own directory in the registry, so the util.lua file is "scoped", so to speak, for that server. If only ever having one lua file for each server in the registry is the rule, then why have a separate directory per server? Instead of myserver/init.lua, it would just be myserver.lua. But having directories makes me feel like it was an intentional choice to enable this type of thing. I'm actually kind of surprised that no other server has leveraged this. Then again, maybe I'm missing something fundamental here. :)

@errantepiphany
Copy link
Contributor Author

errantepiphany commented Jan 13, 2023

I just came up with a fifth option (and added it to the code):

-- option 5: shell exec wrapper
-- ~/.config/nvim/init.lua
require('lspconfig').drools_lsp.setup({
    cmd = { 'drools-lsp' },
})
-- mason.nvim/lua/mason-registry/drools-lsp/init.lua
-- ...
return Pkg.new({
    -- ...
        ctx:link_bin(
            "drools-lsp",
            ctx:write_shell_exec_wrapper(
                "drools-lsp",
                ("java -cp %q org.drools.lsp.server.Main"):format(path.concat {
                    ctx.package:get_install_path(),
                    "drools-lsp-server-jar-with-dependencies.jar",
                })
            )
        )
    -- ...
})

Would you still be okay with me leaving the util.lua? I ask because the downside of the option above is that it's not allowing the user to specify the java path or options. I could make the drools-lsp script more complicated so that it can receive and process arguments and then turn them into options to put into the java command. But I don't see other servers in the registry doing that... Thoughts?

@williamboman
Copy link
Owner

Hey! Thanks for the detailed PR and investigation into different options. I’m afraid providing a custom Lua module (util.lua) for a package is not going to be an option. There are a couple of reasons why but main ones are 1) maintenance, and 2) the registry is being overhauled into static, versioned, YAML definitions over at https://github.com/mason-org/mason-registry. A package will no longer host any arbitrary Lua code, but will only be a static definition.

For the time being I think option 4 would be best. Option 2 & 3, while very similar, makes use of non-public APIs (mason-core.path). Reaching directly into package installation directories should also generally be avoided as these can change over time, but in cases like these it’s unavoidable. One can produce the path to the jar programmatically via require(“mason-registry”).get_package(“drools-lsp”):get_install_path() .. “/<file>.jar.

Installing artifacts other than executables to a stable location is in the backlog. This would allow this jar file to be installed to for example $MASON/share/drools/drools-lsp-server-with-dependencies.jar, which could be referenced like so in Lua:

require(”lspconfig”).drool.setup {
  jar = vim.fn.expand(”$MASON/share/drools-lsp/drools-lsp-server-with-dependencies.jar”)
}

@errantepiphany
Copy link
Contributor Author

errantepiphany commented Jan 13, 2023

@williamboman Thanks for your explanation. I can understand that using non-public apis is bad, and I can also understand why we shouldn't have util.lua per the future plans. However, we should still be able to do both option 4 (user specifies the jar path) and option 5 (we provide a wrapper script, just like I see other servers doing), yes? This way the user has a couple options, still. Please let me know, and I'll make the appropriate changes to the PR. Thanks again!

Signed-off-by: David Ward <dward@redhat.com>
@errantepiphany
Copy link
Contributor Author

@williamboman Okay, I've removed util.lua, and now only init.lua remains. As for configuration options, please look at the comment section in the top of init.lua. Does all that look better to you now? (I've tested both options.) Thanks!

@williamboman
Copy link
Owner

williamboman commented Jan 13, 2023

Great! Yeah both option 4 & 5 would make sense. Not sure if you're involved in the project/community, but do you think they'd be open to adding a manifest to the JAR file? Specifically thinking about providing a Main-Class for the application's entrypoint. This would make porting it to https://github.com/mason-org/mason-registry a bit simpler as it currently is unable to express a java -cp <path> <mainclass> executable wrapper.

edit: See for example this for a JAR executable.

@williamboman

This comment has been minimized.

Copy link
Owner

@williamboman williamboman left a comment

Choose a reason for hiding this comment

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

I applied some minor stylistic changes to make things a bit more grep-friendly. Thanks!!

@@ -0,0 +1,71 @@
--[[
Copy link
Owner

Choose a reason for hiding this comment

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

This documentation is great, but it's not the best place to put it. I see there's a great deal of documentation provided in the lspconfig server config, hopefully I didn't remove anything that isn't there already?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@williamboman It looks all good. I'll switch over my local testing nvim init.lua to use the public github location vs. my local development folders now, and re-test. But again, it looks like the changes are fine. As for the removed documentation at the top of the file, the only thing of real loss was the option of simply configuring the cmd to the exec-shell-wrapper that mason generates. That part is only available to mason users. But that's fine too. I'm going to put more documentation right into the README on the drools-lsp github repo itself. :)

Copy link
Owner

Choose a reason for hiding this comment

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

That part is only available to mason users. But that's fine too. I'm going to put more documentation right into the README on the drools-lsp github repo. :)

Ah, we should be able to provide any necessary extra configuration in mason-lspconfig to turn it into a zero-config setup for Mason users. This directory contains configuration overrides provided by Mason.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, something like this seems like it would do the trick. But how does one leverage that mechanism? Or is it just automatic? I'll create a new branch, make the change, test it, and if all good, submit a new PR for that.

Oh, and btw, I re-tested with your changes straight from github main, and it all works still. So your changes were fine. :)

Copy link
Owner

Choose a reason for hiding this comment

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

Awesome, thanks!! Yep it's automatic. I saw you had a question regarding the directory structure and it's the same structure over there - directories instead of files. It's just a personal preference of mine (in the case of mason-lspconfig it does allow for additional, adjacent, modules should they be needed).

The directory name needs to match up with the lspconfig server name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Although oops, I must have missed putting drools-lsp in https://github.com/williamboman/mason.nvim/blob/main/PACKAGES.md, though... But I think you do regular re-generates, yes?

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah that part is automated on a daily basis. Helps reduce code traffic in manual processes.

@williamboman williamboman merged commit dd61453 into williamboman:main Jan 13, 2023
@errantepiphany
Copy link
Contributor Author

errantepiphany commented Jan 13, 2023

I'm actually the Engineering Manager for the Drools team at Red Hat. ;) So yes, I can have the Main-Class added. Once that gets merged over there, then I'll submit a PR here that changes the server to do a java -jar instead of java -cp, and then I'll also add the config overrides piece in the same PR.

@errantepiphany errantepiphany deleted the DROOLS_LSP branch January 13, 2023 19:09
This was referenced Apr 14, 2023
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.

None yet

3 participants