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

helm-packages always show packages installed from VC as an upgrade option #2692

Open
1 task done
pkryger opened this issue Dec 6, 2024 · 11 comments
Open
1 task done
Labels

Comments

@pkryger
Copy link

pkryger commented Dec 6, 2024

What happened?

When a package is installed using package-vc-install-from-checkout the very next invocation of helm-packages shows the package as available to be upgraded, and should user approve the upgrade it will proceed clobbering installation from VC. In contrast package-list-packages does not show such a package as an upgrade option.

I am not sure that this is a bug, but I think this has a potential of accidentally overwriting VC installed packages when one wants to just sync with current ELPA packages. The latter category can be dozens of items and manual fishing out VC installed packages is a bit cumbersome and prone to mistakes.

I have not checked other VC installation methods (i.e., package-vc-install), but I suspect these will be affected in the same manner.

I can reproduce on emacs-mac 29.4 on macOS as well.

How to reproduce?

  1. Fresh emacs installation, no configuration at all
  2. Clone repository: git clone https://github.com/emacs-helm/helm ~/helm
  3. Start Emacs: emacs -Q
  4. Evaluate:
(require 'package)
(add-to-list 'package-archives '("melpa" . "https://melpa.org/packages/"))
(package-initialize)
(package-refresh-contents)
(package-install 'async) ; dependencies are not handled by package-vc

(require 'package-vc)
(package-vc-install-from-checkout "~/helm" "helm")

(cl-assert (package-installed-p 'helm))
(cl-assert (equal "vc"
		  (package-desc-kind (cadr (assq 'helm package-alist)))))
(cl-assert (equal (file-truename (file-name-as-directory "~/helm"))
		  (file-truename (file-name-directory (locate-library "helm")))))
(cl-assert (equal (file-truename (car load-path))
		  (file-truename "~/helm")))

(package-list-packages) ; type "U x" -> "No packages to upgrade"
(helm-packages) ; helm is shown as an upgrade option

Helm Version

Master branch

Emacs Version

Emacs-30+

OS

GNU/Linux

Relevant backtrace (if possible)

No response

Minimal configuration

  • I agree using a minimal configuration
@pkryger pkryger added the bug label Dec 6, 2024
@thierryvolpiatto
Copy link
Member

thierryvolpiatto commented Dec 6, 2024 via email

@thierryvolpiatto
Copy link
Member

Does this patch fix the issue for you?

diff --git a/helm-packages.el b/helm-packages.el
index be01b3c1..5b7a8d77 100644
--- a/helm-packages.el
+++ b/helm-packages.el
@@ -333,7 +333,7 @@ Arg PACKAGES is a list of strings."
                                     (version-list-<
                                      cversion
                                      (package-desc-version (cadr available))))))
-                      (and (fboundp 'package-vc-p) (package-vc-p desc)))
+                      (and (fboundp 'package-vc-p) (not (package-vc-p desc))))
              collect sym)))
 
 ;;;###autoload

@pkryger
Copy link
Author

pkryger commented Dec 6, 2024

I already discussed with the package-vc developer and installing
packages like helm is not supported by package-vc, this because helm has
only one repo so when package-vc tries to install helm-core it doesn't
find its repo because it is the same as helm.

I haven't thought about that. If it's worth anything I can see that's the case even for a simple packages pulled straight from repository. I just used helm as a minimal reproduction example.

Does this patch fix the issue for you?

diff --git a/helm-packages.el b/helm-packages.el
index be01b3c1..5b7a8d77 100644
--- a/helm-packages.el
+++ b/helm-packages.el
@@ -333,7 +333,7 @@ Arg PACKAGES is a list of strings."
                                     (version-list-<
                                      cversion
                                      (package-desc-version (cadr available))))))
-                      (and (fboundp 'package-vc-p) (package-vc-p desc)))
+                      (and (fboundp 'package-vc-p) (not (package-vc-p desc))))
              collect sym)))
 
 ;;;###autoload

Unfortunately after installing this patch and calling package-refresh-contents everything is shown as an upgrade option in helm-packages. In the minimal example that wold be async and 0blayout, but on my regular emacs it's plenty more.

@thierryvolpiatto
Copy link
Member

thierryvolpiatto commented Dec 6, 2024 via email

@thierryvolpiatto
Copy link
Member

No this patch is wrong as well, we should do the same as what package--upgradeable-packages does. So the problem comes from elsewhere.

@pkryger
Copy link
Author

pkryger commented Dec 7, 2024

Yes, I have no examples currently, I rarely use packages and don't use
package-vc at all, so I am trying to fix this blindly for now.

@thierryvolpiatto thank you very much for looking at this. I am not sure how much time I will have in the coming days. If I have some to spare I may try to give it a shot, and at least create a prototype solution and cut a PR.

FWIW, I have created the reproduction steps in Docker images. I find them really usefull tool when it comes to shuffling different Emacs versions and configuration. Being creative about what's mounted where allows me to just throw an executor (Emacs) on a bunch of code. I am using Silex/docker-emacs, with manually rebuild containers for Apple arch.

The last but not least, despite Helm structure not being supported by package-vc-install, I created the reproduction in the initial report using such a technique. After installing helm with package-vc-install the helm-packages was operational. At least to a point where it asked me if I want to upgrade helm.

Have you considered using similar technique?

@thierryvolpiatto
Copy link
Member

thierryvolpiatto commented Dec 7, 2024 via email

@pkryger
Copy link
Author

pkryger commented Dec 7, 2024

Just tried with package-vc-install-from-checkout , a symlink of the helm
directory is created in elpa directory, the helm-core package is installed from
melpa, same for wfnames and async. [...]

Perhaps it's a good idea of simplifying the issue using a package that is (should?) be compatible with package-vc-install-from-checkout? For example async. That way shenanigans from dependencies would not be meddling waters, while keeping installed packages small.

Cheers,
PK

@thierryvolpiatto
Copy link
Member

thierryvolpiatto commented Dec 7, 2024 via email

@pkryger
Copy link
Author

pkryger commented Dec 8, 2024

Yes it is what I did (used async). I see that package-menu--find-upgrades
excludes the packages installed from vc, if I make a commit in async and run U
in list-packages, async is not marked as upgradable package so I conclude the
unique interface to upgrade such packages is package-vc-upgrade(all). I have
therefore removed the package-vc from helm-package--upgradeable-packages.

I also use package-vc-rebuild for the few packages I tinker with in their
repositories

Let me know if it is ok for you.

I think that will be a very reasonable approach. I guess I'd rather manually
control when to upgrade things I have checked out. But perhaps that's because I
only have a few of these as opposed of dozens of packages from ELPAs.

I guess a new functionality (a new section? a new helm-vc-packages command?)
could be developed for VC stuff. But that sounds more like a feature request
rather than solving immediate issue reported here: inconsistency between
package-list-packages potentially leading to a confusion. I still need to
determine if I can transfer my workflows (also pretty limited) into a written
form, and if you think it's worth it I can create another issue/feature request.

Cheers,

PK

@thierryvolpiatto
Copy link
Member

thierryvolpiatto commented Dec 8, 2024 via email

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

2 participants