copilot.el icon indicating copy to clipboard operation
copilot.el copied to clipboard

Add package to MELPA

Open jgarte opened this issue 2 years ago • 37 comments

Just opening this issue to track adding copilot.el to MELPA.

@zerolfx

What do we have left to do?

jgarte avatar Apr 04 '23 15:04 jgarte

I am planning to use the copilot-node-server NPM package (which is used by https://github.com/TerminalFi/LSP-copilot).

The primary tasks that must be completed are:

  • Installing the NPM package on behalf of the user (in a similar manner to installing a language server in lsp-mode).
  • Finding a method to check the current version and upgrade it if necessary.

Minor tasks include:

  • Ensuring that current users can migrate to the new version
  • Adding an open-source license
  • Submitting the package to MELPA
  • Adding CI for versioning, packaging, and other related tasks

zerolfx avatar Apr 06 '23 08:04 zerolfx

There are a few potential issues to consider:

  • npm becomes a new dependency.
  • Some users may not want to install the npm package globally (though it is the default behavior of lsp-install-server).
  • It can be challenging to check the current version of an installed NPM package.
  • Setting up copilot.el requires additional steps due to these changes.

zerolfx avatar Apr 06 '23 09:04 zerolfx

Node is already a dependency, which always embeds npm, AFAIK.

rakotomandimby avatar Apr 06 '23 10:04 rakotomandimby

Node is already a dependency, which always embeds npm, AFAIK.

On Ubuntu and Arch, npm is not automatically installed when installing nodejs, as it's an optional dependency. But yes, we can assume npm is available if node is installed.

zerolfx avatar Apr 07 '23 01:04 zerolfx

If I were to package this for GNU Guix I would be patching this line to something like the following:

/gnu/store/gvwnkilh065b0sv9rr64xl50s0snjfl1-node-14.19.3/bin/node

Here is some discussion about copilot.el in GNU Guix:

https://issues.guix.gnu.org/62664#0

jgarte avatar Apr 07 '23 04:04 jgarte

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

github-actions[bot] avatar May 08 '23 02:05 github-actions[bot]

This issue was closed because it has been stalled for 5 days with no activity.

github-actions[bot] avatar May 14 '23 02:05 github-actions[bot]

@zerolfx I noticed this issue was closed and the item is crossed out in the readme. Does this mean there are no plans to add this package to MELPA?

artlogic avatar May 17 '23 15:05 artlogic

@artlogic (The item was crossed out in the readme a long while ago.) I have no plans to add the package to MELPA. However, I welcome any contributions.

zerolfx avatar May 19 '23 01:05 zerolfx

Why is this not being distributing via Melpa, because of a Node.js dependency? Not clear but not using standard emacs distribution channels is a mistake. Many packages depended on 3rd-party executables.

sshaw avatar Dec 11 '23 17:12 sshaw

@zerolfx

You should consider @sshaw comment above. Putting this package on MELPA will increase wider adoption of copilot.el amongst other benefits. If you need help with packaging and submitting it just let me know and I can try to help out.

jgarte avatar Dec 11 '23 17:12 jgarte

@sshaw @jgarte Yes, it makes sense but I need help.

A few things to consider:

  • Store target commit ID of copilot.vim in a variable.
  • Prompt and auto-install binary files for new users.
  • Prompt users to reinstall files in case of version mismatch.

Appreciate any assistance. Thank you!

zerolfx avatar Dec 12 '23 02:12 zerolfx

This is definitely something worth prioritizing I think. I'd also be interested to lend a hand. Just a few questions:

  1. Why are we planning to store target commit ID of copilot.vim? Are we not able to distribute the package with the copilot.vim dependencies?
  2. The binary files we want to auto install for new users is nodejs and?
  3. What files could mismatch and require a reinstall?

emil-vdw avatar Dec 12 '23 13:12 emil-vdw

@emil-vdw

Sorry for not making it clear.

The problem is, before submitting this plugin to MELPA, we may need to get rid of the dist folder to make it a fully open-source project. To remove the binary files (e.g. agent.js), we need to provide a way for users to install them with a compatible version (automatically download the copilot.vim repo). Also, we need to provide a way to notify users if their binary files are out-to-date and help them to update them.

zerolfx avatar Dec 12 '23 13:12 zerolfx

@zerolfx would it not be possible to keep dist in our repo but not include it in the MELPA package recipe. That way we don't have to distribute that through MELPA but the dist folder can be downloaded from the git tag on copilot.el that matches the package version installed. That gives us more control over the distribution and more flexibility to modify those files if needed in the future.

One thing that gives me pause is that I'm not quite sure what the license is on the copilot.vim repo.

We also be pertinent to add a license to copilot.el. Shall I open a ticket?

emil-vdw avatar Dec 13 '23 10:12 emil-vdw

@emil-vdw , the license topic defimotely deserves a dedicated topic / ticket.

rakotomandimby avatar Dec 13 '23 11:12 rakotomandimby

To remove the binary files (e.g. agent.js), we need to provide a way for users to install them with a compatible version (automatically download the copilot.vim repo). Also, we need to provide a way to notify users if their binary files are out-to-date and help them to update them.

Alternatively we could ask the user to install copilot-node-server themselves and expect it to be installed in the system, so we don't have to manage the installation. If they need npm/node in their system anyway it's as easy as npm install -g copilot-node-server

danielpza avatar Dec 31 '23 02:12 danielpza

If they need npm/node in their system anyway it's as easy as npm install -g copilot-node-server

I think that that is an excellent idea!

jgarte avatar Dec 31 '23 06:12 jgarte

Here's an example of a prominent Emacs package that does exactly that:

https://github.com/minad/consult/blob/138aff9bdf48f100d6b0c33acf695cc4aed8346c/consult.el#L273

consult does not distribute ripgrep for the user when they install consult. Instead, the author of consult expects the user to have installed ripgrep by some other means. For example, system package manager, etc.

jgarte avatar Dec 31 '23 14:12 jgarte

Aside from any licensing issues, Is there a reason to not bundle copilot-node-server?

consult does not distribute ripgrep for the user when they install consult.

Not a consult user 😅 but I think they do not bundle it because it's not required. ~Once~ One can use grep if they prefer.

sshaw avatar Dec 31 '23 15:12 sshaw

Aside from any licensing issues, Is there a reason to not bundle copilot-node-server?

Hi,

The reason is because the developers of copilot.el should let a dedicated package manager take care of the task of system level package management.

See this guix package for tokei.el, for example:

https://git.savannah.gnu.org/cgit/guix.git/tree/gnu/packages/emacs-xyz.scm?id=master#n28047

The tokei program is referenced below in the defcustom form but the tokei.el package does not try to install the package for the user:

https://github.com/nagy/tokei.el/blob/master/tokei.el#L47

The user just needs to make sure that tokei is available on their system.

In other words, tokei.el does not have to also implement system package management.

Does that make sense?

jgarte avatar Dec 31 '23 17:12 jgarte

@sshaw

It's the same reason that a git binary is not vendored in the repository of git-link.el.

Thanks for that package, BTW. I use it regularly.

jgarte avatar Dec 31 '23 18:12 jgarte

The reason is because the developers of copilot.el should let a dedicated package manager take care of the task of system level package management.

Isn't package.el a dedicated package manager? :)

I was looking for a detailed reason, e.g., GitHub's license does not allow it.

The user just needs to make sure that tokei is available on their system. ... It's the same reason that a git binary is not vendored in the repository of git-link.el.

Doing it because someone else is doing it is not a good reason. Additional analysis should be applied before making a conclusion. Maybe that was done? Not sure. Which is why I was asking :)

