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 04c4c578c7 3/4: Allow for packages to be installed directly from VCS Date: Mon, 14 Feb 2022 20:57:48 +0000 Message-ID: <87bkz9tbcj.fsf@posteo.net> 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="14727"; mail-complaints-to="usenet@ciao.gmane.io" Cc: emacs-devel@gnu.org To: Stefan Monnier Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Mon Feb 14 21:58:57 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 1nJiQy-0003fV-Ku for ged-emacs-devel@m.gmane-mx.org; Mon, 14 Feb 2022 21:58:56 +0100 Original-Received: from localhost ([::1]:53252 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1nJiQv-0001VS-J0 for ged-emacs-devel@m.gmane-mx.org; Mon, 14 Feb 2022 15:58:54 -0500 Original-Received: from eggs.gnu.org ([209.51.188.92]:51378) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1nJiQ4-0000gq-RU for emacs-devel@gnu.org; Mon, 14 Feb 2022 15:58:00 -0500 Original-Received: from mout02.posteo.de ([185.67.36.66]:59703) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1nJiPw-0002Vr-IN for emacs-devel@gnu.org; Mon, 14 Feb 2022 15:58:00 -0500 Original-Received: from submission (posteo.de [185.67.36.169]) by mout02.posteo.de (Postfix) with ESMTPS id AC55E240104 for ; Mon, 14 Feb 2022 21:57:50 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=posteo.net; s=2017; t=1644872270; bh=RqekYF0cULv28nX21kbvnFY34I0n7TejS36kSS6yrHU=; h=From:To:Cc:Subject:Autocrypt:Date:From; b=CQP0H+TJ6Eo28afULr5iqVHq/5XBJZkh5mql4c5sb5S4DAFKjwW1do6LPdiWlDsfa 6msTbMdaapCq/dbLH60ILheh9a6OKzwNOuq9u7tERQIgEqLRwgB4w65308zPook0UC 69D26eInmq30L0zNKNjFg4mmEnWNprlyy2vJedDeT/5qVKuxz3pmlW7wksAhanzDIu TkhSDDDMDk+VB7rmtNsAzud41Q7/q292GS8li1bZYY+KLIT2NRLzCMlN6IZX25wIS0 9PppLf5I9trIXgWv9bJRfrEbILxx3seAlP9qXQbAoDLZH+4UpEQgD8qVpd01maJA8z UkZzOliewZPlw== Original-Received: from customer (localhost [127.0.0.1]) by submission (posteo.de) with ESMTPSA id 4JyGl16PCPz6tm5; Mon, 14 Feb 2022 21:57:49 +0100 (CET) Autocrypt: addr=philipk@posteo.net; prefer-encrypt=nopreference; keydata= mDMEYHHqUhYJKwYBBAHaRw8BAQdAp3GdmYJ6tm5McweY6dEvIYIiry+Oz9rU4MH6NHWK0Ee0QlBo aWxpcCBLYWx1ZGVyY2ljIChnZW5lcmF0ZWQgYnkgYXV0b2NyeXB0LmVsKSA8cGhpbGlwa0Bwb3N0 ZW8ubmV0PoiQBBMWCAA4FiEEDM2H44ZoPt9Ms0eHtVrAHPRh1FwFAmBx6lICGwMFCwkIBwIGFQoJ CAsCBBYCAwECHgECF4AACgkQtVrAHPRh1FyTkgEAjlbGPxFchvMbxzAES3r8QLuZgCxeAXunM9gh io0ePtUBALVhh9G6wIoZhl0gUCbQpoN/UJHI08Gm1qDob5zDxnIHuDgEYHHqUhIKKwYBBAGXVQEF AQEHQNcRB+MUimTMqoxxMMUERpOR+Q4b1KgncDZkhrO2ql1tAwEIB4h4BBgWCAAgFiEEDM2H44Zo Pt9Ms0eHtVrAHPRh1FwFAmBx6lICGwwACgkQtVrAHPRh1Fw1JwD/Qo7kvtib8jy7puyWrSv0MeTS g8qIxgoRWJE/KKdkCLEA/jb9b9/g8nnX+UcwHf/4VfKsjExlnND3FrBviXUW6NcB Received-SPF: pass client-ip=185.67.36.66; envelope-from=philipk@posteo.net; helo=mout02.posteo.de X-Spam_score_int: -43 X-Spam_score: -4.4 X-Spam_bar: ---- X-Spam_report: (-4.4 / 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_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:286298 Archived-At: Stefan Monnier writes: >> +(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`. Ok. >> @@ -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. I guess the question here is if you want to make it explicit that e.g. when removing a package with package-delete, you will be removing a local checkout that might have modifications. If not, it would probably make sense to add warnings where necessary. >> +(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? If you pass the backend directly to vc-working-revision, and set the file to the expanded file name of the current directory, it seems to work (at least for git and SVN). Would it make sense to add that as a first step, then fall back to the 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? Currently the :kind is not stored, so we have to infer it somehow. I am certain this could also be solve some other way, but I would also like to allow for simply placing a directory into elpa/devel to be handled as a package, as before with my site-lisp.el proposal. >> + ;; 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`. That was my first thought too, but are we always certain that .el is the main file? > 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. I will experiment with how viable this is. > 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. Currently, I don't expect it to grow too much beyond the current proposal. If it does turn out that the missing features are too large, pulling it all out into something like package-vc.el/package-extra.el seems reasonable (and perhaps perhaps even fit for distribution via ELPA). > 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? That would also be possible, I believe I initially assumed that :vc would contain the contents of the header, and forgot to revise that assumption later on. > > Stefan > -- Philip Kaludercic