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

Closed
1 task done
pkryger opened this issue Dec 6, 2024 · 17 comments
Closed
1 task done
Labels

Comments

@pkryger
Copy link
Contributor

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
Contributor 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
Contributor 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
Contributor 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
Contributor 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

@pkryger
Copy link
Contributor Author

pkryger commented Feb 3, 2025

Looking for seeing your workflows with package-vc, perhaps I will understand
better its usefulness :-).

Sorry it took so long to respond. Slipped through the cracks.

I think my usage of package-vc has been limited to just a couple of
workflows. I also tend to use :vc keyword of use-package (which uses
package-vc behind the scenes).

There's a couple packages that are not available through MELPA, like
ultra-scroll or transient-showcase. I use more or less:

(use-package ultra-scroll
  :vc (:url "https://github.com/jdtsmith/ultra-scroll.git" :rev :newest))

From time to time I pull a fresh version of the repository (git pull in
(file-name-concat package-user-directory "ultra-scroll")) followed by
package-vc-rebuild.

Another use case is for a few packages I develop, and I'd rather have them
cloned into ~/gh/pkryger/difftastic.el. For these I am using an equivalence
of:

(use-package difftastic
  :vc (:url "https://github.com/pkryger/difftastic.el.git" :rev :newest)
  :load-path "~/gh/pkryger/difftastic.el")

(Actually, I hide that behind
exordium-vc-checkout,
but ultimately, package-vc-install is used to install the package).

In the latter case I control which commit/branch has been checked out and run
package-vc-rebuild from time to time (especially after some development) to
have a fresh version of byte (and native!) compiled files.

Not sure if this is the most optimal workflow, but it somehow fits nicely into
my head for the dozen or so packages I manage (semi) manually.

@thierryvolpiatto
Copy link
Member

thierryvolpiatto commented Feb 3, 2025 via email

@pkryger
Copy link
Contributor Author

pkryger commented Feb 3, 2025

[...] What is different from the workflow you
are using? IOW what is the advantage of using package-vc?

These are very minor, and I think mostly a convenience differences. For
starters repository location can be stored in a config file (and, if someone
wishes to, they can specify a version to use, or version published on ELPA, not
the last one on a main branch). Also the load-path is updated
automatically. Also, the -autoloads.el is generated, so one can use them like
any other package (i.e., w/o require, relaying on autoloads). All of these can
of course be done manually, but for packages that are only available on one of
the forges (GitHub, GitLab, etc.) it is much easier to "just call a function"
rather than doing everyting manually.

@thierryvolpiatto
Copy link
Member

thierryvolpiatto commented Feb 4, 2025 via email

@pkryger
Copy link
Contributor Author

pkryger commented Feb 4, 2025

Not an advantage at least for me, IIUC when you store a version or branch in a
file, you use this version (tag) or branch and if you want to change,
package-vc have to clone again the repo to a new tag/branch.

Not necessarily...

With the workflow
I use I can checkout at any commit or switch to a branch, then recompile and I
am done.

You can just checkout different commit/branch in already cloned repository,
then package-vc-recompile and you're done as well. Autoloads will be
regenerated for you as well.

Also I tried use-package in the past but I had constantly to fix my config by
(macro) expanding the use-package to know what use-package did exactly. I
finally stopped using it, using instead with-eval-after-load and autoload which
are easier to use IMO than use-package and I know exacly what's going on.

I completely understand you. How use-package macroexpands can be surprising,
and certainly still is in some cases.

I also tried to use straight in the past, which have among other things the
same purpose as package-vc, [...]

I've heard about straight but has never tried it and from what I gather it's
similar to package-vc in some aspects. Yet, my lack of experience with the
straight prevents me from making any statements.

[...] I never found any advantages other my current workflow,
only complications.

I completely understand you! When there's a need to do something in a precise
way it's usually better to go imperative way. Especially when one has
experience and knows what exactly they want to achieve. I am not trying to
argue the package-vc is objectively better, just trying to highlight where it
can have appeal.

Anyway: now helm cooperates with package-vc. I think the issue can be
closed and I will just figure out adding affixations for completions.

@thierryvolpiatto
Copy link
Member

Thanks to enlighten me about package-vc, closing now.

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