With git-link.el you do not install git-link.el to use git. You're using git already. It supplements your existing usage.

I assume the case with toki is it's a general purpose executable with 1000s of users outside of Emacs. But may be a licensing issue or distribution size issue. Not sure. People may install toki exclusively for Emacs use but it remains a general purpose executable.

In addition toki a standalone executable that does not require a runtime. The proper comparison here is toki to Node.js, and I have not suggested that Node.js be bundled (yet 🤪)

copilot-node-server was created exclusively for the editing environment. If you're not using it for Emacs (or vim, I suppose) would you ever have it installed? Likely no. This is the difference.

Most Node.js developers use version managers so things will get messy and I guarantee you there will be issues between the version used to install the package and what's in the path, etc... So barriers to entry should be minimum. Currently everything is bundled and all one must do it set the path to node. The idea behind using a package manager is to make using this package easy. Not to add extra steps. So, unless there are legit technical and/or legal reasons, I would bundle it.

sshaw avatar Dec 31 '23 20:12 sshaw

Isn't package.el a dedicated package manager? :)

Yes, package.el is a dedicated package manager for Emacs Lisp code in the same way that npm is a dedicated package manager for JavaScript, and cargo is a dedicated package manager for Rust, etc. In other words, I wouldn't try to package copilot-node-server with package.el. Feel free to correct me if I misunderstood what you were suggesting.

