fix: Find jsonrpc by header file

[jcs090218]

It seems like (package-get-descriptor 'jsonrpc) will return the the latest version on ELPA. Use locate-library instead.

[jkl1337]

This method is more robust, though I submitted an alternative PR that avoids avoidable TOCTOU and adds necessary require 'package.

[jcs090218]

See my comment https://github.com/copilot-emacs/copilot.el/issues/262#issuecomment-1947302208.

[jkl1337]

As noted in https://github.com/copilot-emacs/copilot.el/issues/262#issuecomment-1947306345, (require 'package) is still required due to the use of package functions.

EDIT: I gather the issue was with package-get-version was with the builtin version.

[jcs090218]

I've updated the code. This is much more robust now. :)

Out of curiosity what was the issue with using package-get-version?

Okay, I've done some tests and I think is the bug from Emacs itself. 🤔 At least, package-get-version isn't as robust as package-buffer-info.

[jkl1337]

I would suggest squashing the commits, as it is very hard to follow with the formatting changes going on and then reverting, I would avoid using s-replace, that is just waiting to blow up when somebody has their emacs dir in a patch containing .elc. Use these: https://www.gnu.org/software/emacs/manual/html_node/elisp/File-Name-Components.html eg (file-name-with-extension (file-name-sans-extension elc) "el")

EDIT: file-name-with-extension is Emacs 28. Current updated code 👍

[jcs090218]

FYI, I don't have the push permission so I don't have the control over this.

I would avoid using s-replace, that is just waiting to blow up when somebody has their emacs dir in a patch containing .elc. Use these: https://www.gnu.org/software/emacs/manual/html_node/elisp/File-Name-Components.html eg (file-name-with-extension (file-name-sans-extension elc) "el")

Thanks for the suggestion! Updated! 👍

[jkl1337]

FYI, I don't have the push permission so I don't have the control over this.

Are you speaking about squashing the commits? You can always squash commits on your local repo and force push to your own github repo, the PR will update.

[jcs090218]

There are corners cases where this can still happen, (like not having the .el file or it's compressed to .el.gz).

Yeah, that's why I go for package descriptor as my first attempt. But what makes you think this is a hack? 🤔 And out of curiosity, when will .el.gz happen? At some point, we have to assume users have installed their packages correctly.

However, at the least, if the version check returns nil, probably should assume the dependency is installed.

Thanks for the suggestion. I am keen to keep this PR like this for now because I have another PR in #247, and it will overwrite some of the logic here. Therefore, I will improve this after #247 gets merged.

[jkl1337]

Yeah, that's why I go for package descriptor as my first attempt. But what makes you think this is a hack? 🤔

Doing a version check using mechanisms outside of a package manager is unfortunately a hack. This is partly due to how packaging in emacs works. It is unfortunately the situation. How this is supposed to work is that copilot.el would be a proper package and dependencies would be handled for it. Failing that, probing for the specific feature in question (ie trap if the instance creation fails) would be more ideal, though I am aware that takes more effort. I have nearly 400 packages in my setup and I have not seen one resort to package-buffer-info or similar outside of tools like el-get, etc.

And out of curiosity, when will .el.gz happen?

So it is perfectly fine to have your el files compressed, and emacs works seamlessly. https://www.gnu.org/software/emacs/manual/html_node/emacs/Compressed-Files.html You also don't need to have the el files present.

At some point, we have to assume users have installed their packages correctly.

Exactly, but having compressed sources are compatible with correctly installed packages. So I am saying for features like these in general that are mostly intended as an imperfect fallback, if the check fails, it should soft fail optimistically. So someone that has their sources compressed or some non-typical thing, yeah they are on their own as far as versions, but at least it doesn't just break.

[jcs090218]

Doing a version check using mechanisms outside of a package manager is unfortunately a hack. This is partly due to how packaging in emacs works. It is unfortunately the situation. How this is supposed to work is that copilot.el would be a proper package and dependencies would be handled for it. Failing that, probing for the specific feature in question (ie trap if the instance creation fails) would be more ideal, though I am aware that takes more effort. I have nearly 400 packages in my setup and I have not seen one resort to package-buffer-info or similar outside of tools like el-get, etc.

Yes, you are right. Any improvements are welcome since I don't have a better solution.

So it is perfectly fine to have your el files compressed, and emacs works seamlessly. https://www.gnu.org/software/emacs/manual/html_node/emacs/Compressed-Files.html You also don't need to have the el files present.

👍

Exactly, but having compressed sources are compatible with correctly installed packages. So I am saying for features like these in general that are mostly intended as an imperfect fallback, if the check fails, it should soft fail optimistically. So someone that has their sources compressed or some non-typical thing, yeah they are on their own as far as versions, but at least it doesn't just break.

Thanks for the information. It's very helpful. I'll make improvements after other PRs have merged, and feel free to open PRs for this.

[jkl1337]

Ok, after getting some time to examine the actual problem leading to this I think this can be handled with a trap on invalid-slot-name. PR #264.

[jcs090218]

I think you have the better solution. Close this one now! :)