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

Add package to MELPA #120

Open
jgarte opened this issue Apr 4, 2023 · 37 comments
Open

Add package to MELPA #120

jgarte opened this issue Apr 4, 2023 · 37 comments
Labels

Comments

@jgarte
Copy link

jgarte commented Apr 4, 2023

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

@zerolfx

What do we have left to do?

@zerolfx
Copy link
Collaborator

zerolfx commented Apr 6, 2023

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
Copy link
Collaborator

zerolfx commented Apr 6, 2023

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.

@rakotomandimby
Copy link
Collaborator

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

@zerolfx
Copy link
Collaborator

zerolfx commented Apr 7, 2023

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.

@jgarte
Copy link
Author

jgarte commented Apr 7, 2023

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

@github-actions
Copy link
Contributor

github-actions bot commented May 8, 2023

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 github-actions bot added the Stale label May 8, 2023
@github-actions
Copy link
Contributor

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

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale May 14, 2023
@artlogic
Copy link

@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?

@zerolfx
Copy link
Collaborator

zerolfx commented May 19, 2023

@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.

@sshaw
Copy link

sshaw commented Dec 11, 2023

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.

@jgarte
Copy link
Author

jgarte commented Dec 11, 2023

@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.

@zerolfx zerolfx reopened this Dec 12, 2023
@zerolfx
Copy link
Collaborator

zerolfx commented Dec 12, 2023

@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!

@emil-vdw emil-vdw removed the Stale label Dec 12, 2023
@emil-vdw
Copy link
Collaborator

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?

@zerolfx
Copy link
Collaborator

zerolfx commented Dec 12, 2023

@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.

@emil-vdw
Copy link
Collaborator

@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?

@rakotomandimby
Copy link
Collaborator

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

@danielpza
Copy link

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

@jgarte
Copy link
Author

jgarte commented Dec 31, 2023

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
Copy link
Author

jgarte commented Dec 31, 2023

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.

@sshaw
Copy link

sshaw commented Dec 31, 2023

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.

@jgarte
Copy link
Author

jgarte commented Dec 31, 2023

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
Copy link
Author

jgarte commented Dec 31, 2023

@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.

@sshaw
Copy link

sshaw commented Dec 31, 2023

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.

@jgarte
Copy link
Author

jgarte commented Dec 31, 2023

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.

@zerolfx
Copy link
Collaborator

zerolfx commented Jan 2, 2024

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.

@emil-vdw
Copy link
Collaborator

emil-vdw commented Jan 9, 2024

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 emil-vdw pinned this issue Jan 10, 2024
@jcs090218
Copy link
Member

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
Copy link
Member

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

cc @emil-vdw @zerolfx

@zerolfx
Copy link
Collaborator

zerolfx commented Feb 20, 2024

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?

@jcs090218
Copy link
Member

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

@k-bx
Copy link

k-bx commented Jun 18, 2024

It's been a while. Any progress?

@jgarte
Copy link
Author

jgarte commented Sep 10, 2024

Hi, is there any update on this?

Are we looking for someone to submit the PR to MELPA?

@jcs090218
Copy link
Member

AFAIK, yes! We are waiting for other maintainers to reply. 🤔

@jgarte
Copy link
Author

jgarte commented Oct 15, 2024

@jcs090218 Would you like to open the PR to MELPA? ;() Or should someone else do it? Or should @zerolfx be the one to do it?

@jgarte
Copy link
Author

jgarte commented Oct 17, 2024

@zerolfx @jcs090218 @emil-vdw et al

I've open a PR in order to add the package to the MELPA repos:

melpa/melpa#9217

🎉 🎉 🎉

@jgarte
Copy link
Author

jgarte commented Oct 17, 2024

Oops didn't see #341 till after I opened the PR.

@jcs090218

What's the game plan? 🦆

I'm not up to date on what's going on with this.

@jcs090218
Copy link
Member

What's the game plan? 🦆

I'm not up to date on what's going on with this.

No worries! You are up to date. We still plan to add to MELPA! I have added this package to my ELPA since I completely control it. :)

Thanks for opening the PR to MELPA! 🥳

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

9 participants