I would share your dependency question with the MELPA repository maintainers to see what they recommend and how they would approach it. They will have a more comprehensive and detailed explanation that is probably already clearly documented in their packaging guidelines.

jgarte avatar Dec 31 '23 23:12 jgarte

The tokei program is referenced below in the defcustom form but the tokei.el package does not try to install the package for the user:

https://github.com/nagy/tokei.el/blob/master/tokei.el#L47

The user just needs to make sure that tokei is available on their system.

In other words, tokei.el does not have to also implement system package management.

@jgarte Good examples. However, I'd like to highlight a possible difference - a particular version of copilot.el might only work with a specific version of copilot-node-server. We may need to advise users to install or upgrade to certain versions accordingly. It's not impossible, but it requires more effort from both copilot.el and the users.

zerolfx avatar Jan 02 '24 09:01 zerolfx

Since:

  1. The relationship between copilot server and copilot.el. (Being a dependency of copilot.el and copilot.el not being an extension of the copilot server that is usable or likely to be used in a standalone way)
  2. The complexity it would introduce on the part of the copilot.el maintainers and especially the downstream users to potentially manage and update dependencies manually for an update. (since a particular version of copilot.el might only work with a specific version of copilot-node-server as pointed out above by @zerolfx)

It seems to me that the best approach would be to handle the loading of the copilot server as a dependency of copilot.el itself automatically (or by exposing some copilot-install-server function that the user could run manually post update).

From the perspective of the MELPA repo maintainers I wouldn't think that it matters as long as we are not distributing unlicenced code through their repo. I may be wrong though.

emil-vdw avatar Jan 09 '24 12:01 emil-vdw

If the author is interested, I can work on the CI and the package installation. I've helped lsp-mode integrate their CI workflows. For package installation, it wasn't really hard to do.

jcs090218 avatar Jan 28 '24 03:01 jcs090218

Since #247 is merged, we can proceed with this request. What's our next step?

cc @emil-vdw @zerolfx

jcs090218 avatar Feb 20 '24 07:02 jcs090218

We need to follow the instructions here: https://github.com/melpa/melpa/blob/master/CONTRIBUTING.org

  1. clean up unnecessary files (dist folder, GitHub workflow)
  2. add a new license (I suggest MIT, which is GPL-compatible)
  3. update code to align with MELPA's requirements
  4. submit a PR to MELPA's GitHub repo
  5. update README after the PR getting merged

@jcs090218 Sounds good?

zerolfx avatar Feb 20 '24 08:02 zerolfx

1 to 3 are done now. We can now open the PR to MELPA. I cannot open PRs to MELPA since I have other PRs waiting for review. Can someone open a PR to MELPA? 🤔

cc @emil-vdw @zerolfx

jcs090218 avatar Mar 27 '24 22:03 jcs090218