-
-
Notifications
You must be signed in to change notification settings - Fork 391
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
Comments
Przemysław Kryger ***@***.***> writes:
1. ( ) text/plain (*) text/html
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.
Ok, thanks I will look into this ASAP, I am not familiar with
package-vc, probably it should be enough to prevent upgrading packages
installed with package-vc?
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?
0. Fresh emacs installation, no configuration at all
1. Clone repository: git clone https://github.com/emacs-helm/helm ~/helm
2. Start Emacs: emacs -Q
3. 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")
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.
… (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
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.*Message ID: ***@***.***>
--
Thierry
|
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
|
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.
Unfortunately after installing this patch and calling |
Przemysław Kryger ***@***.***> writes:
1. ( ) text/plain (*) text/html
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.
Argh! yes of course. Can you try this instead:
diff --git a/helm-packages.el b/helm-packages.el
index be01b3c1..7cb5099f 100644
--- a/helm-packages.el
+++ b/helm-packages.el
@@ -327,13 +327,13 @@ Arg PACKAGES is a list of strings."
for pkg = (assq sym package-archive-contents)
for cversion = (and pkg (package-desc-version desc))
for available = (and pkg (not (package-disabled-p sym cversion)) pkg)
- when (or (and available
- (or (and include-builtins (not cversion))
- (and cversion
- (version-list-<
- cversion
- (package-desc-version (cadr available))))))
- (and (fboundp 'package-vc-p) (package-vc-p desc)))
+ when (and available
+ (and (fboundp 'package-vc-p) (not (package-vc-p desc)))
+ (or (and include-builtins (not cversion))
+ (and cversion
+ (version-list-<
+ cversion
+ (package-desc-version (cadr available))))))
collect sym)))
;;;###autoload
In the minimal example that wold be async and 0blayout, but on my
regular emacs it's plenty more.
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.
… —
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you commented.*Message ID: ***@***.***>
--
Thierry
|
No this patch is wrong as well, we should do the same as what |
@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 Have you considered using similar technique? |
Przemysław Kryger ***@***.***> writes:
1. ( ) text/plain (*) text/html
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?
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.
When I call helm-packages I have now a source for upgrading helm!? and a source for deleting helm
dependencies!?, that is helm-core, async and wfnames and the usual sources
listing all other packages.
This is obviously wrong, but I for now don't know what to expect from
package-vc i.e. I am not sure what is it supposed to do (most of my
packages are installed manually from sources).
I will look as soon as possible in package.el and package-vc.el to
understand what they are supposed to do.
…--
Thierry
|
Perhaps it's a good idea of simplifying the issue using a package that is (should?) be compatible with Cheers, |
Przemysław Kryger ***@***.***> writes:
1. ( ) text/plain (*) text/html
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.
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.
Let me know if it is ok for you.
Thanks.
…--
Thierry
|
I also use
I think that will be a very reasonable approach. I guess I'd rather manually I guess a new functionality (a new section? a new Cheers, PK |
Przemysław Kryger ***@***.***> writes:
1. ( ) text/plain (*) text/html
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.
Looking for seeing your workflows with package-vc, perhaps I will
understand better its usefulness :-).
Thanks.
…--
Thierry
|
What happened?
When a package is installed using
package-vc-install-from-checkout
the very next invocation ofhelm-packages
shows the package as available to be upgraded, and should user approve the upgrade it will proceed clobbering installation from VC. In contrastpackage-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?
git clone https://github.com/emacs-helm/helm ~/helm
emacs -Q
Helm Version
Master branch
Emacs Version
Emacs-30+
OS
GNU/Linux
Relevant backtrace (if possible)
No response
Minimal configuration
The text was updated successfully, but these errors were encountered: