From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Philip Kaludercic Newsgroups: gmane.emacs.devel Subject: Re: feature/package-vc has been merged Date: Sun, 06 Nov 2022 11:43:15 +0000 Message-ID: <87fsew8g18.fsf@posteo.net> References: <164484721900.31751.1453162457552427931@vcs2.savannah.gnu.org> <874jvqv2u3.fsf@posteo.net> <875yg6qtbl.fsf@posteo.net> <87ilk33lqk.fsf@posteo.net> <87mt9epqlk.fsf@posteo.net> <87ilk1bgvd.fsf@posteo.net> <87edupbdp0.fsf@posteo.net> <875yg1bc02.fsf@posteo.net> <878rkxgpms.fsf@posteo.net> <87sfiyk3a2.fsf_-_@posteo.net> <838rkp4ptj.fsf@gnu.org> <87zgd58i7y.fsf@posteo.net> <83k0492u5i.fsf@gnu.org> Mime-Version: 1.0 Content-Type: text/plain Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="7467"; mail-complaints-to="usenet@ciao.gmane.io" Cc: monnier@iro.umontreal.ca, rms@gnu.org, emacs-devel@gnu.org To: Eli Zaretskii Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Sun Nov 06 12:44:31 2022 Return-path: Envelope-to: ged-emacs-devel@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1ore4j-0001hj-N0 for ged-emacs-devel@m.gmane-mx.org; Sun, 06 Nov 2022 12:44:29 +0100 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1ore3s-0004mQ-2z; Sun, 06 Nov 2022 06:43:36 -0500 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1ore3q-0004mI-Qp for emacs-devel@gnu.org; Sun, 06 Nov 2022 06:43:34 -0500 Original-Received: from mout01.posteo.de ([185.67.36.65]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1ore3o-00081y-BB for emacs-devel@gnu.org; Sun, 06 Nov 2022 06:43:34 -0500 Original-Received: from submission (posteo.de [185.67.36.169]) by mout01.posteo.de (Postfix) with ESMTPS id 52210240026 for ; Sun, 6 Nov 2022 12:43:27 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=posteo.net; s=2017; t=1667735010; bh=/e0K+FLmxfdpKFZeLw0JNsNxIZAGZ6+VQqu4GpAOhQI=; h=From:To:Cc:Subject:Autocrypt:Date:From; b=Zoz7wAMX6S0Z+/HNnLomR1r7j+cUVGb0TrxsbWB8lCvONXZ3QsS4LorRA8nfLYPAa n0PNvCgGMAeBmXrclLm07cpy7ikwvW+eY929h9iwBXBtSq4n1V+xaasGW7NwgJyvRE USeOk4Foa8MGmwt9RddhZOwUg9OfTbYl0GLb3bSw1tngeDzTeqgi/8WDxzquwoSuxC A3QRytH0bHjdA71Mad1Pu3l9HJYHiHhvb29SHinBmit4uqilR4wthEXTBHucGackJ5 cMLG9AZCEzzwVu+5vBAthPaL/BLv9B5/QhgnS+UJhWZCg/H/vRksT7FkxaYvBxKAXd 5rU/zTrJj3kkw== Original-Received: from customer (localhost [127.0.0.1]) by submission (posteo.de) with ESMTPSA id 4N4sv31LGdz6tmF; Sun, 6 Nov 2022 12:43:24 +0100 (CET) In-Reply-To: <83k0492u5i.fsf@gnu.org> (Eli Zaretskii's message of "Sat, 05 Nov 2022 19:22:33 +0200") Autocrypt: addr=philipk@posteo.net; keydata= mQGNBGLfygUBDADVznbke6w0n9nE42xb+ZggbBy0IYRkkru/K+NA67523YTl2DoR2a5OMW90w7L9 KDtX2Mp34JN/6jVOSVC07VUbHVu6/exoGKixkiTpGhBPy5tUUJoxQKqLrzVQhN3fIyvg1oyHXKZm QGkUeevV0wjj4++xfjmcP235YvDh3TF8HC9t5KxIQIbhWnQm4ZyDkpWWS2CmdNttlj2+eH+51WLL bgx2bcwTmqrs079Q3hgF3yh44bBEmp9MgFjiZldOY2my0/ZSeucRxYmiM0vbJEBQgZV/MvA3gTxe 7ibV3ii7AyoYA8FiFDP98S/R2y5Nfq3ez9B7qeqtpSNseQHOU7h8Y5VV01a71ZszENAmbbwsldb9 j+HRLke7rn6mswDZl1qA/9ZFRzliFOdQtS1878XjraY+h5jfjvxaFVK23prGGVrrKv0LPWavoFUr nsjeHEZhYezBKhC2PwvRtXm01S3rkNbwm9pj0tfLSDW+1pT+6eZWptfQCXF2oEvgfKSTASUAEQEA AbQmUGhpbGlwIEthbHVkZXJjaWMgPHBoaWxpcGtAcG9zdGVvLm5ldD6JAdQEEwEKAD4WIQRxJuHe LwzjXHcL7QHyw8xRPbifZgUCYt/KBQIbAwUJA8JnAAULCQgHAgYVCgkICwIEFgIDAQIeAQIXgAAK CRDyw8xRPbifZkH+DACmCKmhrYgcv2i6dj3vRCVINaLtKUODTna/wAmP20WRKPhqvqvKNUx/wzpT aZrXIxpxOU2xawRWeHhWUktxS+W9L3xTACeR0gf5gomCxD9RuBTIohzWDkQt5rk8QwLqx5rAy5 Received-SPF: pass client-ip=185.67.36.65; envelope-from=philipk@posteo.net; helo=mout01.posteo.de X-Spam_score_int: -20 X-Spam_score: -2.1 X-Spam_bar: -- X-Spam_report: (-2.1 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, RCVD_IN_DNSWL_NONE=-0.0001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Original-Sender: "Emacs-devel" Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Xref: news.gmane.io gmane.emacs.devel:299249 Archived-At: Eli Zaretskii writes: >> From: Philip Kaludercic >> Cc: monnier@iro.umontreal.ca, rms@gnu.org, emacs-devel@gnu.org >> Date: Sat, 05 Nov 2022 16:43:45 +0000 >> >> >> (defcustom package-vc-repository-store >> >> (expand-file-name "emacs/vc-packages" (xdg-data-home)) >> >> "Directory used by `package-vc--unpack' to store repositories." >> > >> > The doc string of a user option should not reference an internal >> > function, it should reference user-visible features. Would it be >> > correct to say that this directory is used by package-vc to store >> > repositories of packages it fetches and/or installs? >> >> You are right, I forgot about this when renaming package-vc-unpack to >> package-vc--unpack. How about >> >> "Directory used to store clone repositories. >> >> This directory is only used for packages with a special `:lisp-dir' >> entry in their package specification (for details on package >> specifications see `package-vc-selected-packages'). Repositories for >> packages like these are cloned into the directory specified here, and >> their `:lisp-dir' is then linked back into the user elpa directory." > > Instead of "specified here", please use "specified by this variable". > And I don't think I understand what the "their `:lisp-dir' is then > linked back into the user elpa directory" part wants to say. > I also have a more general question: you say this "is only used for > packages with a special `:lisp-dir' entry", and that begs the > question: where will we clone a package that doesn't have the > :lisp-dir entry? Directly into the elpa directory. >> >> (defcustom package-vc-selected-packages '() >> >> "List of packages that must be installed. >> >> Each member of the list is of the form (NAME . SPEC), where NAME >> >> is a symbol designating the package and SPEC is one of: >> >> >> >> - nil, if any package version can be installed; >> >> - a version string, if that specific revision is to be installed; >> >> - a property list of the form described in >> >> `package-vc-archive-spec-alist', giving a package >> >> specification. >> > >> > There's no variable package-vc-archive-spec-alist. Did you mean >> > package-vc--archive-spec-alist instead? In that case, I think the >> > format of that list should be spelled out in this doc string, as the >> > referenced variable is an internal one. >> >> You are right (same mistake as before), the documentation for package >> specifications should be moved from the internal variable to either >> 'package-vc-selected-packages' or some other place. It might not be bad >> to document it in package.texi either? Or would that be too Lisp-y for >> the Emacs manual? > > It's okay to have this in package.texi, but perhaps with fewer > details. Users should customize such complex variables via Custom > anyway. OK, I will think about this. >> >> This user option differs from `package-selected-packages' in that >> >> it is meant to be specified manually. You can also use the >> >> function `package-vc-selected-packages' to apply the changes." >> > >> > The function package-vc-selected-packages mentioned in the last >> > sentence doesn't seem to exist. Also, what does it mean "to apply the >> > changes" -- apply the changes to what? >> >> It was supposed to be `package-vc-ensure-packages'. How about >> rephrasing that last sentence into "By calling the function >> `package-vc-selected-packages", the packages specified in this list can >> also be installed. > > Just avoid the passive voice: > > If you want to also install the packages in this list, use > `package-vc-ensure-packages'. Done. >> >> (defun package-vc--build-documentation (pkg-desc file) >> >> "Build documentation FILE for PKG-DESC." >> >> (let ((pkg-dir (package-desc-dir pkg-desc))) >> >> (when (string-match-p "\\.org\\'" file) >> >> (require 'ox) >> >> (require 'ox-texinfo) >> >> (with-temp-buffer >> >> (insert-file-contents file) >> >> (setq file (make-temp-file "ox-texinfo-")) >> >> (org-export-to-file 'texinfo file))) >> >> (call-process "install-info" nil nil nil >> >> file pkg-dir))) >> > >> > I'm confused by this function. Does org-export-to-file produce a >> > .texi file or an Info file? In any case, the semantics is confusing: >> > if FILE has the .org extension, the function installs a file whose >> > name is not FILE, otherwise the function installs FILE. This seems to >> > conflate source and destination files in a confusing way. >> >> I can expand the documentation here. >> >> "file" is the input, either a .texi or an .org file. > > If FILE is input, then saying "Build documentation FILE" is > misleading: there's no need to build FILE, it already exists. You > should say something like > > Build documentation for package PKG-DESC from documentation source in FILE. Right, done. >> If it is an .org file, we want to convert it to .texi first (which >> is what the (when ...) block does), and write the output into a >> temporary file. The temporary file is used as an input to >> install-info, that builds the .info file. > > Ah, so there's a problem here, I think. install-info doesn't produce > Info files, it only installs existing Info files and updates the DIR > files with the entries of the installed Info files. So the step of > producing Info files from Texinfo is missing here. Oh yes, I have to add a makeinfo call somewhere inbetween. The code was based on elpa-admin.el, and it seems I misread it. >> >> (defun package-vc-update (pkg-desc) >> >> "Attempt to update the package PKG-DESC." >> >> ;; HACK: To run `package-vc--unpack-1' after checking out the new >> >> ;; revision, we insert a hook into `vc-post-command-functions', and >> >> ;; remove it right after it ran. To avoid running the hook multiple >> >> ;; times or even for the wrong repository (as `vc-pull' is often >> >> ;; asynchronous), we extract the relevant arguments using a pseudo >> >> ;; filter for `vc-filter-command-function', executed only for the >> >> ;; side effect, and store them in the lexical scope. When the hook >> >> ;; is run, we check if the arguments are the same (`eq') as the ones >> >> ;; previously extracted, and only in that case will be call >> >> ;; `package-vc--unpack-1'. Ugh... >> > >> > Shouldn't this use unwind-protect, to make sure the hacked >> > post-command-hook doesn't leak out? >> >> The issue is that vc-pull *can be* asynchronous, so wrapping it in a >> `unwind-protect' would un-modify the hook too soon. But you are right >> that if vc-pull were to fail, the hook would remain inside of >> `vc-post-command-functions' for too long. >> >> As I said in that comment >> >> "If there is a better way to do this, it should be done." > > If you can use unwind-protect at least in the synchronous case, it > would be better, I think. The issue is that vc-pull doesn't indicate if the command was run asynchronously or not. > And if the async pull could take a long time, I think it's a problem > to have the additional entry in the post-command-hook for all that > time. That is why the function tries to identify who invoked the hook and only run it (once) in that case. For this to be done properly the 'pull' method for VC would probably have to be changed/replaced to make programmatic invocations more predictable.