From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Stefan Monnier Newsgroups: gmane.emacs.devel Subject: Re: feature/package+vc 04c4c578c7 3/4: Allow for packages to be installed directly from VCS Date: Mon, 14 Feb 2022 11:20:32 -0500 Message-ID: References: <164484721900.31751.1453162457552427931@vcs2.savannah.gnu.org> <20220214140020.04438C00891@vcs2.savannah.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="9612"; mail-complaints-to="usenet@ciao.gmane.io" User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/29.0.50 (gnu/linux) Cc: Philip Kaludercic To: emacs-devel@gnu.org Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Mon Feb 14 17:24:50 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 1nJe9h-0002HR-Fx for ged-emacs-devel@m.gmane-mx.org; Mon, 14 Feb 2022 17:24:49 +0100 Original-Received: from localhost ([::1]:49142 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1nJe9g-00040v-2Y for ged-emacs-devel@m.gmane-mx.org; Mon, 14 Feb 2022 11:24:48 -0500 Original-Received: from eggs.gnu.org ([209.51.188.92]:34408) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1nJe5k-0000j1-Pz for emacs-devel@gnu.org; Mon, 14 Feb 2022 11:20:45 -0500 Original-Received: from mailscanner.iro.umontreal.ca ([132.204.25.50]:6335) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1nJe5h-0006Jy-IJ for emacs-devel@gnu.org; Mon, 14 Feb 2022 11:20:44 -0500 Original-Received: from pmg1.iro.umontreal.ca (localhost.localdomain [127.0.0.1]) by pmg1.iro.umontreal.ca (Proxmox) with ESMTP id B0AA810018C; Mon, 14 Feb 2022 11:20:36 -0500 (EST) Original-Received: from mail01.iro.umontreal.ca (unknown [172.31.2.1]) by pmg1.iro.umontreal.ca (Proxmox) with ESMTP id 9221C10008C; Mon, 14 Feb 2022 11:20:33 -0500 (EST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=iro.umontreal.ca; s=mail; t=1644855633; bh=/doYYqFRo8Q7kIuZuZRX9duIGUpPvYCv4WroOJeQbGg=; h=From:To:Cc:Subject:References:Date:In-Reply-To:From; b=S+YmOjTL9eang3FeQcuzZlORPrt2V2b2QA0XaSLQyr9cVeyN0hRtSxRMB2ykLkL2l 6guVHFSo0IzfdFDaczgG/5SOPareR+5udHrKII1nwFS0P6sqOO4Dk7cBJ2JG0BTxb+ GX1DOyLg583wn4AkBGB7sShPMg+gc/e9XudkdxuBE9s4wKG1Y/IA3qQZUB8HYzrMlc LbyBcMHdlqUzjUtMJZV2aOMQsSNrCSjXlu5sNgyUNwvOU28AJwX2NKRVy7trXP+I3f EotsVsi4OWnf2kb0JcMoNGkvf51zvwXjDHvwvLPyRd5tkurw+6M7nxNgcgB8qNReRc wgXgJW9UKR6Iw== Original-Received: from pastel (unknown [45.72.237.157]) by mail01.iro.umontreal.ca (Postfix) with ESMTPSA id 7159512036E; Mon, 14 Feb 2022 11:20:33 -0500 (EST) In-Reply-To: <20220214140020.04438C00891@vcs2.savannah.gnu.org> (Philip Kaludercic's message of "Mon, 14 Feb 2022 09:00:19 -0500 (EST)") Received-SPF: pass client-ip=132.204.25.50; envelope-from=monnier@iro.umontreal.ca; helo=mailscanner.iro.umontreal.ca X-Spam_score_int: -42 X-Spam_score: -4.3 X-Spam_bar: ---- X-Spam_report: (-4.3 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, RCVD_IN_DNSWL_MED=-2.3, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, T_SCC_BODY_TEXT_LINE=-0.01 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: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Original-Sender: "Emacs-devel" Xref: news.gmane.io gmane.emacs.devel:286284 Archived-At: > Allow for packages to be installed directly from VCS Yay! > +(defcustom package-devel-dir (expand-file-name "devel" package-user-dir) > + "Directory containing the user's Emacs Lisp package checkouts. > +The directory name should be absolute. > +Apart from this directory, Emacs also looks for system-wide > +packages in `package-directory-list'." > + :type 'directory > + :initialize #'custom-initialize-delay > + :set-after '(package-user-dir) > + :risky t > + :version "29.1") Since this is not autoloaded, it should not need (nor want) `custom-initialize-delay`. > @@ -560,7 +571,9 @@ This is, approximately, the inverse of `version-to-list'. > This is the name of the package with its version appended." > (format "%s-%s" > (package-desc-name pkg-desc) > - (package-version-join (package-desc-version pkg-desc)))) > + (if (eq (package-desc-kind pkg-desc) 'source) > + "devel" > + (package-version-join (package-desc-version pkg-desc))))) Currently the way `package.el` handles the use of Git-installed packages is that it allows a package's directory to be just `` instead of `-`. So maybe we could do the same here. > +(declare-function vc-working-revision "vc" (file &optional backend)) > +(defun package-devel-commit (pkg) > + "Extract the commit of a development package PKG." > + (cl-assert (eq (package-desc-kind pkg) 'source)) > + (require 'vc) > + (cl-loop with dir = (package-desc-dir pkg) > + for file in (directory-files dir t "\\.el\\'" t) > + when (vc-working-revision file) return it > + finally return "unknown")) Any chance we could use `vc-working-revision` on the package's root directory to avoid this loop? > @@ -681,6 +704,14 @@ return it." > (read (current-buffer))) > (error "Can't find define-package in %s" pkg-file)))) > (setf (package-desc-dir pkg-desc) pkg-dir) > + (when (file-exists-p (expand-file-name > + (symbol-name (package-desc-name pkg-desc)) > + package-devel-dir)) > + ;; XXX: This check seems dirty, there should be a better > + ;; way to deduce if a package is in the devel directory. > + (setf (package-desc-kind pkg-desc) 'source) > + (push (cons :commit (package-devel-commit pkg-desc)) > + (package-desc-extras pkg-desc))) > (if (file-exists-p signed-file) > (setf (package-desc-signed pkg-desc) t)) > pkg-desc))))) Hmm... why do we need to know if a package is in the devel directory? > + ;; In case the package was installed directly from source, the > + ;; dependency list wasn't know beforehand, and they might have > + ;; to be installed explicitly. > + (let (deps) > + (dolist (file (directory-files pkg-dir t "\\.el\\'" t)) > + (with-temp-buffer > + (insert-file-contents file) > + (when-let* ((require-lines (lm-header-multiline "package-requires"))) > + (thread-last > + (mapconcat #'identity require-lines " ") > + package-read-from-string > + package--prepare-dependencies > + (nconc deps) > + (setq deps))))) Hmm... I expected here we'd open the `.el` file and call `package-buffer-info`. Also, I'm wondering if `package-unpack` is the right place to put this code. Maybe it should be in a completely separate function: `package-unpack` takes a buffer containing the package, whereas for those VCS-packages we start from something quite different, and the equivalent to the `pkg-desc` we pass for installation should be fairly different as well (should more similar to the "package specs" used in `elpa-admin.el`). So I think it'll want a completely separate code path. IOW, I'm not sure `package-fetch` should call `package-install`. I suspect it would work as well or better if it did most of the installation with its own fresh new code, and only hooks into the rest of the code once the files are extracted and a `-pkg.el` was generated. Also, unless the new code is small, it should likely live in another file. `package.el` tends to get loaded at startup in all sessions (unless you use `package-quickstart`), so we should try and trim it down rather than grow it. Most of its code (`package-install`, `list-packages`, ...) should be in a separate file from the one that provides `package-activate-all`, I think. Admittedly, maybe the better way is not to move `package-install` out into a `package-extra.el` but rather to move `package-activate-all` to a preloaded `package-hooks.el`. > + ((when-let* ((desc (cadr (assoc name-or-url package-archive-contents > + #'string=))) > + (spec (or (alist-get :vc (package-desc-extras desc)) > + (user-error "Package has no VC header")))) Hmmm... so IIUC you intend to change `elpa-admin.el` to include a `:vc` extra info in the `archive-contents` file for every package? Any reason to make that a plain string (that we then need to parse back into a list) rather than a list? Stefan