From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Eli Zaretskii Newsgroups: gmane.emacs.devel Subject: Re: feature/package-vc has been merged Date: Sat, 05 Nov 2022 13:13:12 +0200 Message-ID: <838rkp4ptj.fsf@gnu.org> References: <164484721900.31751.1453162457552427931@vcs2.savannah.gnu.org> <87zgdjqcu0.fsf@posteo.net> <87zgdivc3f.fsf@posteo.net> <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> Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="28810"; mail-complaints-to="usenet@ciao.gmane.io" Cc: monnier@iro.umontreal.ca, rms@gnu.org, emacs-devel@gnu.org To: Philip Kaludercic Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Sat Nov 05 12:14:25 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 1orH84-0007Ne-Ov for ged-emacs-devel@m.gmane-mx.org; Sat, 05 Nov 2022 12:14:24 +0100 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1orH7K-0000aF-A2; Sat, 05 Nov 2022 07:13:38 -0400 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 1orH7I-0000a3-R3 for emacs-devel@gnu.org; Sat, 05 Nov 2022 07:13:36 -0400 Original-Received: from fencepost.gnu.org ([2001:470:142:3::e]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1orH7I-0000qS-GZ; Sat, 05 Nov 2022 07:13:36 -0400 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=gnu.org; s=fencepost-gnu-org; h=References:Subject:In-Reply-To:To:From:Date: mime-version; bh=MLSYGVHF1zzIuSD62NHbgXrjUDTfmjtF516izo+l6uo=; b=KRuIKy/t8lzo kqKthr2PQBzT9XMM0UlFFug6yGRyoyhMmSPR/6YvfM/NleCJrjrppWzsHsVphJ4A6cIMbX5nrz1We kk557D4696ySq0XFvXbs7w52G6Rzbs7OecGWH60KqGhCyQhbEhArVPdmT+Zevw+2E4uVj2Ozuh0eD srnoqfRptRC6W496rrpDXkdIDHlhfBZ1jv1sgvE4Ch0e1yshFsJG5NtxItaaXRNBDGtd0Hg7rXOk0 2VVuOZf4M5cheXA6rB7MhpCERxhjFJ7hz19OZfLwF4pdYCMN0Ovt/McAiiJqpTSlDds7bt9V+FCXb 6+LEveOJU5AZRDm6VEQdFg==; Original-Received: from [87.69.77.57] (helo=home-c4e4a596f7) by fencepost.gnu.org with esmtpsa (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1orH6x-0008Iv-0p; Sat, 05 Nov 2022 07:13:15 -0400 In-Reply-To: <87sfiyk3a2.fsf_-_@posteo.net> (message from Philip Kaludercic on Fri, 04 Nov 2022 18:01:09 +0000) 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:299183 Archived-At: > From: Philip Kaludercic > Cc: Richard Stallman , emacs-devel@gnu.org > Date: Fri, 04 Nov 2022 18:01:09 +0000 > > Stefan Monnier writes: > > >> OK, I've merged master into feature/package+vc, resolving all conflicts > >> and would be prepared to merge feature/package+vc if there are no > >> further objections. > > > > No objections on my side, > > I have merged the branch onto master, so if anyone wants to fix, > add/remove or improve anything, feel free to do so. Thanks. A few comments about the documentation (you will see that I fixed some of the issues where I could): > (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? > (defun package-vc-ensure-packages () > "Ensure source packages specified in `package-vc-selected-packages'." This doc string should explain what does this function ensure. "Ensure source" doesn't clarify that. I think, but cannot be sure, this should say something like Ensure packages specified in `package-vc-selected-packages' are installed." > (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. > 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? > (defvar package-vc--archive-spec-alist nil > "List of package specifications for each archive. > The list maps each package name, as a string, to a plist. > Valid keys include > > `:url' (string) The "Valid keys" part is "out of the blue" here. Keys of what? If that's a reference to the "plist" part preceding it, then we aren't talking about a plist, we are talking about keyword/value pairs, right? > (defun package-vc--version (pkg) > "Extract the commit of a development package PKG." This doc string doesn't seem to match what the function does. > (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. > (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?