* bug#63671: Add function to test equality of hash tables
2023-05-25 6:31 5% ` Ihor Radchenko
2023-05-25 7:22 5% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2023-05-25 7:22 5% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
1 sibling, 0 replies; 63+ results
From: Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2023-05-25 7:22 UTC (permalink / raw)
To: Ihor Radchenko; +Cc: Mattias Engdegård, 63671
Ihor Radchenko <yantar92@posteo.net> writes:
> If we are serious about adding this feature, lets not close this report.
> My email was on emacs-devel, and it is thus less visible compared to
> something being tracked on debbugs.
I would be happy to see this feature added to Emacs! Unfortunately, I
won't be of much help right now since I have no C experience.
^ permalink raw reply [relevance 5%]
* bug#63671: Add function to test equality of hash tables
2023-05-25 6:31 5% ` Ihor Radchenko
@ 2023-05-25 7:22 5% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-05-25 7:22 5% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
1 sibling, 0 replies; 63+ results
From: Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2023-05-25 7:22 UTC (permalink / raw)
To: Ihor Radchenko; +Cc: Mattias Engdegård, 63671
Ihor Radchenko <yantar92@posteo.net> writes:
> If we are serious about adding this feature, lets not close this report.
> My email was on emacs-devel, and it is thus less visible compared to
> something being tracked on debbugs.
I would be happy to see this feature added to Emacs! Unfortunately, I
won't be of much help right now since I have no C experience.
^ permalink raw reply [relevance 5%]
* bug#63671: Add function to test equality of hash tables
2023-05-25 2:44 5% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2023-05-25 6:31 5% ` Ihor Radchenko
2023-05-25 7:22 5% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-05-25 7:22 5% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
0 siblings, 2 replies; 63+ results
From: Ihor Radchenko @ 2023-05-25 6:31 UTC (permalink / raw)
To: Joseph Turner; +Cc: Mattias Engdegård, 63671
Joseph Turner <joseph@breatheoutbreathe.in> writes:
> Thank you for the references, Ihor and Mattias! Since this is a
> duplicate of Ihor's thread, my report should probably be closed.
If we are serious about adding this feature, lets not close this report.
My email was on emacs-devel, and it is thus less visible compared to
something being tracked on debbugs.
--
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>
^ permalink raw reply [relevance 5%]
* bug#63671: Add function to test equality of hash tables
2023-05-24 20:34 4% ` Mattias Engdegård
@ 2023-05-25 2:44 5% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-05-25 6:31 5% ` Ihor Radchenko
0 siblings, 1 reply; 63+ results
From: Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2023-05-25 2:44 UTC (permalink / raw)
To: Mattias Engdegård; +Cc: yantar92, 63671
Mattias Engdegård <mattias.engdegard@gmail.com> writes:
> 24 maj 2023 kl. 19.27 skrev Joseph Turner <joseph@breatheoutbreathe.in>:
>
>> Would extending `equal' to handle hash tables be generally useful?
>
> It would, but doing so would be very risky at this point since it would change long-standing semantics.
>
> We could make an augmented version of `equal` but what we really want is one that works for user-defined types (see Ihor's reference to a previous discussion).
>
> Anyway, here's an old patch I had lying around, in case we decide that we do need a shoddy equality predicate for hash tables only.
Thank you for the references, Ihor and Mattias! Since this is a
duplicate of Ihor's thread, my report should probably be closed.
How do I close this report?
Joseph
^ permalink raw reply [relevance 5%]
* bug#63671: Add function to test equality of hash tables
2023-05-24 17:27 5% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-05-24 20:01 5% ` Ihor Radchenko
@ 2023-05-24 20:34 4% ` Mattias Engdegård
2023-05-25 2:44 5% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
1 sibling, 1 reply; 63+ results
From: Mattias Engdegård @ 2023-05-24 20:34 UTC (permalink / raw)
To: Joseph Turner; +Cc: 63671
[-- Attachment #1: Type: text/plain, Size: 553 bytes --]
24 maj 2023 kl. 19.27 skrev Joseph Turner <joseph@breatheoutbreathe.in>:
> Would extending `equal' to handle hash tables be generally useful?
It would, but doing so would be very risky at this point since it would change long-standing semantics.
We could make an augmented version of `equal` but what we really want is one that works for user-defined types (see Ihor's reference to a previous discussion).
Anyway, here's an old patch I had lying around, in case we decide that we do need a shoddy equality predicate for hash tables only.
[-- Attachment #2: hash-table-equal-p.diff --]
[-- Type: application/octet-stream, Size: 3636 bytes --]
diff --git a/lisp/emacs-lisp/subr-x.el b/lisp/emacs-lisp/subr-x.el
index 9cd793d05c..17ca80a297 100644
--- a/lisp/emacs-lisp/subr-x.el
+++ b/lisp/emacs-lisp/subr-x.el
@@ -93,6 +93,28 @@ hash-table-values
"Return a list of values in HASH-TABLE."
(cl-loop for v being the hash-values of hash-table collect v))
+(defun hash-table-equal-p (h1 h2 &optional value-eq)
+ "Whether the hash tables H1 and H2 are equal with respect to VALUE-EQ.
+Equality means that the tables have the same equality predicate
+and the same set of key-value pairs where keys are compared by
+that predicate and values by VALUE-EQ, which defaults to `eq'."
+ (or (eq h1 h2)
+ (and (= (hash-table-count h1) (hash-table-count h2))
+ (eq (hash-table-test h1) (hash-table-test h2))
+ (progn
+ (unless value-eq
+ (setq value-eq #'eq))
+ ;; Loop over the physically smaller table.
+ (when (> (hash-table-size h1) (hash-table-size h2))
+ (cl-rotatef h1 h2))
+ (catch 'done
+ (maphash
+ (lambda (k v)
+ (unless (funcall value-eq v (gethash k h2 (not v)))
+ (throw 'done nil)))
+ h1)
+ t)))))
+
(defsubst string-empty-p (string)
"Check whether STRING is empty."
(string= string ""))
diff --git a/test/lisp/emacs-lisp/subr-x-tests.el b/test/lisp/emacs-lisp/subr-x-tests.el
index 7f3916c2c0..923155eedb 100644
--- a/test/lisp/emacs-lisp/subr-x-tests.el
+++ b/test/lisp/emacs-lisp/subr-x-tests.el
@@ -743,6 +743,55 @@ test-with-buffer-unmodified-if-unchanged
(with-current-buffer inner
(should-not (buffer-modified-p))))))))
+(ert-deftest subr-x--hash-table-equal-p ()
+ (cl-flet ((hashtab (test &rest elts)
+ (let ((h (make-hash-table :test test)))
+ (while elts
+ (let* ((key (pop elts))
+ (val (pop elts)))
+ (puthash key val h)))
+ h)))
+
+ (let ((h1 (hashtab #'eq 'a (list 1) 'b (list 2))
+ (h2 (hashtab #'eq 'a (list 1) 'b (list 2)))))
+ (should (hash-table-equal-p h1 h2 #'equal))
+ (should (hash-table-equal-p h2 h1 #'equal))
+ (should (not (hash-table-equal-p h1 h2 #'eq)))
+ (should (not (hash-table-equal-p h2 h1 #'eq)))
+ (should (hash-table-equal-p h1 h1 #'eq)))
+
+ (let ((h1 (hashtab #'eql 1 'a 2 'b)
+ (h2 (hashtab #'equal 1 'a 2 'b))))
+ (should (not (hash-table-equal-p h1 h2)))
+ (should (not (hash-table-equal-p h2 h1))))
+
+ (let ((h1 (hashtab #'eql 1 'a 2 'a)
+ (h2 (hashtab #'eql 1 'a))))
+ (should (not (hash-table-equal-p h1 h2)))
+ (should (not (hash-table-equal-p h2 h1))))
+
+ (let ((h1 (hashtab #'eql 1 'a 2 'a)
+ (h2 (hashtab #'eql 1 'a 2 'b))))
+ (should (not (hash-table-equal-p h1 h2)))
+ (should (not (hash-table-equal-p h2 h1))))
+
+ (let ((h1 (hashtab #'eql 1 'a 2 'a)
+ (h2 (hashtab #'eql 1 'a 3 'a))))
+ (should (not (hash-table-equal-p h1 h2)))
+ (should (not (hash-table-equal-p h2 h1))))
+
+ (let ((h1 (hashtab #'eql)
+ (h2 (hashtab #'eql))))
+ (should (hash-table-equal-p h1 h2))
+ (should (hash-table-equal-p h2 h1)))
+
+ (let ((h1 (make-hash-table :test #'eql :size 1000 :rehash-size 3.5))
+ (h2 (hashtab #'eql 10 'a 20 'b)))
+ (puthash 10 'a h1)
+ (puthash 20 'b h1)
+ (should (hash-table-equal-p h1 h2))
+ (should (hash-table-equal-p h2 h1)))
+ ))
(provide 'subr-x-tests)
;;; subr-x-tests.el ends here
^ permalink raw reply related [relevance 4%]
* bug#63671: Add function to test equality of hash tables
2023-05-24 17:27 5% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2023-05-24 20:01 5% ` Ihor Radchenko
2023-05-24 20:34 4% ` Mattias Engdegård
1 sibling, 0 replies; 63+ results
From: Ihor Radchenko @ 2023-05-24 20:01 UTC (permalink / raw)
To: Joseph Turner; +Cc: Mattias Engdegård, 63671
Joseph Turner via "Bug reports for GNU Emacs, the Swiss army knife of
text editors" <bug-gnu-emacs@gnu.org> writes:
>> The standard library must be generally useful with clear semantics.
>
> Would extending `equal' to handle hash tables be generally useful?
See https://yhetil.org/emacs-devel/871qvz4kdw.fsf@localhost/
--
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>
^ permalink raw reply [relevance 5%]
* bug#63671: Add function to test equality of hash tables
@ 2023-05-24 17:27 5% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-05-24 20:01 5% ` Ihor Radchenko
2023-05-24 20:34 4% ` Mattias Engdegård
0 siblings, 2 replies; 63+ results
From: Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2023-05-24 17:27 UTC (permalink / raw)
To: Mattias Engdegård; +Cc: 63671
Mattias Engdegård <mattias.engdegard@gmail.com> writes:
> Thank you, but this seems a bit too specific to your own requirements
> -- maybe you can keep that procedure locally where you need it?
It's no problem to keep the procedure local.
> The standard library must be generally useful with clear semantics.
Would extending `equal' to handle hash tables be generally useful?
Joseph
^ permalink raw reply [relevance 5%]
* bug#63513: [PATCH] Make persist-defvar work with records and hash tables
2023-05-15 11:31 0% ` Eli Zaretskii
@ 2023-05-23 20:14 9% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
0 siblings, 0 replies; 63+ results
From: Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2023-05-23 20:14 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: adam, 63513, Stefan Monnier, Phillip Lord
[-- Attachment #1: Type: text/plain, Size: 258 bytes --]
The patch should now work on Emacs versions before Emacs 30. I also
added tests for persisting records and hash tables.
Instead of copying the updated behavior of copy-tree into persist.el,
would it be more appropriate to require compat.el?
Best,
Joseph
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Add-test-persist-save-macro.patch --]
[-- Type: text/x-diff, Size: 3379 bytes --]
From 7907521e72e2e99b883912f250b7afb14cbf5e80 Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Tue, 23 May 2023 12:57:02 -0700
Subject: [PATCH 1/5] Add test-persist-save macro
---
test/persist-tests.el | 76 +++++++++++++++++++------------------------
1 file changed, 33 insertions(+), 43 deletions(-)
diff --git a/test/persist-tests.el b/test/persist-tests.el
index b6645a9297..0a85b78767 100644
--- a/test/persist-tests.el
+++ b/test/persist-tests.el
@@ -25,51 +25,41 @@ (ert-deftest test-persist-save-only-persistant ()
:type 'error
:exclude-subtypes t))
-(ert-deftest test-persist-save ()
- (with-local-temp-persist
- (let ((sym (cl-gensym)))
- ;; precondition
- (should-not (file-exists-p (persist--file-location sym)))
- (set sym 10)
- (persist-symbol sym 10)
- (persist-save sym)
- (should t)
- (should-not (file-exists-p (persist--file-location sym)))
- (set sym 20)
- (persist-save sym)
- (should (file-exists-p (persist--file-location sym)))
- (should
- (string-match-p
- "20"
- (with-temp-buffer
- (insert-file-contents (persist--file-location sym))
- (buffer-string))))
- (set sym 10)
- (persist-save sym)
- (should-not (file-exists-p (persist--file-location sym)))
- (should-error
- (persist-save 'fred)))))
+(defmacro test-persist-save (init default change printed-changed)
+ "Test persisting symbols.
+- symbol is not persisted when value is set to INIT and default
+ value is set to DEFAULT.
+- symbol is persisted when value is changed according to CHANGE.
+- persisted file contents match PRINTED-CHANGED.
+- symbol is not persisted after value is set back to DEFAULT."
+ `(with-local-temp-persist
+ (let ((sym (cl-gensym)))
+ (should-not (file-exists-p (persist--file-location sym)))
+ (set sym ,init)
+ (persist-symbol sym ,default)
+ (persist-save sym)
+ (should t)
+ (should-not (file-exists-p (persist--file-location sym)))
+ ,change
+ (persist-save sym)
+ (should (file-exists-p (persist--file-location sym)))
+ (should
+ (string-match-p
+ ,printed-changed
+ (with-temp-buffer
+ (insert-file-contents (persist--file-location sym))
+ (buffer-string))))
+ (set sym ,default)
+ (persist-save sym)
+ (should-not (file-exists-p (persist--file-location sym))))))
-(ert-deftest test-persist-save-non-number ()
- "Test saving something that is not a number.
+(ert-deftest test-persist-save-number ()
+ "Test saving number."
+ (test-persist-save 1 1 (set sym 2) "2"))
-`test-persist-save' missed "
- (with-local-temp-persist
- (let ((sym (cl-gensym)))
- (set sym "fred")
- (persist-symbol sym "fred")
- (persist-save sym)
- (should t)
- (should-not (file-exists-p (persist--file-location sym)))
- (set sym "george")
- (persist-save sym)
- (should (file-exists-p (persist--file-location sym)))
- (should
- (string-match-p
- "george"
- (with-temp-buffer
- (insert-file-contents (persist--file-location sym))
- (buffer-string)))))))
+(ert-deftest test-persist-save-string ()
+ "Test saving string."
+ (test-persist-save "foo" "foo" (set sym "bar") "bar"))
(ert-deftest test-persist-load ()
(with-local-temp-persist
--
2.40.1
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-Add-persist-hash-equal.patch --]
[-- Type: text/x-diff, Size: 1460 bytes --]
From 41f90ac59a26018382d1bb9153af08ce4b9423ff Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Tue, 23 May 2023 12:52:24 -0700
Subject: [PATCH 2/5] Add persist-hash-equal
See bug#63671.
---
persist.el | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/persist.el b/persist.el
index d80943d19e..0069273ca2 100644
--- a/persist.el
+++ b/persist.el
@@ -187,5 +187,22 @@ (defun persist--save-all ()
(add-hook 'kill-emacs-hook
'persist--save-all)
+(defun persist-hash-equal (hash1 hash2)
+ "Return non-nil when the contents of HASH1 and HASH2 are equal.
+Table values are compared using `equal' unless they are both hash
+tables themselves, in which case `hash-equal' is used.
+Does not compare equality predicates."
+ (and (= (hash-table-count hash1)
+ (hash-table-count hash2))
+ (catch 'flag (maphash (lambda (key hash1-value)
+ (let ((hash2-value (gethash key hash2)))
+ (or (if (and (hash-table-p hash1-value)
+ (hash-table-p hash2-value))
+ (hash-equal hash1-value hash2-value)
+ (equal hash1-value hash2-value))
+ (throw 'flag nil))))
+ hash1)
+ t)))
+
(provide 'persist)
;;; persist.el ends here
--
2.40.1
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: 0003-Make-persist-defvar-work-with-hash-tables.patch --]
[-- Type: text/x-diff, Size: 2988 bytes --]
From 53e166cb0c7c2624a5a54edafe8828d7b7edc612 Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Tue, 23 May 2023 13:09:29 -0700
Subject: [PATCH 3/5] Make persist-defvar work with hash tables
Previously, when persist-defvar received a hash table for an initial
value, updated values were not persisted.
---
persist.el | 16 +++++++++++-----
test/persist-tests.el | 8 ++++++++
2 files changed, 19 insertions(+), 5 deletions(-)
diff --git a/persist.el b/persist.el
index 0069273ca2..725f2f71cf 100644
--- a/persist.el
+++ b/persist.el
@@ -118,7 +118,9 @@ (defun persist-symbol (symbol &optional initvalue)
(let ((initvalue (or initvalue (symbol-value symbol))))
(add-to-list 'persist--symbols symbol)
(put symbol 'persist t)
- (put symbol 'persist-default initvalue)))
+ (if (hash-table-p initvalue)
+ (put symbol 'persist-default (copy-hash-table initvalue))
+ (put symbol 'persist-default initvalue))))
(defun persist--persistant-p (symbol)
"Return non-nil if SYMBOL is a persistant variable."
@@ -132,9 +134,13 @@ (defun persist-save (symbol)
(unless (persist--persistant-p symbol)
(error (format
"Symbol %s is not persistant" symbol)))
- (let ((symbol-file-loc (persist--file-location symbol)))
- (if (equal (symbol-value symbol)
- (persist-default symbol))
+ (let ((symbol-file-loc (persist--file-location symbol))
+ (value (symbol-value symbol))
+ (default (persist-default symbol)))
+ (if (if (and (hash-table-p value)
+ (hash-table-p default))
+ (persist-hash-equal value default)
+ (equal value default))
(when (file-exists-p symbol-file-loc)
(delete-file symbol-file-loc))
(let ((dir-loc
@@ -148,7 +154,7 @@ (defun persist-save (symbol)
(print-escape-control-characters t)
(print-escape-nonascii t)
(print-circle t))
- (print (symbol-value symbol) (current-buffer)))
+ (print value (current-buffer)))
(write-region (point-min) (point-max)
symbol-file-loc
nil 'quiet))))))
diff --git a/test/persist-tests.el b/test/persist-tests.el
index 0a85b78767..8a30a24e23 100644
--- a/test/persist-tests.el
+++ b/test/persist-tests.el
@@ -61,6 +61,14 @@ (ert-deftest test-persist-save-string ()
"Test saving string."
(test-persist-save "foo" "foo" (set sym "bar") "bar"))
+(ert-deftest test-persist-save-hash ()
+ "Test saving hash table."
+ (let* ((hash (make-hash-table))
+ (default (copy-hash-table hash)))
+ (test-persist-save hash default
+ (puthash 'foo "bar" (symbol-value sym))
+ "#s(hash-table size 65 test eql rehash-size 1.5 rehash-threshold 0.8125 data (foo \"bar\"))")))
+
(ert-deftest test-persist-load ()
(with-local-temp-persist
(let ((sym (cl-gensym)))
--
2.40.1
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #5: 0004-Add-persist-copy-tree.patch --]
[-- Type: text/x-diff, Size: 1912 bytes --]
From ccc1be590b6c7b71aaa4ee3dd3bf25184ea7c122 Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Tue, 23 May 2023 13:43:02 -0700
Subject: [PATCH 4/5] Add persist-copy-tree
The behavior of copy-tree was changed in Emacs 30. This function will
ensure that persist works correctly for previous Emacs versions.
---
persist.el | 29 +++++++++++++++++++++++++++++
1 file changed, 29 insertions(+)
diff --git a/persist.el b/persist.el
index 725f2f71cf..eb408b5dca 100644
--- a/persist.el
+++ b/persist.el
@@ -210,5 +210,34 @@ (defun persist-hash-equal (hash1 hash2)
hash1)
t)))
+
+(defun persist-copy-tree (tree &optional vectors-and-records)
+ "Make a copy of TREE.
+If TREE is a cons cell, this recursively copies both its car and its cdr.
+Contrast to `copy-sequence', which copies only along the cdrs.
+With the second argument VECTORS-AND-RECORDS non-nil, this
+traverses and copies vectors and records as well as conses."
+ (declare (side-effect-free error-free))
+ (if (consp tree)
+ (let (result)
+ (while (consp tree)
+ (let ((newcar (car tree)))
+ (if (or (consp (car tree))
+ (and vectors-and-records
+ (or (vectorp (car tree)) (recordp (car tree)))))
+ (setq newcar (copy-tree (car tree) vectors-and-records)))
+ (push newcar result))
+ (setq tree (cdr tree)))
+ (nconc (nreverse result)
+ (if (and vectors-and-records (or (vectorp tree) (recordp tree)))
+ (copy-tree tree vectors-and-records)
+ tree)))
+ (if (and vectors-and-records (or (vectorp tree) (recordp tree)))
+ (let ((i (length (setq tree (copy-sequence tree)))))
+ (while (>= (setq i (1- i)) 0)
+ (aset tree i (copy-tree (aref tree i) vectors-and-records)))
+ tree)
+ tree)))
+
(provide 'persist)
;;; persist.el ends here
--
2.40.1
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #6: 0005-Make-persist-defvar-work-with-records.patch --]
[-- Type: text/x-diff, Size: 1763 bytes --]
From 4658bde147253d2f070b14c6f54300f640da063e Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Tue, 23 May 2023 13:44:40 -0700
Subject: [PATCH 5/5] Make persist-defvar work with records
Previously, when persist-defvar received a record for an initial
value, updated values were not persisted.
---
persist.el | 2 +-
test/persist-tests.el | 8 ++++++++
2 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/persist.el b/persist.el
index eb408b5dca..39ce54bbf0 100644
--- a/persist.el
+++ b/persist.el
@@ -120,7 +120,7 @@ (defun persist-symbol (symbol &optional initvalue)
(put symbol 'persist t)
(if (hash-table-p initvalue)
(put symbol 'persist-default (copy-hash-table initvalue))
- (put symbol 'persist-default initvalue))))
+ (put symbol 'persist-default (persist-copy-tree initvalue t)))))
(defun persist--persistant-p (symbol)
"Return non-nil if SYMBOL is a persistant variable."
diff --git a/test/persist-tests.el b/test/persist-tests.el
index 8a30a24e23..18b8af2b89 100644
--- a/test/persist-tests.el
+++ b/test/persist-tests.el
@@ -69,6 +69,14 @@ (ert-deftest test-persist-save-hash ()
(puthash 'foo "bar" (symbol-value sym))
"#s(hash-table size 65 test eql rehash-size 1.5 rehash-threshold 0.8125 data (foo \"bar\"))")))
+(ert-deftest test-persist-save-record ()
+ "Test saving record."
+ (let* ((rec (record 'foo 'a 'b))
+ (default (copy-sequence rec)))
+ (test-persist-save rec default
+ (setf (aref (symbol-value sym) 2) 'quux)
+ "#s(foo a quux)")))
+
(ert-deftest test-persist-load ()
(with-local-temp-persist
(let ((sym (cl-gensym)))
--
2.40.1
^ permalink raw reply related [relevance 9%]
* bug#63671: Add function to test equality of hash tables
@ 2023-05-23 19:32 5% Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
0 siblings, 0 replies; 63+ results
From: Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2023-05-23 19:32 UTC (permalink / raw)
To: 63671
Hello!
Would y'all be open to adding something like this?
(defun hash-equal (hash1 hash2)
"Return non-nil when the contents of HASH1 and HASH2 are equal.
Table values are compared using `equal' unless they are both hash
tables themselves, in which case `hash-equal' is used.
Does not compare equality predicates."
(and (= (hash-table-count hash1)
(hash-table-count hash2))
(catch 'flag (maphash (lambda (key hash1-value)
(let ((hash2-value (gethash key hash2)))
(or (if (and (hash-table-p hash1-value)
(hash-table-p hash2-value))
(hash-equal hash1-value hash2-value)
(equal hash1-value hash2-value))
(throw 'flag nil))))
hash1)
t)))
Rudimentary test:
(let ((hash1 (make-hash-table))
(hash2 (make-hash-table))
(hash3 (make-hash-table))
(hash4 (make-hash-table)))
(puthash 'foo "foo" hash1)
(puthash 'foo "foo" hash2)
(puthash 'bar "foo" hash3)
(puthash 'bar "foo" hash4)
(puthash 'baz hash3 hash1)
(puthash 'baz hash4 hash2)
(hash-equal hash1 hash2))
We could use hash-table-test to compare predicates, perhaps
dependent on the presence of a 'compare-tests flag?
Best,
Joseph
^ permalink raw reply [relevance 5%]
* bug#63509: [PATCH] Make copy-tree work with records
2023-05-18 19:05 7% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2023-05-19 6:07 0% ` Eli Zaretskii
0 siblings, 0 replies; 63+ results
From: Eli Zaretskii @ 2023-05-19 6:07 UTC (permalink / raw)
To: Joseph Turner; +Cc: 63509-done, monnier
> From: Joseph Turner <joseph@breatheoutbreathe.in>
> Cc: monnier@iro.umontreal.ca, 63509@debbugs.gnu.org
> Date: Thu, 18 May 2023 12:05:57 -0700
>
> Please let me know if any further changes need to be made!
Thanks, installed on master, and closing the bug.
^ permalink raw reply [relevance 0%]
* bug#63509: [PATCH] Make copy-tree work with records
2023-05-18 10:53 0% ` Eli Zaretskii
@ 2023-05-18 19:05 7% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-05-19 6:07 0% ` Eli Zaretskii
0 siblings, 1 reply; 63+ results
From: Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2023-05-18 19:05 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: 63509, monnier
[-- Attachment #1: Type: text/plain, Size: 1877 bytes --]
Eli Zaretskii <eliz@gnu.org> writes:
> Yes, this new feature will be installed on the master branch, which
> will become Emacs 30.
Moved note from NEWS.29 to NEWS.
>> --- a/doc/lispref/records.texi
>> +++ b/doc/lispref/records.texi
>> @@ -81,6 +81,18 @@ This function returns a new record with type @var{type} and
>> @end example
>> @end defun
>>
>> +@defun copy-tree tree &optional vector-like-p
>> +This function copies a record when @var{vector-like-p} is
>> +non-@code{nil}.
>> +
>> +@example
>> +@group
>> +(copy-tree (record 'foo "a"))
>> + @result{} #s(foo "a")
>> +@end group
>> +@end example
>> +@end defun
>
> This addition is redundant. We don't describe the same function in
> more than one place. If there are reasons to mention it in other
> places, we just add there a short note with a cross-reference to the
> detailed description.
Replaced @defun with a short sentence with @pxref.
>> ++++
>> +** 'copy-tree' now correctly copies records when its optional second
>
> The "correctly" part hints that the previous behavior was a bug, which
> it wasn't (and we don't mention bugfixes in NEWS anyway). So I would
> rephrase
>
> 'copy-tree' can now copy records as well, when its optional...
>
>> +argument is non-nil. The second argument has been renamed from VECP
>> +to VECTOR-LIKE-P since it now works with both vectors and records.
>
> The last sentence should be removed: we don't mention such minor
> details in NEWS, unless the change is an incompatible change.
Done.
> Last, but not least: please always accompany your changes with
> ChageLog-style commit log messages describing the changes. You can
> find more information about this in the file CONTRIBUTE in the Emacs
> tree, and you can see many examples by typing "git log" in the
> repository.
Done.
Please let me know if any further changes need to be made!
Best,
Joseph
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Make-copy-tree-work-with-records.patch --]
[-- Type: text/x-diff, Size: 6739 bytes --]
From 0ae16ca89e581d3c732607b2daa700d8316a71e3 Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Sun, 14 May 2023 21:02:15 -0700
Subject: [PATCH] Make copy-tree work with records
* doc/lispref/lists.texi (Building Cons Cells and Lists): Document new
behavior of copy-tree.
* doc/lispref/records.texi (Record Functions): Cross-reference to
lists.texi.
* etc/NEWS: Mention change. (Bug#63509)
* lisp/emacs-lisp/shortdoc.el: Add copy-tree example to vector group.
* lisp/subr.el (copy-tree): Recurse into records as well as vectors
when optional second argument is non-nil. Rename second argument to
from vecp to vector-like-p.
* test/lisp/subr-tests.el: Test new behavior.
---
doc/lispref/lists.texi | 9 +++++----
doc/lispref/records.texi | 3 +++
etc/NEWS | 3 +++
lisp/emacs-lisp/shortdoc.el | 2 ++
lisp/subr.el | 14 +++++++-------
test/lisp/subr-tests.el | 31 +++++++++++++++++++++++++++++++
6 files changed, 51 insertions(+), 11 deletions(-)
diff --git a/doc/lispref/lists.texi b/doc/lispref/lists.texi
index 22a5f7f1239..16ed0358974 100644
--- a/doc/lispref/lists.texi
+++ b/doc/lispref/lists.texi
@@ -696,16 +696,17 @@ not a list, the sequence's elements do not become elements of the
resulting list. Instead, the sequence becomes the final @sc{cdr}, like
any other non-list final argument.
-@defun copy-tree tree &optional vecp
+@defun copy-tree tree &optional vector-like-p
This function returns a copy of the tree @var{tree}. If @var{tree} is a
cons cell, this makes a new cons cell with the same @sc{car} and
@sc{cdr}, then recursively copies the @sc{car} and @sc{cdr} in the
same way.
Normally, when @var{tree} is anything other than a cons cell,
-@code{copy-tree} simply returns @var{tree}. However, if @var{vecp} is
-non-@code{nil}, it copies vectors too (and operates recursively on
-their elements). This function cannot cope with circular lists.
+@code{copy-tree} simply returns @var{tree}. However, if
+@var{vector-like-p} is non-@code{nil}, it copies vectors and records
+too (and operates recursively on their elements). This function
+cannot cope with circular lists.
@end defun
@defun flatten-tree tree
diff --git a/doc/lispref/records.texi b/doc/lispref/records.texi
index 26c6f30a6b5..d2c80a27f98 100644
--- a/doc/lispref/records.texi
+++ b/doc/lispref/records.texi
@@ -81,6 +81,9 @@ This function returns a new record with type @var{type} and
@end example
@end defun
+@code{copy-tree} works with records when its optional second argument
+is non-@code{nil} (@pxref{Building Lists}).
+
@node Backward Compatibility
@section Backward Compatibility
diff --git a/etc/NEWS b/etc/NEWS
index ce865c9904d..c5063a718b9 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -585,6 +585,9 @@ Since circular alias chains now cannot occur, 'function-alias-p',
'indirect-function' and 'indirect-variable' will never signal an error.
Their 'noerror' arguments have no effect and are therefore obsolete.
++++
+** 'copy-tree' now copies records when its optional argument is non-nil.
+
\f
* Changes in Emacs 30.1 on Non-Free Operating Systems
diff --git a/lisp/emacs-lisp/shortdoc.el b/lisp/emacs-lisp/shortdoc.el
index 9a6f5dd12ce..6580e0e4e0c 100644
--- a/lisp/emacs-lisp/shortdoc.el
+++ b/lisp/emacs-lisp/shortdoc.el
@@ -833,6 +833,8 @@ A FUNC form can have any number of `:no-eval' (or `:no-value'),
(seq-subseq
:eval (seq-subseq [1 2 3 4 5] 1 3)
:eval (seq-subseq [1 2 3 4 5] 1))
+ (copy-tree
+ :eval (copy-tree [1 2 3 4]))
"Mapping Over Vectors"
(mapcar
:eval (mapcar #'identity [1 2 3]))
diff --git a/lisp/subr.el b/lisp/subr.el
index 03d3324f3d8..83735933963 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -824,26 +824,26 @@ of course, also replace TO with a slightly larger value
next (+ from (* n inc)))))
(nreverse seq))))
-(defun copy-tree (tree &optional vecp)
+(defun copy-tree (tree &optional vector-like-p)
"Make a copy of TREE.
If TREE is a cons cell, this recursively copies both its car and its cdr.
Contrast to `copy-sequence', which copies only along the cdrs. With second
-argument VECP, this copies vectors as well as conses."
+argument VECTOR-LIKE-P, this copies vectors and records as well as conses."
(declare (side-effect-free error-free))
(if (consp tree)
(let (result)
(while (consp tree)
(let ((newcar (car tree)))
- (if (or (consp (car tree)) (and vecp (vectorp (car tree))))
- (setq newcar (copy-tree (car tree) vecp)))
+ (if (or (consp (car tree)) (and vector-like-p (or (vectorp (car tree)) (recordp (car tree)))))
+ (setq newcar (copy-tree (car tree) vector-like-p)))
(push newcar result))
(setq tree (cdr tree)))
(nconc (nreverse result)
- (if (and vecp (vectorp tree)) (copy-tree tree vecp) tree)))
- (if (and vecp (vectorp tree))
+ (if (and vector-like-p (or (vectorp tree) (recordp tree))) (copy-tree tree vector-like-p) tree)))
+ (if (and vector-like-p (or (vectorp tree) (recordp tree)))
(let ((i (length (setq tree (copy-sequence tree)))))
(while (>= (setq i (1- i)) 0)
- (aset tree i (copy-tree (aref tree i) vecp)))
+ (aset tree i (copy-tree (aref tree i) vector-like-p)))
tree)
tree)))
diff --git a/test/lisp/subr-tests.el b/test/lisp/subr-tests.el
index 8f46c2af136..4ebb68556be 100644
--- a/test/lisp/subr-tests.el
+++ b/test/lisp/subr-tests.el
@@ -1206,5 +1206,36 @@ final or penultimate step during initialization."))
(should (equal a-dedup '("a" "b" "a" "b" "c")))
(should (eq a a-dedup))))
+(ert-deftest subr--copy-tree ()
+ (should (eq (copy-tree nil) nil))
+ (let* ((a (list (list "a") "b" (list "c") "g"))
+ (copy1 (copy-tree a))
+ (copy2 (copy-tree a t)))
+ (should (equal a copy1))
+ (should (equal a copy2))
+ (should-not (eq a copy1))
+ (should-not (eq a copy2)))
+ (let* ((a (list (list "a") "b" (list "c" (record 'foo "d")) (list ["e" "f"]) "g"))
+ (copy1 (copy-tree a))
+ (copy2 (copy-tree a t)))
+ (should (equal a copy1))
+ (should (equal a copy2))
+ (should-not (eq a copy1))
+ (should-not (eq a copy2)))
+ (let* ((a (record 'foo "a" (record 'bar "b")))
+ (copy1 (copy-tree a))
+ (copy2 (copy-tree a t)))
+ (should (equal a copy1))
+ (should (equal a copy2))
+ (should (eq a copy1))
+ (should-not (eq a copy2)))
+ (let* ((a ["a" "b" ["c" ["d"]]])
+ (copy1 (copy-tree a))
+ (copy2 (copy-tree a t)))
+ (should (equal a copy1))
+ (should (equal a copy2))
+ (should (eq a copy1))
+ (should-not (eq a copy2))))
+
(provide 'subr-tests)
;;; subr-tests.el ends here
--
2.40.1
^ permalink raw reply related [relevance 7%]
* bug#63509: [PATCH] Make copy-tree work with records
2023-05-15 17:59 7% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2023-05-18 10:53 0% ` Eli Zaretskii
2023-05-18 19:05 7% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
0 siblings, 1 reply; 63+ results
From: Eli Zaretskii @ 2023-05-18 10:53 UTC (permalink / raw)
To: Joseph Turner; +Cc: 63509, monnier
> From: Joseph Turner <joseph@breatheoutbreathe.in>
> Cc: Stefan Monnier <monnier@iro.umontreal.ca>, 63509@debbugs.gnu.org
> Date: Mon, 15 May 2023 10:59:57 -0700
>
> Eli Zaretskii <eliz@gnu.org> writes:
>
> > - NEWS entry
>
> I updated the 29 entry. Should I move it to 30?
Yes, this new feature will be installed on the master branch, which
will become Emacs 30.
> --- a/doc/lispref/records.texi
> +++ b/doc/lispref/records.texi
> @@ -81,6 +81,18 @@ This function returns a new record with type @var{type} and
> @end example
> @end defun
>
> +@defun copy-tree tree &optional vector-like-p
> +This function copies a record when @var{vector-like-p} is
> +non-@code{nil}.
> +
> +@example
> +@group
> +(copy-tree (record 'foo "a"))
> + @result{} #s(foo "a")
> +@end group
> +@end example
> +@end defun
This addition is redundant. We don't describe the same function in
more than one place. If there are reasons to mention it in other
places, we just add there a short note with a cross-reference to the
detailed description.
> ++++
> +** 'copy-tree' now correctly copies records when its optional second
The "correctly" part hints that the previous behavior was a bug, which
it wasn't (and we don't mention bugfixes in NEWS anyway). So I would
rephrase
'copy-tree' can now copy records as well, when its optional...
> +argument is non-nil. The second argument has been renamed from VECP
> +to VECTOR-LIKE-P since it now works with both vectors and records.
The last sentence should be removed: we don't mention such minor
details in NEWS, unless the change is an incompatible change.
Last, but not least: please always accompany your changes with
ChageLog-style commit log messages describing the changes. You can
find more information about this in the file CONTRIBUTE in the Emacs
tree, and you can see many examples by typing "git log" in the
repository.
Thanks.
^ permalink raw reply [relevance 0%]
* bug#63336: [PATCH] package-vc: Process :make and :shell-command spec args
2023-05-16 21:08 5% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2023-05-17 14:07 5% ` Philip Kaludercic
0 siblings, 0 replies; 63+ results
From: Philip Kaludercic @ 2023-05-17 14:07 UTC (permalink / raw)
To: Joseph Turner; +Cc: 63336-done
Joseph Turner <joseph@breatheoutbreathe.in> writes:
> Philip Kaludercic <philipk@posteo.net> writes:
>
>> I've pushed the changes to master. If you are still interested in
>> improving the granularity of the issue, create a new report when you
>> have a patch we can discuss.
>
> Thank you! I assume you're referring to something like a 'user-defined
> option for package-vc-allow-side-effects.
Right,
> At some point, I may submit
> another patch adding that feature!
but there is no hurry for that.
> Best,
>
> Joseph
^ permalink raw reply [relevance 5%]
* bug#63336: [PATCH] package-vc: Process :make and :shell-command spec args
@ 2023-05-16 21:08 5% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-05-17 14:07 5% ` Philip Kaludercic
0 siblings, 1 reply; 63+ results
From: Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2023-05-16 21:08 UTC (permalink / raw)
To: Philip Kaludercic; +Cc: 63336-done
Philip Kaludercic <philipk@posteo.net> writes:
> I've pushed the changes to master. If you are still interested in
> improving the granularity of the issue, create a new report when you
> have a patch we can discuss.
Thank you! I assume you're referring to something like a 'user-defined
option for package-vc-allow-side-effects. At some point, I may submit
another patch adding that feature!
Best,
Joseph
^ permalink raw reply [relevance 5%]
* bug#63336: [PATCH] package-vc: Process :make and :shell-command spec args
2023-05-15 9:12 5% ` Philip Kaludercic
@ 2023-05-15 19:03 5% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
0 siblings, 1 reply; 63+ results
From: Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2023-05-15 19:03 UTC (permalink / raw)
To: Philip Kaludercic; +Cc: 63336
Philip Kaludercic <philipk@posteo.net> writes:
> Joseph Turner <joseph@breatheoutbreathe.in> writes:
>
>> Philip Kaludercic <philipk@posteo.net> writes:
>>
>>> Joseph Turner <joseph@breatheoutbreathe.in> writes:
>>>
>>>> Philip Kaludercic <philipk@posteo.net> writes:
>>>>
>>>>> Joseph Turner <joseph@breatheoutbreathe.in> writes:
>>>>>
>>>>>> Philip Kaludercic <philipk@posteo.net> writes:
>>>>>>
>>>>>>> Joseph Turner <joseph@breatheoutbreathe.in> writes:
>>
>>>> We also might want to add another option for
>>>> package-vc-allow-side-effects like 'user-defined, which only runs :make
>>>> and :shell-command args which were specified by the user (as opposed to
>>>> those which were downloaded from elpa). WDYT?
>>>
>>> That sounds like a good idea, but let us do that in a separate patch.
>>
>> Okay!
>>
>>>> To update the manual, shall I edit doc/emacs/package.texi directly or is
>>>> there another file to edit?
>>>
>>> Yes, just update the table under the "Specifying Package Sources" subsection.
>>
>> See patch.
>>
>>>>> If :shell-command fails, do we really want to proceed to :make?
>>>>
>>>> Up to you! I was following the lead of elpa-admin.el.
>>>
>>> In that case let us do that too, unless there is a good reason not to.
>>
>> +1
>>
>>>> I switched the first two cases. I think pcase is readable here,
>>>> especially if we add an 'user-defined option. What would you use
>>>> instead?
>>>
>>> I would have just used a regular cond.
>>>
>>> --8<---------------cut here---------------start------------->8---
>>> (cond
>>> ((null package-vc-process-make)
>>> ...)
>>> ((listp package-vc-process-make)
>>> ...)
>>> (...))
>>> --8<---------------cut here---------------end--------------->8---
>>>
>>> But this doesn't matter, do what you prefer.
>>
>> Thank you! I like pcase here.
>>
>>>> +Be careful when changing this option as processing :make and
>>>> +:shell-command will run potentially harmful code.
>>>
>>> Sounds scary. I guess that is the point, but what do you think about
>>> something like
>>>
>>> Be careful when changing this option, as installing and updating a
>>> package can potentially run harmful code. If possible, allow packages
>>> you trust to run code, if it is necessary for a package to be properly
>>> initialised.
>>
>> Thank you! What do you think about the version in the attached patch?
>>
>>>> +When set to a list of symbols (packages), run commands for only
>>>> +packages in the list. When `nil', never run commands. Otherwise
>>>> +when non-`nil', run commands for any package with :make or
>>>> +:shell-command specified.
>>>
>>> Watch out. According to (elisp) Documentation Tips, nil is not quoted.
>>
>> Good to know! Fixed.
>>
>> From 812e32ea6c3f7b2d71174658db0e272b0b4fb84b Mon Sep 17 00:00:00 2001
>> From: Joseph Turner <joseph@breatheoutbreathe.in>
>> Date: Sat, 13 May 2023 10:05:04 -0700
>> Subject: [PATCH] package-vc: Process :make and :shell-command spec args
>>
>> ---
>> doc/emacs/package.texi | 9 ++++++++
>> lisp/emacs-lisp/package-vc.el | 42 +++++++++++++++++++++++++++++++++++
>> 2 files changed, 51 insertions(+)
>>
>> diff --git a/doc/emacs/package.texi b/doc/emacs/package.texi
>> index 6722185cb20..4f606b22e54 100644
>> --- a/doc/emacs/package.texi
>> +++ b/doc/emacs/package.texi
>> @@ -682,6 +682,15 @@ A string providing the repository-relative name of the documentation
>> file from which to build an Info file. This can be a Texinfo file or
>> an Org file.
>>
>> +@item :make
>> +A string or list of strings providing the target or targets defined in
>> +the repository Makefile which should run before building the Info
>> +file. Only takes effect when package-vc-allow-side-effects is non-nil.
>
> A @var is missing here
Thank you!
>> +
>> +@item :shell-command
>> +A string providing the shell command to run before building the Info
>> +file. Only takes effect when package-vc-allow-side-effects is non-nil.
>
> and here. I can take care of that.
Thank you!
>> +
>> @item :vc-backend
>> A symbol naming the VC backend to use for downloading a copy of the
>> package's repository (@pxref{Version Control Systems,,,emacs, The GNU
>> diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
>> index beca0bd00e2..d2f6d287224 100644
>> --- a/lisp/emacs-lisp/package-vc.el
>> +++ b/lisp/emacs-lisp/package-vc.el
>> @@ -344,6 +344,38 @@ asynchronously."
>> "\n")
>> nil pkg-file nil 'silent))))
>>
>> +(defcustom package-vc-allow-side-effects nil
>> + "Whether to process :make and :shell-command spec arguments.
>> +
>> +It may be necessary to run :make and :shell-command arguments in
>> +order to initialize a package or build its documentation, but
>> +please be careful when changing this option, as installing and
>> +updating a package can run potentially harmful code.
>> +
>> +When set to a list of symbols (packages), run commands for only
>> +packages in the list. When nil, never run commands. Otherwise
>> +when non-nil, run commands for any package with :make or
>> +:shell-command specified.
>> +
>> +Package specs are loaded from trusted package archives."
>> + :type '(choice (const :tag "Run for all packages" t)
>> + (repeat :tag "Run only for selected packages" (symbol :tag "Package name"))
>> + (const :tag "Never run" nil))
>> + :version "30.1")
>> +
>> +(defun package-vc--make (pkg-spec pkg-desc)
>> + "Process :make and :shell-command PKG-SPEC arguments for PKG-DESC."
>> + (let ((target (plist-get pkg-spec :make))
>> + (cmd (plist-get pkg-spec :shell-command))
>> + (buf (format " *package-vc make %s*" (package-desc-name pkg-desc))))
>> + (when (or cmd target)
>> + (with-current-buffer (get-buffer-create buf)
>> + (erase-buffer)
>> + (when (and cmd (/= 0 (call-process shell-file-name nil t nil shell-command-switch cmd)))
>> + (warn "Failed to run %s, see buffer %S" cmd (buffer-name)))
>> + (when (and target (/= 0 (apply #'call-process "make" nil t nil (if (consp target) target (list target)))))
>> + (warn "Failed to make %s, see buffer %S" target (buffer-name)))))))
>> +
>> (declare-function org-export-to-file "ox" (backend file))
>>
>> (defun package-vc--build-documentation (pkg-desc file)
>> @@ -486,6 +518,16 @@ documentation and marking the package as installed."
>> ;; Generate package file
>> (package-vc--generate-description-file pkg-desc pkg-file)
>>
>> + ;; Process :make and :shell-command arguments before building documentation
>> + (pcase package-vc-allow-side-effects
>> + ('nil ; When `nil', do nothing.
>> + nil)
>> + ((pred consp) ; When non-`nil' list, check if package is on the list.
>> + (when (memq (package-desc-name pkg-desc) package-vc-allow-side-effects)
>> + (package-vc--make pkg-spec pkg-desc)))
>> + (_ ; When otherwise non-`nil', run commands.
>> + (package-vc--make pkg-spec pkg-desc)))
>
> Thinking about this again, I am still not convinced. Isn't
>
> --8<---------------cut here---------------start------------->8---
> (when (or (eq package-vc-allow-side-effects t)
> (memq (package-desc-name pkg-desc)
> package-vc-allow-side-effects))
> (package-vc--make pkg-spec pkg-desc))
> --8<---------------cut here---------------end--------------->8---
>
> much simpler? Again, you don't have to prepare another patch, I'm just
> interested in what you think.
I take it all back and insist upon the opposite :)
You are right, that's much simpler.
>> +
>> ;; Detect a manual
>> (when (executable-find "install-info")
>> (dolist (doc-file (ensure-list (plist-get pkg-spec :doc)))
^ permalink raw reply [relevance 5%]
* bug#63509: [PATCH] Make copy-tree work with records
2023-05-15 11:26 0% ` Eli Zaretskii
@ 2023-05-15 17:59 7% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-05-18 10:53 0% ` Eli Zaretskii
0 siblings, 1 reply; 63+ results
From: Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2023-05-15 17:59 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: 63509, Stefan Monnier
[-- Attachment #1: Type: text/plain, Size: 476 bytes --]
Eli Zaretskii <eliz@gnu.org> writes:
> - NEWS entry
I updated the 29 entry. Should I move it to 30?
> - update for the ELisp manual
Changes made in patch:
- updated the entry in lists.texi
- added a new entry in records.texi
- added entry in the vector group in shortdoc.el
> - preferably added tests to regression-test this feature
Done.
> I also think we should rename the second argument, as VECP no longer
> fits.
How about VECTOR-LIKE-P? See patch.
Joseph
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Make-copy-tree-work-with-records.patch --]
[-- Type: text/x-diff, Size: 6502 bytes --]
From 14383e9fe8a107f597d06ad486a441b761cc6ade Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Sun, 14 May 2023 21:02:15 -0700
Subject: [PATCH] Make copy-tree work with records
---
doc/lispref/lists.texi | 9 +++++----
doc/lispref/records.texi | 12 ++++++++++++
etc/NEWS.29 | 5 +++++
lisp/emacs-lisp/shortdoc.el | 2 ++
lisp/subr.el | 14 +++++++-------
test/lisp/subr-tests.el | 31 +++++++++++++++++++++++++++++++
6 files changed, 62 insertions(+), 11 deletions(-)
diff --git a/doc/lispref/lists.texi b/doc/lispref/lists.texi
index 22a5f7f1239..16ed0358974 100644
--- a/doc/lispref/lists.texi
+++ b/doc/lispref/lists.texi
@@ -696,16 +696,17 @@ not a list, the sequence's elements do not become elements of the
resulting list. Instead, the sequence becomes the final @sc{cdr}, like
any other non-list final argument.
-@defun copy-tree tree &optional vecp
+@defun copy-tree tree &optional vector-like-p
This function returns a copy of the tree @var{tree}. If @var{tree} is a
cons cell, this makes a new cons cell with the same @sc{car} and
@sc{cdr}, then recursively copies the @sc{car} and @sc{cdr} in the
same way.
Normally, when @var{tree} is anything other than a cons cell,
-@code{copy-tree} simply returns @var{tree}. However, if @var{vecp} is
-non-@code{nil}, it copies vectors too (and operates recursively on
-their elements). This function cannot cope with circular lists.
+@code{copy-tree} simply returns @var{tree}. However, if
+@var{vector-like-p} is non-@code{nil}, it copies vectors and records
+too (and operates recursively on their elements). This function
+cannot cope with circular lists.
@end defun
@defun flatten-tree tree
diff --git a/doc/lispref/records.texi b/doc/lispref/records.texi
index 26c6f30a6b5..0f44198a6b0 100644
--- a/doc/lispref/records.texi
+++ b/doc/lispref/records.texi
@@ -81,6 +81,18 @@ This function returns a new record with type @var{type} and
@end example
@end defun
+@defun copy-tree tree &optional vector-like-p
+This function copies a record when @var{vector-like-p} is
+non-@code{nil}.
+
+@example
+@group
+(copy-tree (record 'foo "a"))
+ @result{} #s(foo "a")
+@end group
+@end example
+@end defun
+
@node Backward Compatibility
@section Backward Compatibility
diff --git a/etc/NEWS.29 b/etc/NEWS.29
index fa428d9c790..ae9a89203bf 100644
--- a/etc/NEWS.29
+++ b/etc/NEWS.29
@@ -4897,6 +4897,11 @@ Instead, Emacs uses the already-existing 'make-directory' handlers.
This can let a caller know whether it created DIR. Formerly,
'make-directory's return value was unspecified.
++++
+** 'copy-tree' now correctly copies records when its optional second
+argument is non-nil. The second argument has been renamed from VECP
+to VECTOR-LIKE-P since it now works with both vectors and records.
+
\f
* Changes in Emacs 29.1 on Non-Free Operating Systems
diff --git a/lisp/emacs-lisp/shortdoc.el b/lisp/emacs-lisp/shortdoc.el
index 9a6f5dd12ce..6580e0e4e0c 100644
--- a/lisp/emacs-lisp/shortdoc.el
+++ b/lisp/emacs-lisp/shortdoc.el
@@ -833,6 +833,8 @@ A FUNC form can have any number of `:no-eval' (or `:no-value'),
(seq-subseq
:eval (seq-subseq [1 2 3 4 5] 1 3)
:eval (seq-subseq [1 2 3 4 5] 1))
+ (copy-tree
+ :eval (copy-tree [1 2 3 4]))
"Mapping Over Vectors"
(mapcar
:eval (mapcar #'identity [1 2 3]))
diff --git a/lisp/subr.el b/lisp/subr.el
index 03d3324f3d8..83735933963 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -824,26 +824,26 @@ of course, also replace TO with a slightly larger value
next (+ from (* n inc)))))
(nreverse seq))))
-(defun copy-tree (tree &optional vecp)
+(defun copy-tree (tree &optional vector-like-p)
"Make a copy of TREE.
If TREE is a cons cell, this recursively copies both its car and its cdr.
Contrast to `copy-sequence', which copies only along the cdrs. With second
-argument VECP, this copies vectors as well as conses."
+argument VECTOR-LIKE-P, this copies vectors and records as well as conses."
(declare (side-effect-free error-free))
(if (consp tree)
(let (result)
(while (consp tree)
(let ((newcar (car tree)))
- (if (or (consp (car tree)) (and vecp (vectorp (car tree))))
- (setq newcar (copy-tree (car tree) vecp)))
+ (if (or (consp (car tree)) (and vector-like-p (or (vectorp (car tree)) (recordp (car tree)))))
+ (setq newcar (copy-tree (car tree) vector-like-p)))
(push newcar result))
(setq tree (cdr tree)))
(nconc (nreverse result)
- (if (and vecp (vectorp tree)) (copy-tree tree vecp) tree)))
- (if (and vecp (vectorp tree))
+ (if (and vector-like-p (or (vectorp tree) (recordp tree))) (copy-tree tree vector-like-p) tree)))
+ (if (and vector-like-p (or (vectorp tree) (recordp tree)))
(let ((i (length (setq tree (copy-sequence tree)))))
(while (>= (setq i (1- i)) 0)
- (aset tree i (copy-tree (aref tree i) vecp)))
+ (aset tree i (copy-tree (aref tree i) vector-like-p)))
tree)
tree)))
diff --git a/test/lisp/subr-tests.el b/test/lisp/subr-tests.el
index 8f46c2af136..4ebb68556be 100644
--- a/test/lisp/subr-tests.el
+++ b/test/lisp/subr-tests.el
@@ -1206,5 +1206,36 @@ final or penultimate step during initialization."))
(should (equal a-dedup '("a" "b" "a" "b" "c")))
(should (eq a a-dedup))))
+(ert-deftest subr--copy-tree ()
+ (should (eq (copy-tree nil) nil))
+ (let* ((a (list (list "a") "b" (list "c") "g"))
+ (copy1 (copy-tree a))
+ (copy2 (copy-tree a t)))
+ (should (equal a copy1))
+ (should (equal a copy2))
+ (should-not (eq a copy1))
+ (should-not (eq a copy2)))
+ (let* ((a (list (list "a") "b" (list "c" (record 'foo "d")) (list ["e" "f"]) "g"))
+ (copy1 (copy-tree a))
+ (copy2 (copy-tree a t)))
+ (should (equal a copy1))
+ (should (equal a copy2))
+ (should-not (eq a copy1))
+ (should-not (eq a copy2)))
+ (let* ((a (record 'foo "a" (record 'bar "b")))
+ (copy1 (copy-tree a))
+ (copy2 (copy-tree a t)))
+ (should (equal a copy1))
+ (should (equal a copy2))
+ (should (eq a copy1))
+ (should-not (eq a copy2)))
+ (let* ((a ["a" "b" ["c" ["d"]]])
+ (copy1 (copy-tree a))
+ (copy2 (copy-tree a t)))
+ (should (equal a copy1))
+ (should (equal a copy2))
+ (should (eq a copy1))
+ (should-not (eq a copy2))))
+
(provide 'subr-tests)
;;; subr-tests.el ends here
--
2.40.1
^ permalink raw reply related [relevance 7%]
* bug#63513: [PATCH] Make persist-defvar work with records and hash tables
2023-05-15 5:56 10% bug#63513: [PATCH] Make persist-defvar work with records and hash tables Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2023-05-15 11:31 0% ` Eli Zaretskii
2023-05-23 20:14 9% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
0 siblings, 1 reply; 63+ results
From: Eli Zaretskii @ 2023-05-15 11:31 UTC (permalink / raw)
To: Joseph Turner, Phillip Lord, Stefan Monnier; +Cc: adam, 63513
> Cc: Adam Porter <adam@alphapapa.net>
> Date: Sun, 14 May 2023 22:56:20 -0700
> From: Joseph Turner via "Bug reports for GNU Emacs,
> the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
>
> persist-defvar does not persist records or hash tables correctly.
>
> Here's a minimal example with a hash table:
>
> (progn
> (persist-defvar foo (make-hash-table :test #'equal) "docstring")
> (puthash 'bar t foo)
> (persist-save 'foo)
> (gethash 'bar (persist-default 'foo))) ;; Expected nil, got t
>
> This patch fixes persisting records by using copy-tree. Currently,
> copy-tree does not work with records (see
> <https://lists.gnu.org/archive/html/bug-gnu-emacs/2023-05/msg00875.html>).
> The following snippet will return the expected value only when both this
> patch and the above-linked patch are applied:
>
> (progn
> (cl-defstruct baz a)
> (persist-defvar quux (make-baz :a nil) "docstring")
> (setf (baz-a quux) t)
> (persist-save 'quux)
> (baz-a (persist-default 'quux))) ;; Expected nil, got t
>
> Before applying this patch, the updated values in both cases are not
> persisted to disk. With the patch, the updated values are persisted as
> expected.
Philip, any comments?
^ permalink raw reply [relevance 0%]
* bug#63509: [PATCH] Make copy-tree work with records
2023-05-15 3:57 10% bug#63509: [PATCH] Make copy-tree work with records Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2023-05-15 11:26 0% ` Eli Zaretskii
2023-05-15 17:59 7% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
0 siblings, 1 reply; 63+ results
From: Eli Zaretskii @ 2023-05-15 11:26 UTC (permalink / raw)
To: Joseph Turner, Stefan Monnier; +Cc: 63509
> Date: Sun, 14 May 2023 20:57:17 -0700
> From: Joseph Turner via "Bug reports for GNU Emacs,
> the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
>
> copy-tree does not currently work with records:
It isn't documented to work with anything but cons cells or vectors.
> Attached patch fixes this behavior.
Thanks, but such a change must come with:
- NEWS entry
- update for the ELisp manual
- preferably added tests to regression-test this feature
I also think we should rename the second argument, as VECP no longer
fits.
> Please tell me if I misunderstand the intended behavior of copy-tree.
Adding Stefan in case he has comments.
^ permalink raw reply [relevance 0%]
* bug#63336: [PATCH] package-vc: Process :make and :shell-command spec args
2023-05-14 23:01 8% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2023-05-15 9:12 5% ` Philip Kaludercic
2023-05-15 19:03 5% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
0 siblings, 1 reply; 63+ results
From: Philip Kaludercic @ 2023-05-15 9:12 UTC (permalink / raw)
To: Joseph Turner; +Cc: 63336
Joseph Turner <joseph@breatheoutbreathe.in> writes:
> Philip Kaludercic <philipk@posteo.net> writes:
>
>> Joseph Turner <joseph@breatheoutbreathe.in> writes:
>>
>>> Philip Kaludercic <philipk@posteo.net> writes:
>>>
>>>> Joseph Turner <joseph@breatheoutbreathe.in> writes:
>>>>
>>>>> Philip Kaludercic <philipk@posteo.net> writes:
>>>>>
>>>>>> Joseph Turner <joseph@breatheoutbreathe.in> writes:
>
>>> We also might want to add another option for
>>> package-vc-allow-side-effects like 'user-defined, which only runs :make
>>> and :shell-command args which were specified by the user (as opposed to
>>> those which were downloaded from elpa). WDYT?
>>
>> That sounds like a good idea, but let us do that in a separate patch.
>
> Okay!
>
>>> To update the manual, shall I edit doc/emacs/package.texi directly or is
>>> there another file to edit?
>>
>> Yes, just update the table under the "Specifying Package Sources" subsection.
>
> See patch.
>
>>>> If :shell-command fails, do we really want to proceed to :make?
>>>
>>> Up to you! I was following the lead of elpa-admin.el.
>>
>> In that case let us do that too, unless there is a good reason not to.
>
> +1
>
>>> I switched the first two cases. I think pcase is readable here,
>>> especially if we add an 'user-defined option. What would you use
>>> instead?
>>
>> I would have just used a regular cond.
>>
>> --8<---------------cut here---------------start------------->8---
>> (cond
>> ((null package-vc-process-make)
>> ...)
>> ((listp package-vc-process-make)
>> ...)
>> (...))
>> --8<---------------cut here---------------end--------------->8---
>>
>> But this doesn't matter, do what you prefer.
>
> Thank you! I like pcase here.
>
>>> +Be careful when changing this option as processing :make and
>>> +:shell-command will run potentially harmful code.
>>
>> Sounds scary. I guess that is the point, but what do you think about
>> something like
>>
>> Be careful when changing this option, as installing and updating a
>> package can potentially run harmful code. If possible, allow packages
>> you trust to run code, if it is necessary for a package to be properly
>> initialised.
>
> Thank you! What do you think about the version in the attached patch?
>
>>> +When set to a list of symbols (packages), run commands for only
>>> +packages in the list. When `nil', never run commands. Otherwise
>>> +when non-`nil', run commands for any package with :make or
>>> +:shell-command specified.
>>
>> Watch out. According to (elisp) Documentation Tips, nil is not quoted.
>
> Good to know! Fixed.
>
> From 812e32ea6c3f7b2d71174658db0e272b0b4fb84b Mon Sep 17 00:00:00 2001
> From: Joseph Turner <joseph@breatheoutbreathe.in>
> Date: Sat, 13 May 2023 10:05:04 -0700
> Subject: [PATCH] package-vc: Process :make and :shell-command spec args
>
> ---
> doc/emacs/package.texi | 9 ++++++++
> lisp/emacs-lisp/package-vc.el | 42 +++++++++++++++++++++++++++++++++++
> 2 files changed, 51 insertions(+)
>
> diff --git a/doc/emacs/package.texi b/doc/emacs/package.texi
> index 6722185cb20..4f606b22e54 100644
> --- a/doc/emacs/package.texi
> +++ b/doc/emacs/package.texi
> @@ -682,6 +682,15 @@ A string providing the repository-relative name of the documentation
> file from which to build an Info file. This can be a Texinfo file or
> an Org file.
>
> +@item :make
> +A string or list of strings providing the target or targets defined in
> +the repository Makefile which should run before building the Info
> +file. Only takes effect when package-vc-allow-side-effects is non-nil.
A @var is missing here
> +
> +@item :shell-command
> +A string providing the shell command to run before building the Info
> +file. Only takes effect when package-vc-allow-side-effects is non-nil.
and here. I can take care of that.
> +
> @item :vc-backend
> A symbol naming the VC backend to use for downloading a copy of the
> package's repository (@pxref{Version Control Systems,,,emacs, The GNU
> diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
> index beca0bd00e2..d2f6d287224 100644
> --- a/lisp/emacs-lisp/package-vc.el
> +++ b/lisp/emacs-lisp/package-vc.el
> @@ -344,6 +344,38 @@ asynchronously."
> "\n")
> nil pkg-file nil 'silent))))
>
> +(defcustom package-vc-allow-side-effects nil
> + "Whether to process :make and :shell-command spec arguments.
> +
> +It may be necessary to run :make and :shell-command arguments in
> +order to initialize a package or build its documentation, but
> +please be careful when changing this option, as installing and
> +updating a package can run potentially harmful code.
> +
> +When set to a list of symbols (packages), run commands for only
> +packages in the list. When nil, never run commands. Otherwise
> +when non-nil, run commands for any package with :make or
> +:shell-command specified.
> +
> +Package specs are loaded from trusted package archives."
> + :type '(choice (const :tag "Run for all packages" t)
> + (repeat :tag "Run only for selected packages" (symbol :tag "Package name"))
> + (const :tag "Never run" nil))
> + :version "30.1")
> +
> +(defun package-vc--make (pkg-spec pkg-desc)
> + "Process :make and :shell-command PKG-SPEC arguments for PKG-DESC."
> + (let ((target (plist-get pkg-spec :make))
> + (cmd (plist-get pkg-spec :shell-command))
> + (buf (format " *package-vc make %s*" (package-desc-name pkg-desc))))
> + (when (or cmd target)
> + (with-current-buffer (get-buffer-create buf)
> + (erase-buffer)
> + (when (and cmd (/= 0 (call-process shell-file-name nil t nil shell-command-switch cmd)))
> + (warn "Failed to run %s, see buffer %S" cmd (buffer-name)))
> + (when (and target (/= 0 (apply #'call-process "make" nil t nil (if (consp target) target (list target)))))
> + (warn "Failed to make %s, see buffer %S" target (buffer-name)))))))
> +
> (declare-function org-export-to-file "ox" (backend file))
>
> (defun package-vc--build-documentation (pkg-desc file)
> @@ -486,6 +518,16 @@ documentation and marking the package as installed."
> ;; Generate package file
> (package-vc--generate-description-file pkg-desc pkg-file)
>
> + ;; Process :make and :shell-command arguments before building documentation
> + (pcase package-vc-allow-side-effects
> + ('nil ; When `nil', do nothing.
> + nil)
> + ((pred consp) ; When non-`nil' list, check if package is on the list.
> + (when (memq (package-desc-name pkg-desc) package-vc-allow-side-effects)
> + (package-vc--make pkg-spec pkg-desc)))
> + (_ ; When otherwise non-`nil', run commands.
> + (package-vc--make pkg-spec pkg-desc)))
Thinking about this again, I am still not convinced. Isn't
--8<---------------cut here---------------start------------->8---
(when (or (eq package-vc-allow-side-effects t)
(memq (package-desc-name pkg-desc)
package-vc-allow-side-effects))
(package-vc--make pkg-spec pkg-desc))
--8<---------------cut here---------------end--------------->8---
much simpler? Again, you don't have to prepare another patch, I'm just
interested in what you think.
> +
> ;; Detect a manual
> (when (executable-find "install-info")
> (dolist (doc-file (ensure-list (plist-get pkg-spec :doc)))
^ permalink raw reply [relevance 5%]
* bug#63513: [PATCH] Make persist-defvar work with records and hash tables
@ 2023-05-15 5:56 10% Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-05-15 11:31 0% ` Eli Zaretskii
0 siblings, 1 reply; 63+ results
From: Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2023-05-15 5:56 UTC (permalink / raw)
To: 63513; +Cc: Adam Porter
[-- Attachment #1: Type: text/plain, Size: 964 bytes --]
Hello!
persist-defvar does not persist records or hash tables correctly.
Here's a minimal example with a hash table:
(progn
(persist-defvar foo (make-hash-table :test #'equal) "docstring")
(puthash 'bar t foo)
(persist-save 'foo)
(gethash 'bar (persist-default 'foo))) ;; Expected nil, got t
This patch fixes persisting records by using copy-tree. Currently,
copy-tree does not work with records (see
<https://lists.gnu.org/archive/html/bug-gnu-emacs/2023-05/msg00875.html>).
The following snippet will return the expected value only when both this
patch and the above-linked patch are applied:
(progn
(cl-defstruct baz a)
(persist-defvar quux (make-baz :a nil) "docstring")
(setf (baz-a quux) t)
(persist-save 'quux)
(baz-a (persist-default 'quux))) ;; Expected nil, got t
Before applying this patch, the updated values in both cases are not
persisted to disk. With the patch, the updated values are persisted as
expected.
Best,
Joseph
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Make-persist-defvar-work-with-records-and-hash-table.patch --]
[-- Type: text/x-diff, Size: 1141 bytes --]
From b1512983fb82b9800ab6d0d1c9ca359e1251e94a Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Sun, 14 May 2023 22:32:16 -0700
Subject: [PATCH] Make persist-defvar work with records and hash tables
Previously, when persist-defvar received a record or hash table for an
initial value, updated values were not persisted. This was because the
value and default value shared the same structure.
---
persist.el | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/persist.el b/persist.el
index d80943d19e..a40fc2b297 100644
--- a/persist.el
+++ b/persist.el
@@ -118,7 +118,9 @@ (defun persist-symbol (symbol &optional initvalue)
(let ((initvalue (or initvalue (symbol-value symbol))))
(add-to-list 'persist--symbols symbol)
(put symbol 'persist t)
- (put symbol 'persist-default initvalue)))
+ (if (eq 'hash-table (type-of initvalue))
+ (put symbol 'persist-default (copy-hash-table initvalue))
+ (put symbol 'persist-default (copy-tree initvalue t)))))
(defun persist--persistant-p (symbol)
"Return non-nil if SYMBOL is a persistant variable."
--
2.40.1
^ permalink raw reply related [relevance 10%]
* bug#63509: [PATCH] Make copy-tree work with records
@ 2023-05-15 3:57 10% Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-05-15 11:26 0% ` Eli Zaretskii
0 siblings, 1 reply; 63+ results
From: Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2023-05-15 3:57 UTC (permalink / raw)
To: 63509
[-- Attachment #1: Type: text/plain, Size: 367 bytes --]
Hello,
copy-tree does not currently work with records:
(cl-defstruct foo bar)
(let* ((rec (make-foo :bar "hello"))
(copy (copy-tree rec t)))
(setf (foo-bar copy) "goodbye")
(foo-bar rec))
Expected "hello"; actual "goodbye".
Attached patch fixes this behavior.
Please tell me if I misunderstand the intended behavior of copy-tree.
Thank you,
Joseph
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Make-copy-tree-work-with-records.patch --]
[-- Type: text/x-diff, Size: 1440 bytes --]
From a20e119a0f60f85039726bf6ebcbe441060f9ef2 Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Sun, 14 May 2023 21:02:15 -0700
Subject: [PATCH] Make copy-tree work with records
---
lisp/subr.el | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/lisp/subr.el b/lisp/subr.el
index 03d3324f3d8..9573b19ae7e 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -828,7 +828,7 @@ of course, also replace TO with a slightly larger value
"Make a copy of TREE.
If TREE is a cons cell, this recursively copies both its car and its cdr.
Contrast to `copy-sequence', which copies only along the cdrs. With second
-argument VECP, this copies vectors as well as conses."
+argument VECP, this copies vectors and records as well as conses."
(declare (side-effect-free error-free))
(if (consp tree)
(let (result)
@@ -839,8 +839,8 @@ argument VECP, this copies vectors as well as conses."
(push newcar result))
(setq tree (cdr tree)))
(nconc (nreverse result)
- (if (and vecp (vectorp tree)) (copy-tree tree vecp) tree)))
- (if (and vecp (vectorp tree))
+ (if (and vecp (or (vectorp tree) (recordp tree))) (copy-tree tree vecp) tree)))
+ (if (and vecp (or (vectorp tree) (recordp tree)))
(let ((i (length (setq tree (copy-sequence tree)))))
(while (>= (setq i (1- i)) 0)
(aset tree i (copy-tree (aref tree i) vecp)))
--
2.40.1
^ permalink raw reply related [relevance 10%]
* bug#63336: [PATCH] package-vc: Process :make and :shell-command spec args
2023-05-14 19:30 5% ` Philip Kaludercic
@ 2023-05-14 23:01 8% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-05-15 9:12 5% ` Philip Kaludercic
0 siblings, 1 reply; 63+ results
From: Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2023-05-14 23:01 UTC (permalink / raw)
To: Philip Kaludercic; +Cc: 63336
[-- Attachment #1: Type: text/plain, Size: 2440 bytes --]
Philip Kaludercic <philipk@posteo.net> writes:
> Joseph Turner <joseph@breatheoutbreathe.in> writes:
>
>> Philip Kaludercic <philipk@posteo.net> writes:
>>
>>> Joseph Turner <joseph@breatheoutbreathe.in> writes:
>>>
>>>> Philip Kaludercic <philipk@posteo.net> writes:
>>>>
>>>>> Joseph Turner <joseph@breatheoutbreathe.in> writes:
>> We also might want to add another option for
>> package-vc-allow-side-effects like 'user-defined, which only runs :make
>> and :shell-command args which were specified by the user (as opposed to
>> those which were downloaded from elpa). WDYT?
>
> That sounds like a good idea, but let us do that in a separate patch.
Okay!
>> To update the manual, shall I edit doc/emacs/package.texi directly or is
>> there another file to edit?
>
> Yes, just update the table under the "Specifying Package Sources" subsection.
See patch.
>>> If :shell-command fails, do we really want to proceed to :make?
>>
>> Up to you! I was following the lead of elpa-admin.el.
>
> In that case let us do that too, unless there is a good reason not to.
+1
>> I switched the first two cases. I think pcase is readable here,
>> especially if we add an 'user-defined option. What would you use
>> instead?
>
> I would have just used a regular cond.
>
> --8<---------------cut here---------------start------------->8---
> (cond
> ((null package-vc-process-make)
> ...)
> ((listp package-vc-process-make)
> ...)
> (...))
> --8<---------------cut here---------------end--------------->8---
>
> But this doesn't matter, do what you prefer.
Thank you! I like pcase here.
>> +Be careful when changing this option as processing :make and
>> +:shell-command will run potentially harmful code.
>
> Sounds scary. I guess that is the point, but what do you think about
> something like
>
> Be careful when changing this option, as installing and updating a
> package can potentially run harmful code. If possible, allow packages
> you trust to run code, if it is necessary for a package to be properly
> initialised.
Thank you! What do you think about the version in the attached patch?
>> +When set to a list of symbols (packages), run commands for only
>> +packages in the list. When `nil', never run commands. Otherwise
>> +when non-`nil', run commands for any package with :make or
>> +:shell-command specified.
>
> Watch out. According to (elisp) Documentation Tips, nil is not quoted.
Good to know! Fixed.
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-package-vc-Process-make-and-shell-command-spec-args.patch --]
[-- Type: text/x-diff, Size: 4126 bytes --]
From 812e32ea6c3f7b2d71174658db0e272b0b4fb84b Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Sat, 13 May 2023 10:05:04 -0700
Subject: [PATCH] package-vc: Process :make and :shell-command spec args
---
doc/emacs/package.texi | 9 ++++++++
lisp/emacs-lisp/package-vc.el | 42 +++++++++++++++++++++++++++++++++++
2 files changed, 51 insertions(+)
diff --git a/doc/emacs/package.texi b/doc/emacs/package.texi
index 6722185cb20..4f606b22e54 100644
--- a/doc/emacs/package.texi
+++ b/doc/emacs/package.texi
@@ -682,6 +682,15 @@ A string providing the repository-relative name of the documentation
file from which to build an Info file. This can be a Texinfo file or
an Org file.
+@item :make
+A string or list of strings providing the target or targets defined in
+the repository Makefile which should run before building the Info
+file. Only takes effect when package-vc-allow-side-effects is non-nil.
+
+@item :shell-command
+A string providing the shell command to run before building the Info
+file. Only takes effect when package-vc-allow-side-effects is non-nil.
+
@item :vc-backend
A symbol naming the VC backend to use for downloading a copy of the
package's repository (@pxref{Version Control Systems,,,emacs, The GNU
diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
index beca0bd00e2..d2f6d287224 100644
--- a/lisp/emacs-lisp/package-vc.el
+++ b/lisp/emacs-lisp/package-vc.el
@@ -344,6 +344,38 @@ asynchronously."
"\n")
nil pkg-file nil 'silent))))
+(defcustom package-vc-allow-side-effects nil
+ "Whether to process :make and :shell-command spec arguments.
+
+It may be necessary to run :make and :shell-command arguments in
+order to initialize a package or build its documentation, but
+please be careful when changing this option, as installing and
+updating a package can run potentially harmful code.
+
+When set to a list of symbols (packages), run commands for only
+packages in the list. When nil, never run commands. Otherwise
+when non-nil, run commands for any package with :make or
+:shell-command specified.
+
+Package specs are loaded from trusted package archives."
+ :type '(choice (const :tag "Run for all packages" t)
+ (repeat :tag "Run only for selected packages" (symbol :tag "Package name"))
+ (const :tag "Never run" nil))
+ :version "30.1")
+
+(defun package-vc--make (pkg-spec pkg-desc)
+ "Process :make and :shell-command PKG-SPEC arguments for PKG-DESC."
+ (let ((target (plist-get pkg-spec :make))
+ (cmd (plist-get pkg-spec :shell-command))
+ (buf (format " *package-vc make %s*" (package-desc-name pkg-desc))))
+ (when (or cmd target)
+ (with-current-buffer (get-buffer-create buf)
+ (erase-buffer)
+ (when (and cmd (/= 0 (call-process shell-file-name nil t nil shell-command-switch cmd)))
+ (warn "Failed to run %s, see buffer %S" cmd (buffer-name)))
+ (when (and target (/= 0 (apply #'call-process "make" nil t nil (if (consp target) target (list target)))))
+ (warn "Failed to make %s, see buffer %S" target (buffer-name)))))))
+
(declare-function org-export-to-file "ox" (backend file))
(defun package-vc--build-documentation (pkg-desc file)
@@ -486,6 +518,16 @@ documentation and marking the package as installed."
;; Generate package file
(package-vc--generate-description-file pkg-desc pkg-file)
+ ;; Process :make and :shell-command arguments before building documentation
+ (pcase package-vc-allow-side-effects
+ ('nil ; When `nil', do nothing.
+ nil)
+ ((pred consp) ; When non-`nil' list, check if package is on the list.
+ (when (memq (package-desc-name pkg-desc) package-vc-allow-side-effects)
+ (package-vc--make pkg-spec pkg-desc)))
+ (_ ; When otherwise non-`nil', run commands.
+ (package-vc--make pkg-spec pkg-desc)))
+
;; Detect a manual
(when (executable-find "install-info")
(dolist (doc-file (ensure-list (plist-get pkg-spec :doc)))
--
2.40.1
^ permalink raw reply related [relevance 8%]
* bug#63336: [PATCH] package-vc: Process :make and :shell-command spec args
2023-05-14 8:08 8% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2023-05-14 19:30 5% ` Philip Kaludercic
2023-05-14 23:01 8% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
0 siblings, 1 reply; 63+ results
From: Philip Kaludercic @ 2023-05-14 19:30 UTC (permalink / raw)
To: Joseph Turner; +Cc: 63336
Joseph Turner <joseph@breatheoutbreathe.in> writes:
> Philip Kaludercic <philipk@posteo.net> writes:
>
>> Joseph Turner <joseph@breatheoutbreathe.in> writes:
>>
>>> Philip Kaludercic <philipk@posteo.net> writes:
>>>
>>>> Joseph Turner <joseph@breatheoutbreathe.in> writes:
>>> From b27724197acd4ee72f9d336843f0e6ed9fcee87b Mon Sep 17 00:00:00 2001
>>> From: Joseph Turner <joseph@breatheoutbreathe.in>
>>> Date: Sat, 13 May 2023 10:05:04 -0700
>>> Subject: [PATCH] package-vc: Process :make and :shell-command spec args
>>>
>>> ---
>>> lisp/emacs-lisp/package-vc.el | 37 +++++++++++++++++++++++++++++++++++
>>> 1 file changed, 37 insertions(+)
>>>
>>> diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
>>> index beca0bd00e2..8529d1dad5c 100644
>>> --- a/lisp/emacs-lisp/package-vc.el
>>> +++ b/lisp/emacs-lisp/package-vc.el
>>> @@ -344,6 +344,33 @@ asynchronously."
>>> "\n")
>>> nil pkg-file nil 'silent))))
>>>
>>> +(defcustom package-vc-process-make nil
>>
>> Have we discussed the name of this user option? I feel it is too
>> immediate, and therefore not intuitively obvious what purpose it serves.
>> I would imagine something along the lines of
>> "package-vc-allow-side-effects" or "package-vc-permit-building" could be
>> better? WDYT?
>
> I like "package-vc-allow-side-effects". Changed in attached patch.
>
>>> + "Whether to process :make and :shell-command spec arguments.
>>
>> I guess here too an explanation would be warranted (and in the manual).
>> Explaining what the issue is, and why one might be wary to enable the option.
>
> Does my addition suffice?
>
> We also might want to add another option for
> package-vc-allow-side-effects like 'user-defined, which only runs :make
> and :shell-command args which were specified by the user (as opposed to
> those which were downloaded from elpa). WDYT?
That sounds like a good idea, but let us do that in a separate patch.
> To update the manual, shall I edit doc/emacs/package.texi directly or is
> there another file to edit?
Yes, just update the table under the "Specifying Package Sources" subsection.
>>> +When set to a list of symbols (packages), run commands for only
>>> +packages in the list. When `nil', never run commands. Otherwise
>>> +when non-`nil', run commands for any package with :make or
>>> +:shell-command specified.
>>> +
>>> +Package specs are loaded from trusted package archives."
>>> + :type '(choice (const :tag "Run for all packages" t)
>>> + (repeat :tag "Run only for selected packages" (symbol :tag "Package name"))
>>> + (const :tag "Never run" nil))
>>> + :version "30.1")
>>> +
>>> +(defun package-vc--make (pkg-spec pkg-desc)
>>> + "Process :make and :shell-command PKG-SPEC arguments for PKG-DESC."
>>> + (let ((target (plist-get pkg-spec :make))
>>> + (cmd (plist-get pkg-spec :shell-command)))
>>> + (when (or cmd target)
>>> + (with-current-buffer (get-buffer-create
>>
>> I'd format the buffer name in the top let to prevent this line-break here.
>
> Done.
>
>>> + (format " *package-vc make %s*" (package-desc-name pkg-desc)))
>>> + (erase-buffer)
>>> + (when (and cmd (/= 0 (call-process shell-file-name nil t nil shell-command-switch cmd)))
>>> + (warn "Failed to run %s, see buffer %S" cmd (buffer-name)))
>>> + (when (and target (/= 0 (apply #'call-process "make" nil t nil (if (consp target) target (list target)))))
>>> + (warn "Failed to make %s, see buffer %S" target (buffer-name)))))))
>>
>> If :shell-command fails, do we really want to proceed to :make?
>
> Up to you! I was following the lead of elpa-admin.el.
In that case let us do that too, unless there is a good reason not to.
>>> (declare-function org-export-to-file "ox" (backend file))
>>>
>>> (defun package-vc--build-documentation (pkg-desc file)
>>> @@ -486,6 +513,16 @@ documentation and marking the package as installed."
>>> ;; Generate package file
>>> (package-vc--generate-description-file pkg-desc pkg-file)
>>>
>>> + ;; Process :make and :shell-command arguments before building documentation
>>> + (pcase package-vc-process-make
>>> + ((pred consp) ; When non-`nil' list, check if package is on the list.
>>> + (when (memq (package-desc-name pkg-desc) package-vc-process-make)
>>> + (package-vc--make pkg-spec pkg-desc)))
>>> + ('nil ; When `nil', do nothing.
>>> + nil)
>>
>> Perhaps swap the two conditions, first checking nil then listp which I
>> think reads more natural. Then again, is pcase actually serving
>> anything here?
>
> I switched the first two cases. I think pcase is readable here,
> especially if we add an 'user-defined option. What would you use
> instead?
I would have just used a regular cond.
--8<---------------cut here---------------start------------->8---
(cond
((null package-vc-process-make)
...)
((listp package-vc-process-make)
...)
(...))
--8<---------------cut here---------------end--------------->8---
But this doesn't matter, do what you prefer.
>>> + (_ ; When otherwise non-`nil', run commands.
>>> + (package-vc--make pkg-spec pkg-desc)))
>>> +
>>> ;; Detect a manual
>>> (when (executable-find "install-info")
>>> (dolist (doc-file (ensure-list (plist-get pkg-spec :doc)))
>
> From 3e7084e8e3e3ba142f383e90bfa656f59f3cc1ad Mon Sep 17 00:00:00 2001
> From: Joseph Turner <joseph@breatheoutbreathe.in>
> Date: Sat, 13 May 2023 10:05:04 -0700
> Subject: [PATCH] package-vc: Process :make and :shell-command spec args
>
> ---
> lisp/emacs-lisp/package-vc.el | 40 +++++++++++++++++++++++++++++++++++
> 1 file changed, 40 insertions(+)
>
> diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
> index beca0bd00e2..8403add364c 100644
> --- a/lisp/emacs-lisp/package-vc.el
> +++ b/lisp/emacs-lisp/package-vc.el
> @@ -344,6 +344,36 @@ asynchronously."
> "\n")
> nil pkg-file nil 'silent))))
>
> +(defcustom package-vc-allow-side-effects nil
> + "Whether to process :make and :shell-command spec arguments.
> +
> +Be careful when changing this option as processing :make and
> +:shell-command will run potentially harmful code.
Sounds scary. I guess that is the point, but what do you think about
something like
Be careful when changing this option, as installing and updating a
package can potentially run harmful code. If possible, allow packages
you trust to run code, if it is necessary for a package to be properly
initialised.
> +
> +When set to a list of symbols (packages), run commands for only
> +packages in the list. When `nil', never run commands. Otherwise
> +when non-`nil', run commands for any package with :make or
> +:shell-command specified.
Watch out. According to (elisp) Documentation Tips, nil is not quoted.
> +
> +Package specs are loaded from trusted package archives."
> + :type '(choice (const :tag "Run for all packages" t)
> + (repeat :tag "Run only for selected packages" (symbol :tag "Package name"))
> + (const :tag "Never run" nil))
> + :version "30.1")
> +
> +(defun package-vc--make (pkg-spec pkg-desc)
> + "Process :make and :shell-command PKG-SPEC arguments for PKG-DESC."
> + (let ((target (plist-get pkg-spec :make))
> + (cmd (plist-get pkg-spec :shell-command))
> + (buf (format " *package-vc make %s*" (package-desc-name pkg-desc))))
> + (when (or cmd target)
> + (with-current-buffer (get-buffer-create buf)
> + (erase-buffer)
> + (when (and cmd (/= 0 (call-process shell-file-name nil t nil shell-command-switch cmd)))
> + (warn "Failed to run %s, see buffer %S" cmd (buffer-name)))
> + (when (and target (/= 0 (apply #'call-process "make" nil t nil (if (consp target) target (list target)))))
> + (warn "Failed to make %s, see buffer %S" target (buffer-name)))))))
> +
> (declare-function org-export-to-file "ox" (backend file))
>
> (defun package-vc--build-documentation (pkg-desc file)
> @@ -486,6 +516,16 @@ documentation and marking the package as installed."
> ;; Generate package file
> (package-vc--generate-description-file pkg-desc pkg-file)
>
> + ;; Process :make and :shell-command arguments before building documentation
> + (pcase package-vc-allow-side-effects
> + ('nil ; When `nil', do nothing.
> + nil)
> + ((pred consp) ; When non-`nil' list, check if package is on the list.
> + (when (memq (package-desc-name pkg-desc) package-vc-allow-side-effects)
> + (package-vc--make pkg-spec pkg-desc)))
> + (_ ; When otherwise non-`nil', run commands.
> + (package-vc--make pkg-spec pkg-desc)))
> +
> ;; Detect a manual
> (when (executable-find "install-info")
> (dolist (doc-file (ensure-list (plist-get pkg-spec :doc)))
^ permalink raw reply [relevance 5%]
* bug#63336: [PATCH] package-vc: Process :make and :shell-command spec args
2023-05-14 7:44 5% ` Philip Kaludercic
@ 2023-05-14 8:08 8% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-05-14 19:30 5% ` Philip Kaludercic
0 siblings, 1 reply; 63+ results
From: Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2023-05-14 8:08 UTC (permalink / raw)
To: Philip Kaludercic; +Cc: 63336
[-- Attachment #1: Type: text/plain, Size: 4703 bytes --]
Philip Kaludercic <philipk@posteo.net> writes:
> Joseph Turner <joseph@breatheoutbreathe.in> writes:
>
>> Philip Kaludercic <philipk@posteo.net> writes:
>>
>>> Joseph Turner <joseph@breatheoutbreathe.in> writes:
>> From b27724197acd4ee72f9d336843f0e6ed9fcee87b Mon Sep 17 00:00:00 2001
>> From: Joseph Turner <joseph@breatheoutbreathe.in>
>> Date: Sat, 13 May 2023 10:05:04 -0700
>> Subject: [PATCH] package-vc: Process :make and :shell-command spec args
>>
>> ---
>> lisp/emacs-lisp/package-vc.el | 37 +++++++++++++++++++++++++++++++++++
>> 1 file changed, 37 insertions(+)
>>
>> diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
>> index beca0bd00e2..8529d1dad5c 100644
>> --- a/lisp/emacs-lisp/package-vc.el
>> +++ b/lisp/emacs-lisp/package-vc.el
>> @@ -344,6 +344,33 @@ asynchronously."
>> "\n")
>> nil pkg-file nil 'silent))))
>>
>> +(defcustom package-vc-process-make nil
>
> Have we discussed the name of this user option? I feel it is too
> immediate, and therefore not intuitively obvious what purpose it serves.
> I would imagine something along the lines of
> "package-vc-allow-side-effects" or "package-vc-permit-building" could be
> better? WDYT?
I like "package-vc-allow-side-effects". Changed in attached patch.
>> + "Whether to process :make and :shell-command spec arguments.
>
> I guess here too an explanation would be warranted (and in the manual).
> Explaining what the issue is, and why one might be wary to enable the option.
Does my addition suffice?
We also might want to add another option for
package-vc-allow-side-effects like 'user-defined, which only runs :make
and :shell-command args which were specified by the user (as opposed to
those which were downloaded from elpa). WDYT?
To update the manual, shall I edit doc/emacs/package.texi directly or is
there another file to edit?
>> +When set to a list of symbols (packages), run commands for only
>> +packages in the list. When `nil', never run commands. Otherwise
>> +when non-`nil', run commands for any package with :make or
>> +:shell-command specified.
>> +
>> +Package specs are loaded from trusted package archives."
>> + :type '(choice (const :tag "Run for all packages" t)
>> + (repeat :tag "Run only for selected packages" (symbol :tag "Package name"))
>> + (const :tag "Never run" nil))
>> + :version "30.1")
>> +
>> +(defun package-vc--make (pkg-spec pkg-desc)
>> + "Process :make and :shell-command PKG-SPEC arguments for PKG-DESC."
>> + (let ((target (plist-get pkg-spec :make))
>> + (cmd (plist-get pkg-spec :shell-command)))
>> + (when (or cmd target)
>> + (with-current-buffer (get-buffer-create
>
> I'd format the buffer name in the top let to prevent this line-break here.
Done.
>> + (format " *package-vc make %s*" (package-desc-name pkg-desc)))
>> + (erase-buffer)
>> + (when (and cmd (/= 0 (call-process shell-file-name nil t nil shell-command-switch cmd)))
>> + (warn "Failed to run %s, see buffer %S" cmd (buffer-name)))
>> + (when (and target (/= 0 (apply #'call-process "make" nil t nil (if (consp target) target (list target)))))
>> + (warn "Failed to make %s, see buffer %S" target (buffer-name)))))))
>
> If :shell-command fails, do we really want to proceed to :make?
Up to you! I was following the lead of elpa-admin.el.
>> (declare-function org-export-to-file "ox" (backend file))
>>
>> (defun package-vc--build-documentation (pkg-desc file)
>> @@ -486,6 +513,16 @@ documentation and marking the package as installed."
>> ;; Generate package file
>> (package-vc--generate-description-file pkg-desc pkg-file)
>>
>> + ;; Process :make and :shell-command arguments before building documentation
>> + (pcase package-vc-process-make
>> + ((pred consp) ; When non-`nil' list, check if package is on the list.
>> + (when (memq (package-desc-name pkg-desc) package-vc-process-make)
>> + (package-vc--make pkg-spec pkg-desc)))
>> + ('nil ; When `nil', do nothing.
>> + nil)
>
> Perhaps swap the two conditions, first checking nil then listp which I
> think reads more natural. Then again, is pcase actually serving
> anything here?
I switched the first two cases. I think pcase is readable here,
especially if we add an 'user-defined option. What would you use
instead?
>> + (_ ; When otherwise non-`nil', run commands.
>> + (package-vc--make pkg-spec pkg-desc)))
>> +
>> ;; Detect a manual
>> (when (executable-find "install-info")
>> (dolist (doc-file (ensure-list (plist-get pkg-spec :doc)))
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-package-vc-Process-make-and-shell-command-spec-args.patch --]
[-- Type: text/x-diff, Size: 3065 bytes --]
From 3e7084e8e3e3ba142f383e90bfa656f59f3cc1ad Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Sat, 13 May 2023 10:05:04 -0700
Subject: [PATCH] package-vc: Process :make and :shell-command spec args
---
lisp/emacs-lisp/package-vc.el | 40 +++++++++++++++++++++++++++++++++++
1 file changed, 40 insertions(+)
diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
index beca0bd00e2..8403add364c 100644
--- a/lisp/emacs-lisp/package-vc.el
+++ b/lisp/emacs-lisp/package-vc.el
@@ -344,6 +344,36 @@ asynchronously."
"\n")
nil pkg-file nil 'silent))))
+(defcustom package-vc-allow-side-effects nil
+ "Whether to process :make and :shell-command spec arguments.
+
+Be careful when changing this option as processing :make and
+:shell-command will run potentially harmful code.
+
+When set to a list of symbols (packages), run commands for only
+packages in the list. When `nil', never run commands. Otherwise
+when non-`nil', run commands for any package with :make or
+:shell-command specified.
+
+Package specs are loaded from trusted package archives."
+ :type '(choice (const :tag "Run for all packages" t)
+ (repeat :tag "Run only for selected packages" (symbol :tag "Package name"))
+ (const :tag "Never run" nil))
+ :version "30.1")
+
+(defun package-vc--make (pkg-spec pkg-desc)
+ "Process :make and :shell-command PKG-SPEC arguments for PKG-DESC."
+ (let ((target (plist-get pkg-spec :make))
+ (cmd (plist-get pkg-spec :shell-command))
+ (buf (format " *package-vc make %s*" (package-desc-name pkg-desc))))
+ (when (or cmd target)
+ (with-current-buffer (get-buffer-create buf)
+ (erase-buffer)
+ (when (and cmd (/= 0 (call-process shell-file-name nil t nil shell-command-switch cmd)))
+ (warn "Failed to run %s, see buffer %S" cmd (buffer-name)))
+ (when (and target (/= 0 (apply #'call-process "make" nil t nil (if (consp target) target (list target)))))
+ (warn "Failed to make %s, see buffer %S" target (buffer-name)))))))
+
(declare-function org-export-to-file "ox" (backend file))
(defun package-vc--build-documentation (pkg-desc file)
@@ -486,6 +516,16 @@ documentation and marking the package as installed."
;; Generate package file
(package-vc--generate-description-file pkg-desc pkg-file)
+ ;; Process :make and :shell-command arguments before building documentation
+ (pcase package-vc-allow-side-effects
+ ('nil ; When `nil', do nothing.
+ nil)
+ ((pred consp) ; When non-`nil' list, check if package is on the list.
+ (when (memq (package-desc-name pkg-desc) package-vc-allow-side-effects)
+ (package-vc--make pkg-spec pkg-desc)))
+ (_ ; When otherwise non-`nil', run commands.
+ (package-vc--make pkg-spec pkg-desc)))
+
;; Detect a manual
(when (executable-find "install-info")
(dolist (doc-file (ensure-list (plist-get pkg-spec :doc)))
--
2.40.1
^ permalink raw reply related [relevance 8%]
* bug#63336: [PATCH] package-vc: Process :make and :shell-command spec args
2023-05-11 1:37 9% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2023-05-14 7:44 5% ` Philip Kaludercic
2023-05-14 8:08 8% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
0 siblings, 1 reply; 63+ results
From: Philip Kaludercic @ 2023-05-14 7:44 UTC (permalink / raw)
To: Joseph Turner; +Cc: 63336
Joseph Turner <joseph@breatheoutbreathe.in> writes:
> Philip Kaludercic <philipk@posteo.net> writes:
>
>> Joseph Turner <joseph@breatheoutbreathe.in> writes:
>>
>>>>> +(defun package-vc--make (pkg-spec dir)
>>>>> + "Process :make and :shell-command spec arguments."
>>>>> + (let ((target (plist-get pkg-spec :make))
>>>>> + (cmd (plist-get pkg-spec :shell-command)))
>>>>> + (when (or cmd target)
>>>>> + (with-current-buffer (get-buffer-create " *package-vc make*")
>>>> ^
>>>> should the package name
>>>> be mentioned here?
>>>
>>> I like this idea, but IIUC package-vc--make would then need to take an
>>> extra arg, since pkg-spec doesn't contain the :name of the package. We
>>> could also add :name to the pkg-spec plist?
>>
>> I wouldn't be in favour of that, I think that passing the name as a
>> separate argument would be a better solution.
>
> I agree.
>
>>> For comparison, package-vc--build-documentation creates a buffer called
>>> " *package-vc doc*" without the package name.
>>
>> The difference I see here is that documentation usually builds fine,
>> while :make or :shell-command have a higher chance of failing because
>> some software is missing, especially if people don't use :make the way
>> it is used on the ELPA server but to build external dependencies (I'm
>> thinking of mail clients like notmuch)
>
> That makes sense to me. In the attached patch, I pass pkg-desc to
> package-vc--make instead just name.
>
> Want me to submit a separate patch which adds the package name to the
> " *package-vc doc*" buffer name?
No, I don't think it is necessary. But thanks.
>>>>> + target (buffer-name)))))))
>>>>> +
>>>>> (declare-function org-export-to-file "ox" (backend file))
>>>>>
>>>>> (defun package-vc--build-documentation (pkg-desc file)
>>>>> @@ -486,6 +515,9 @@ documentation and marking the package as installed."
>>>>> ;; Generate package file
>>>>> (package-vc--generate-description-file pkg-desc pkg-file)
>>>>>
>>>>> + ;; Process :make and :shell-command arguments before building documentation
>>>>> + (when package-vc-process-make (package-vc--make pkg-spec pkg-dir))
>>>>
>>>> Wasn't the plan to allow `package-vc-process-make' to either be a
>>>> generic "build-anything" or a selective listing of packages where we
>>>> allow :make and :shell-command to be executed?
>>>
>>> Let me know if the attached commit accomplishes what you had in mind.
>>
>> Yes, that (or rather the newest version from a different message) looks good.
>>
>>>>> +
>>>>> ;; Detect a manual
>>>>> (when (executable-find "install-info")
>>>>> (dolist (doc-file (ensure-list (plist-get pkg-spec :doc)))
>>>>
>>>> Otherwise this looks good, but I haven't tried it out yet.
>>>
>>> I fixed up a couple other issues:
>>>
>>> - removed unnecessary dir arg to package-vc--make
>>> - added function arg to the docstring for package-vc--make
>>>
>>> I'm not sure if the customization type for package-vc-process-make is
>>> correct. Please double check that.
>>>
>>> Also, should users be able to run :make and :shell-command args defined
>>> in a spec passed into package-vc-install?
>>
>> Yes, is that currently not supported?
>
> Nevermind! It is supported. I didn't notice that package-vc--unpack adds
> the user-defined pkg-spec to package-vc-selected-packages just before
> calling package-vc--unpack-1.
1+
> Best,
>
> Joseph
>
> From b27724197acd4ee72f9d336843f0e6ed9fcee87b Mon Sep 17 00:00:00 2001
> From: Joseph Turner <joseph@breatheoutbreathe.in>
> Date: Sat, 13 May 2023 10:05:04 -0700
> Subject: [PATCH] package-vc: Process :make and :shell-command spec args
>
> ---
> lisp/emacs-lisp/package-vc.el | 37 +++++++++++++++++++++++++++++++++++
> 1 file changed, 37 insertions(+)
>
> diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
> index beca0bd00e2..8529d1dad5c 100644
> --- a/lisp/emacs-lisp/package-vc.el
> +++ b/lisp/emacs-lisp/package-vc.el
> @@ -344,6 +344,33 @@ asynchronously."
> "\n")
> nil pkg-file nil 'silent))))
>
> +(defcustom package-vc-process-make nil
Have we discussed the name of this user option? I feel it is too
immediate, and therefore not intuitively obvious what purpose it serves.
I would imagine something along the lines of
"package-vc-allow-side-effects" or "package-vc-permit-building" could be
better? WDYT?
> + "Whether to process :make and :shell-command spec arguments.
I guess here too an explanation would be warranted (and in the manual).
Explaining what the issue is, and why one might be wary to enable the option.
> +When set to a list of symbols (packages), run commands for only
> +packages in the list. When `nil', never run commands. Otherwise
> +when non-`nil', run commands for any package with :make or
> +:shell-command specified.
> +
> +Package specs are loaded from trusted package archives."
> + :type '(choice (const :tag "Run for all packages" t)
> + (repeat :tag "Run only for selected packages" (symbol :tag "Package name"))
> + (const :tag "Never run" nil))
> + :version "30.1")
> +
> +(defun package-vc--make (pkg-spec pkg-desc)
> + "Process :make and :shell-command PKG-SPEC arguments for PKG-DESC."
> + (let ((target (plist-get pkg-spec :make))
> + (cmd (plist-get pkg-spec :shell-command)))
> + (when (or cmd target)
> + (with-current-buffer (get-buffer-create
I'd format the buffer name in the top let to prevent this line-break here.
> + (format " *package-vc make %s*" (package-desc-name pkg-desc)))
> + (erase-buffer)
> + (when (and cmd (/= 0 (call-process shell-file-name nil t nil shell-command-switch cmd)))
> + (warn "Failed to run %s, see buffer %S" cmd (buffer-name)))
> + (when (and target (/= 0 (apply #'call-process "make" nil t nil (if (consp target) target (list target)))))
> + (warn "Failed to make %s, see buffer %S" target (buffer-name)))))))
If :shell-command fails, do we really want to proceed to :make?
> (declare-function org-export-to-file "ox" (backend file))
>
> (defun package-vc--build-documentation (pkg-desc file)
> @@ -486,6 +513,16 @@ documentation and marking the package as installed."
> ;; Generate package file
> (package-vc--generate-description-file pkg-desc pkg-file)
>
> + ;; Process :make and :shell-command arguments before building documentation
> + (pcase package-vc-process-make
> + ((pred consp) ; When non-`nil' list, check if package is on the list.
> + (when (memq (package-desc-name pkg-desc) package-vc-process-make)
> + (package-vc--make pkg-spec pkg-desc)))
> + ('nil ; When `nil', do nothing.
> + nil)
Perhaps swap the two conditions, first checking nil then listp which I
think reads more natural. Then again, is pcase actually serving
anything here?
> + (_ ; When otherwise non-`nil', run commands.
> + (package-vc--make pkg-spec pkg-desc)))
> +
> ;; Detect a manual
> (when (executable-find "install-info")
> (dolist (doc-file (ensure-list (plist-get pkg-spec :doc)))
^ permalink raw reply [relevance 5%]
* bug#63337: [PATCH] package-vc--build-documentation: Fix relative @include statements
2023-05-13 17:14 5% ` Philip Kaludercic
@ 2023-05-13 18:31 5% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
0 siblings, 0 replies; 63+ results
From: Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2023-05-13 18:31 UTC (permalink / raw)
To: Philip Kaludercic; +Cc: 63337
Philip Kaludercic <philipk@posteo.net> writes:
> Joseph Turner <joseph@breatheoutbreathe.in> writes:
>
>> Philip Kaludercic <philipk@posteo.net> writes:
>>
>>> Joseph Turner <joseph@breatheoutbreathe.in> writes:
>>>
>>>> Philip Kaludercic <philipk@posteo.net> writes:
>>>>
>>>>> Joseph Turner <joseph@breatheoutbreathe.in> writes:
>>>>>
>>>>>> Hello!
>>>>>>
>>>>>> Because package-vc--build-documentation exports the texinfo manual to a
>>>>>> temp file inside /tmp/ , any @include statements with relative paths
>>>>>> break the makeinfo call.
>>>>>>
>>>>>> I noticed this issue when attempting to use package-vc to install
>>>>>> org-transclusion, whose manual contains the line
>>>>>>
>>>>>> #+texinfo: @include fdl.texi
>>>>>>
>>>>>> See: https://raw.githubusercontent.com/nobiot/org-transclusion/main/docs/org-transclusion-manual.org
>>>>>>
>>>>>> The attached patch solves this problem by passing the -I flag to
>>>>>> makeinfo. From makeinfo --help:
>>>>>>
>>>>>> -I DIR append DIR to the @include search path.
>>
>>>>> According to the docs, makeinfo has -I to append the search path, and -P
>>>>> to prepend. I don't know how well either of the two are supported, but
>>>>> assuming they are, shouldn't -P be preferred? Or wouldn't it have any
>>>>> effect?
>>>>
>>>> I am not sure what difference it would make. I don't know if the default
>>>> @include search path includes anything besides the working directory.
>>>>
>>>> In the attached diff, I have changed -I to -P.
>>>
>>> I can confirm that this patch does the right thing, and I think we
>>> should apply it.
>>
>> I think Eli suggested we prepend (-I) instead of append (-P), as in the
>> very first patch I sent, also attached here.
>
> I did not understand the argument, but it probably does not matter that
> much. As this patch has Eli's blessing, I will apply it.
Great! Thank you!!
>
>> From a41abce88ed3b833c5531208945474c9cd16284b Mon Sep 17 00:00:00 2001
>> From: Joseph Turner <joseph@breatheoutbreathe.in>
>> Date: Sat, 6 May 2023 14:49:43 -0700
>> Subject: [PATCH] Fix: (package-vc--build-documentation) Relative @include
>> statements
>>
>> ---
>> lisp/emacs-lisp/package-vc.el | 5 ++++-
>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
>> index 489610e2a1e..63c10285ca7 100644
>> --- a/lisp/emacs-lisp/package-vc.el
>> +++ b/lisp/emacs-lisp/package-vc.el
>> @@ -381,6 +381,7 @@ FILE can be an Org file, indicated by its \".org\" extension,
>> otherwise it's assumed to be an Info file."
>> (let* ((pkg-name (package-desc-name pkg-desc))
>> (default-directory (package-desc-dir pkg-desc))
>> + (docs-directory (expand-file-name (file-name-directory file)))
>> (output (expand-file-name (format "%s.info" pkg-name)))
>> clean-up)
>> (when (string-match-p "\\.org\\'" file)
>> @@ -395,7 +396,9 @@ otherwise it's assumed to be an Info file."
>> (erase-buffer)
>> (cond
>> ((/= 0 (call-process "makeinfo" nil t nil
>> - "--no-split" file "-o" output))
>> + "-I" docs-directory
>> + "--no-split" file
>> + "-o" output))
>> (message "Failed to build manual %s, see buffer %S"
>> file (buffer-name)))
>> ((/= 0 (call-process "install-info" nil t nil
^ permalink raw reply [relevance 5%]
* bug#63336: [PATCH] package-vc: Process :make and :shell-command spec args
2023-05-10 6:35 5% ` Philip Kaludercic
2023-05-11 1:37 9% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2023-05-13 17:18 0% ` Philip Kaludercic
1 sibling, 0 replies; 63+ results
From: Philip Kaludercic @ 2023-05-13 17:18 UTC (permalink / raw)
To: Joseph Turner; +Cc: 63336
ping?
Philip Kaludercic <philipk@posteo.net> writes:
> Joseph Turner <joseph@breatheoutbreathe.in> writes:
>
>>>> +(defun package-vc--make (pkg-spec dir)
>>>> + "Process :make and :shell-command spec arguments."
>>>> + (let ((target (plist-get pkg-spec :make))
>>>> + (cmd (plist-get pkg-spec :shell-command)))
>>>> + (when (or cmd target)
>>>> + (with-current-buffer (get-buffer-create " *package-vc make*")
>>> ^
>>> should the package name
>>> be mentioned here?
>>
>> I like this idea, but IIUC package-vc--make would then need to take an
>> extra arg, since pkg-spec doesn't contain the :name of the package. We
>> could also add :name to the pkg-spec plist?
>
> I wouldn't be in favour of that, I think that passing the name as a
> separate argument would be a better solution.
>
>> For comparison, package-vc--build-documentation creates a buffer called
>> " *package-vc doc*" without the package name.
>
> The difference I see here is that documentation usually builds fine,
> while :make or :shell-command have a higher chance of failing because
> some software is missing, especially if people don't use :make the way
> it is used on the ELPA server but to build external dependencies (I'm
> thinking of mail clients like notmuch)
>
>>>> + target (buffer-name)))))))
>>>> +
>>>> (declare-function org-export-to-file "ox" (backend file))
>>>>
>>>> (defun package-vc--build-documentation (pkg-desc file)
>>>> @@ -486,6 +515,9 @@ documentation and marking the package as installed."
>>>> ;; Generate package file
>>>> (package-vc--generate-description-file pkg-desc pkg-file)
>>>>
>>>> + ;; Process :make and :shell-command arguments before building documentation
>>>> + (when package-vc-process-make (package-vc--make pkg-spec pkg-dir))
>>>
>>> Wasn't the plan to allow `package-vc-process-make' to either be a
>>> generic "build-anything" or a selective listing of packages where we
>>> allow :make and :shell-command to be executed?
>>
>> Let me know if the attached commit accomplishes what you had in mind.
>
> Yes, that (or rather the newest version from a different message) looks good.
>
>>>> +
>>>> ;; Detect a manual
>>>> (when (executable-find "install-info")
>>>> (dolist (doc-file (ensure-list (plist-get pkg-spec :doc)))
>>>
>>> Otherwise this looks good, but I haven't tried it out yet.
>>
>> I fixed up a couple other issues:
>>
>> - removed unnecessary dir arg to package-vc--make
>> - added function arg to the docstring for package-vc--make
>>
>> I'm not sure if the customization type for package-vc-process-make is
>> correct. Please double check that.
>>
>> Also, should users be able to run :make and :shell-command args defined
>> in a spec passed into package-vc-install?
>
> Yes, is that currently not supported?
>
>> Best,
>>
>> Joseph
^ permalink raw reply [relevance 0%]
* bug#63337: [PATCH] package-vc--build-documentation: Fix relative @include statements
2023-05-13 16:38 10% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2023-05-13 17:14 5% ` Philip Kaludercic
2023-05-13 18:31 5% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
0 siblings, 1 reply; 63+ results
From: Philip Kaludercic @ 2023-05-13 17:14 UTC (permalink / raw)
To: Joseph Turner; +Cc: 63337
Joseph Turner <joseph@breatheoutbreathe.in> writes:
> Philip Kaludercic <philipk@posteo.net> writes:
>
>> Joseph Turner <joseph@breatheoutbreathe.in> writes:
>>
>>> Philip Kaludercic <philipk@posteo.net> writes:
>>>
>>>> Joseph Turner <joseph@breatheoutbreathe.in> writes:
>>>>
>>>>> Hello!
>>>>>
>>>>> Because package-vc--build-documentation exports the texinfo manual to a
>>>>> temp file inside /tmp/ , any @include statements with relative paths
>>>>> break the makeinfo call.
>>>>>
>>>>> I noticed this issue when attempting to use package-vc to install
>>>>> org-transclusion, whose manual contains the line
>>>>>
>>>>> #+texinfo: @include fdl.texi
>>>>>
>>>>> See: https://raw.githubusercontent.com/nobiot/org-transclusion/main/docs/org-transclusion-manual.org
>>>>>
>>>>> The attached patch solves this problem by passing the -I flag to
>>>>> makeinfo. From makeinfo --help:
>>>>>
>>>>> -I DIR append DIR to the @include search path.
>
>>>> According to the docs, makeinfo has -I to append the search path, and -P
>>>> to prepend. I don't know how well either of the two are supported, but
>>>> assuming they are, shouldn't -P be preferred? Or wouldn't it have any
>>>> effect?
>>>
>>> I am not sure what difference it would make. I don't know if the default
>>> @include search path includes anything besides the working directory.
>>>
>>> In the attached diff, I have changed -I to -P.
>>
>> I can confirm that this patch does the right thing, and I think we
>> should apply it.
>
> I think Eli suggested we prepend (-I) instead of append (-P), as in the
> very first patch I sent, also attached here.
I did not understand the argument, but it probably does not matter that
much. As this patch has Eli's blessing, I will apply it.
> From a41abce88ed3b833c5531208945474c9cd16284b Mon Sep 17 00:00:00 2001
> From: Joseph Turner <joseph@breatheoutbreathe.in>
> Date: Sat, 6 May 2023 14:49:43 -0700
> Subject: [PATCH] Fix: (package-vc--build-documentation) Relative @include
> statements
>
> ---
> lisp/emacs-lisp/package-vc.el | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
> index 489610e2a1e..63c10285ca7 100644
> --- a/lisp/emacs-lisp/package-vc.el
> +++ b/lisp/emacs-lisp/package-vc.el
> @@ -381,6 +381,7 @@ FILE can be an Org file, indicated by its \".org\" extension,
> otherwise it's assumed to be an Info file."
> (let* ((pkg-name (package-desc-name pkg-desc))
> (default-directory (package-desc-dir pkg-desc))
> + (docs-directory (expand-file-name (file-name-directory file)))
> (output (expand-file-name (format "%s.info" pkg-name)))
> clean-up)
> (when (string-match-p "\\.org\\'" file)
> @@ -395,7 +396,9 @@ otherwise it's assumed to be an Info file."
> (erase-buffer)
> (cond
> ((/= 0 (call-process "makeinfo" nil t nil
> - "--no-split" file "-o" output))
> + "-I" docs-directory
> + "--no-split" file
> + "-o" output))
> (message "Failed to build manual %s, see buffer %S"
> file (buffer-name)))
> ((/= 0 (call-process "install-info" nil t nil
^ permalink raw reply [relevance 5%]
* bug#63337: [PATCH] package-vc--build-documentation: Fix relative @include statements
2023-05-13 8:41 5% ` Philip Kaludercic
@ 2023-05-13 16:38 10% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-05-13 17:14 5% ` Philip Kaludercic
0 siblings, 1 reply; 63+ results
From: Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2023-05-13 16:38 UTC (permalink / raw)
To: Philip Kaludercic; +Cc: 63337
[-- Attachment #1: Type: text/plain, Size: 1543 bytes --]
Philip Kaludercic <philipk@posteo.net> writes:
> Joseph Turner <joseph@breatheoutbreathe.in> writes:
>
>> Philip Kaludercic <philipk@posteo.net> writes:
>>
>>> Joseph Turner <joseph@breatheoutbreathe.in> writes:
>>>
>>>> Hello!
>>>>
>>>> Because package-vc--build-documentation exports the texinfo manual to a
>>>> temp file inside /tmp/ , any @include statements with relative paths
>>>> break the makeinfo call.
>>>>
>>>> I noticed this issue when attempting to use package-vc to install
>>>> org-transclusion, whose manual contains the line
>>>>
>>>> #+texinfo: @include fdl.texi
>>>>
>>>> See: https://raw.githubusercontent.com/nobiot/org-transclusion/main/docs/org-transclusion-manual.org
>>>>
>>>> The attached patch solves this problem by passing the -I flag to
>>>> makeinfo. From makeinfo --help:
>>>>
>>>> -I DIR append DIR to the @include search path.
>>> According to the docs, makeinfo has -I to append the search path, and -P
>>> to prepend. I don't know how well either of the two are supported, but
>>> assuming they are, shouldn't -P be preferred? Or wouldn't it have any
>>> effect?
>>
>> I am not sure what difference it would make. I don't know if the default
>> @include search path includes anything besides the working directory.
>>
>> In the attached diff, I have changed -I to -P.
>
> I can confirm that this patch does the right thing, and I think we
> should apply it.
I think Eli suggested we prepend (-I) instead of append (-P), as in the
very first patch I sent, also attached here.
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-package-vc-build-documentation-Relative-include-.patch --]
[-- Type: text/x-diff, Size: 1478 bytes --]
From a41abce88ed3b833c5531208945474c9cd16284b Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Sat, 6 May 2023 14:49:43 -0700
Subject: [PATCH] Fix: (package-vc--build-documentation) Relative @include
statements
---
lisp/emacs-lisp/package-vc.el | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
index 489610e2a1e..63c10285ca7 100644
--- a/lisp/emacs-lisp/package-vc.el
+++ b/lisp/emacs-lisp/package-vc.el
@@ -381,6 +381,7 @@ FILE can be an Org file, indicated by its \".org\" extension,
otherwise it's assumed to be an Info file."
(let* ((pkg-name (package-desc-name pkg-desc))
(default-directory (package-desc-dir pkg-desc))
+ (docs-directory (expand-file-name (file-name-directory file)))
(output (expand-file-name (format "%s.info" pkg-name)))
clean-up)
(when (string-match-p "\\.org\\'" file)
@@ -395,7 +396,9 @@ otherwise it's assumed to be an Info file."
(erase-buffer)
(cond
((/= 0 (call-process "makeinfo" nil t nil
- "--no-split" file "-o" output))
+ "-I" docs-directory
+ "--no-split" file
+ "-o" output))
(message "Failed to build manual %s, see buffer %S"
file (buffer-name)))
((/= 0 (call-process "install-info" nil t nil
--
2.39.2
^ permalink raw reply related [relevance 10%]
* bug#63337: [PATCH] package-vc--build-documentation: Fix relative @include statements
2023-05-07 18:40 10% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-05-07 19:11 0% ` Eli Zaretskii
@ 2023-05-13 8:41 5% ` Philip Kaludercic
2023-05-13 16:38 10% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
1 sibling, 1 reply; 63+ results
From: Philip Kaludercic @ 2023-05-13 8:41 UTC (permalink / raw)
To: Joseph Turner; +Cc: 63337
Joseph Turner <joseph@breatheoutbreathe.in> writes:
> Philip Kaludercic <philipk@posteo.net> writes:
>
>> Joseph Turner <joseph@breatheoutbreathe.in> writes:
>>
>>> Hello!
>>>
>>> Because package-vc--build-documentation exports the texinfo manual to a
>>> temp file inside /tmp/ , any @include statements with relative paths
>>> break the makeinfo call.
>>>
>>> I noticed this issue when attempting to use package-vc to install
>>> org-transclusion, whose manual contains the line
>>>
>>> #+texinfo: @include fdl.texi
>>>
>>> See: https://raw.githubusercontent.com/nobiot/org-transclusion/main/docs/org-transclusion-manual.org
>>>
>>> The attached patch solves this problem by passing the -I flag to
>>> makeinfo. From makeinfo --help:
>>>
>>> -I DIR append DIR to the @include search path.
>>
>> Good catch, this should be applied to emacs-29.
>>
>>> Best,
>>>
>>> Joseph
>>>
>>> From a41abce88ed3b833c5531208945474c9cd16284b Mon Sep 17 00:00:00 2001
>>> From: Joseph Turner <joseph@breatheoutbreathe.in>
>>> Date: Sat, 6 May 2023 14:49:43 -0700
>>> Subject: [PATCH] Fix: (package-vc--build-documentation) Relative @include
>>> statements
>>>
>>> ---
>>> lisp/emacs-lisp/package-vc.el | 5 ++++-
>>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
>>> index 489610e2a1e..63c10285ca7 100644
>>> --- a/lisp/emacs-lisp/package-vc.el
>>> +++ b/lisp/emacs-lisp/package-vc.el
>>> @@ -381,6 +381,7 @@ FILE can be an Org file, indicated by its \".org\" extension,
>>> otherwise it's assumed to be an Info file."
>>> (let* ((pkg-name (package-desc-name pkg-desc))
>>> (default-directory (package-desc-dir pkg-desc))
>>> + (docs-directory (expand-file-name (file-name-directory file)))
>>> (output (expand-file-name (format "%s.info" pkg-name)))
>>> clean-up)
>>> (when (string-match-p "\\.org\\'" file)
>>> @@ -395,7 +396,9 @@ otherwise it's assumed to be an Info file."
>>> (erase-buffer)
>>> (cond
>>> ((/= 0 (call-process "makeinfo" nil t nil
>>> - "--no-split" file "-o" output))
>>> + "-I" docs-directory
>>
>> According to the docs, makeinfo has -I to append the search path, and -P
>> to prepend. I don't know how well either of the two are supported, but
>> assuming they are, shouldn't -P be preferred? Or wouldn't it have any
>> effect?
>
> I am not sure what difference it would make. I don't know if the default
> @include search path includes anything besides the working directory.
>
> In the attached diff, I have changed -I to -P.
I can confirm that this patch does the right thing, and I think we
should apply it.
>>> + "--no-split" file
>>> + "-o" output))
>>> (message "Failed to build manual %s, see buffer %S"
>>> file (buffer-name)))
>>> ((/= 0 (call-process "install-info" nil t nil
^ permalink raw reply [relevance 5%]
* bug#63337: [PATCH] package-vc--build-documentation: Fix relative @include statements
@ 2023-05-13 5:54 5% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
0 siblings, 0 replies; 63+ results
From: Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2023-05-13 5:54 UTC (permalink / raw)
To: Philip Kaludercic; +Cc: 63337, Eli Zaretskii
Philip Kaludercic <philipk@posteo.net> writes:
> Eli Zaretskii <eliz@gnu.org> writes:
>
>>> From: Philip Kaludercic <philipk@posteo.net>
>>> Cc: Eli Zaretskii <eliz@gnu.org>, 63337@debbugs.gnu.org
>>> Date: Fri, 12 May 2023 06:51:19 +0000
>>>
>>> One issue I still notice in this case is that if the file doesn't exist,
>>> the issue is raised by `insert-file-contents' which aborts the entire
>>> installation. Does checking the existence of the file, and continuing
>>> on if this is not the case make sense:
>>
>> If the file's (non)existence is the problem, then checking that up
>> front is an okay solution, IMO. Are there any downsides to doing that?
>
> One might not notice that there was an issue with the package
> specification, since a number of messages are usually generated when
> installing a package. But I agree that this sounds better than not
> being able to install the package at all (as is currently the case with
> org-transclusion).
This change makes sense to me also. Would it be appropriate to use warn
instead of message? I'm not sure the convention here.
This change belongs in a separate commit, right?
^ permalink raw reply [relevance 5%]
* bug#63337: [PATCH] package-vc--build-documentation: Fix relative @include statements
2023-05-12 6:56 5% ` Philip Kaludercic
@ 2023-05-13 5:47 5% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
0 siblings, 0 replies; 63+ results
From: Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2023-05-13 5:47 UTC (permalink / raw)
To: Philip Kaludercic; +Cc: 63337, Eli Zaretskii
Philip Kaludercic <philipk@posteo.net> writes:
> Oh, and a big problem is that the "dir" file is written into the wrong
> directory. It has to be located in the root directory of the package,
> not in docs/ (in the case of org-transclusion). Sadly adjusting the
> second argument doesn't fix the issue:
>
> diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
> index e9794eac783..9876705e57f 100644
> --- a/lisp/emacs-lisp/package-vc.el
> +++ b/lisp/emacs-lisp/package-vc.el
> @@ -369,7 +369,8 @@ package-vc--build-documentation
> (message "Failed to build manual %s, see buffer %S"
> file (buffer-name)))
> ((/= 0 (call-process "install-info" nil t nil
> - output (expand-file-name "dir")))
> + output
> + (expand-file-name "dir" (package-desc-dir pkg-desc))))
> (message "Failed to install manual %s, see buffer %S"
> output (buffer-name)))
> ((kill-buffer))))
>
>
> While the entry does appear in (dir) Top, the file cannot be opened:
>
> Info-find-file: Info file ‘org-transclusion’ does not exist; consider installing it
IIUC, you're saying that default-directory needs to be (package-desc-dir pkg-desc) when
install-info runs, right? Perhaps we should revisit the first patch I sent:
- leave default-directory as-is
- pass "-I docs-directory" to makeinfo
If makeinfo starts including more directory besides the working
directory in the future, I think it will still make sense to append the
docs-directory to the search path.
^ permalink raw reply [relevance 5%]
* bug#63337: [PATCH] package-vc--build-documentation: Fix relative @include statements
2023-05-11 2:04 5% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-05-12 6:51 5% ` Philip Kaludercic
@ 2023-05-12 6:56 5% ` Philip Kaludercic
2023-05-13 5:47 5% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
1 sibling, 1 reply; 63+ results
From: Philip Kaludercic @ 2023-05-12 6:56 UTC (permalink / raw)
To: Joseph Turner; +Cc: 63337, Eli Zaretskii
[-- Attachment #1: Type: text/plain, Size: 683 bytes --]
Joseph Turner <joseph@breatheoutbreathe.in> writes:
> Philip Kaludercic <philipk@posteo.net> writes:
>
>> Ok, do you have a few example repositories that we can use to test edge-cases?
>
> I first noticed this issue when attempting to build the docs for
> org-transclusion. Besides that, we could select a few package specs
> containing :make at random from
> https://git.savannah.gnu.org/cgit/emacs/elpa.git/tree/elpa-packages ?
Oh, and a big problem is that the "dir" file is written into the wrong
directory. It has to be located in the root directory of the package,
not in docs/ (in the case of org-transclusion). Sadly adjusting the
second argument doesn't fix the issue:
[-- Attachment #2: Type: text/plain, Size: 704 bytes --]
diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
index e9794eac783..9876705e57f 100644
--- a/lisp/emacs-lisp/package-vc.el
+++ b/lisp/emacs-lisp/package-vc.el
@@ -369,7 +369,8 @@ package-vc--build-documentation
(message "Failed to build manual %s, see buffer %S"
file (buffer-name)))
((/= 0 (call-process "install-info" nil t nil
- output (expand-file-name "dir")))
+ output
+ (expand-file-name "dir" (package-desc-dir pkg-desc))))
(message "Failed to install manual %s, see buffer %S"
output (buffer-name)))
((kill-buffer))))
[-- Attachment #3: Type: text/plain, Size: 177 bytes --]
While the entry does appear in (dir) Top, the file cannot be opened:
Info-find-file: Info file ‘org-transclusion’ does not exist; consider installing it
> Joseph
^ permalink raw reply related [relevance 5%]
* bug#63337: [PATCH] package-vc--build-documentation: Fix relative @include statements
2023-05-11 2:04 5% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2023-05-12 6:51 5% ` Philip Kaludercic
2023-05-12 6:56 5% ` Philip Kaludercic
1 sibling, 1 reply; 63+ results
From: Philip Kaludercic @ 2023-05-12 6:51 UTC (permalink / raw)
To: Joseph Turner; +Cc: 63337, Eli Zaretskii
[-- Attachment #1: Type: text/plain, Size: 686 bytes --]
Joseph Turner <joseph@breatheoutbreathe.in> writes:
> Philip Kaludercic <philipk@posteo.net> writes:
>
>> Ok, do you have a few example repositories that we can use to test edge-cases?
>
> I first noticed this issue when attempting to build the docs for
> org-transclusion. Besides that, we could select a few package specs
> containing :make at random from
> https://git.savannah.gnu.org/cgit/emacs/elpa.git/tree/elpa-packages ?
One issue I still notice in this case is that if the file doesn't exist,
the issue is raised by `insert-file-contents' which aborts the entire
installation. Does checking the existence of the file, and continuing
on if this is not the case make sense:
[-- Attachment #2: Type: text/plain, Size: 2453 bytes --]
diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
index e9794eac783..1c1492d87b2 100644
--- a/lisp/emacs-lisp/package-vc.el
+++ b/lisp/emacs-lisp/package-vc.el
@@ -353,28 +353,30 @@ package-vc--build-documentation
(default-directory (file-name-directory file-name))
(output (expand-file-name (format "%s.info" pkg-name)))
clean-up)
- (when (string-match-p "\\.org\\'" file)
- (require 'ox)
- (require 'ox-texinfo)
- (with-temp-buffer
- (insert-file-contents file-name)
- (setq file (make-temp-file "ox-texinfo-"))
- (org-export-to-file 'texinfo file)
- (setq clean-up t)))
- (with-current-buffer (get-buffer-create " *package-vc doc*")
- (erase-buffer)
- (cond
- ((/= 0 (call-process "makeinfo" nil t nil
- "--no-split" file "-o" output))
- (message "Failed to build manual %s, see buffer %S"
- file (buffer-name)))
- ((/= 0 (call-process "install-info" nil t nil
- output (expand-file-name "dir")))
- (message "Failed to install manual %s, see buffer %S"
- output (buffer-name)))
- ((kill-buffer))))
- (when clean-up
- (delete-file file))))
+ (if (not (file-exists-p file-name))
+ (message "Documentation file %S for %s not found" file pkg-name)
+ (when (string-match-p "\\.org\\'" file)
+ (require 'ox)
+ (require 'ox-texinfo)
+ (with-temp-buffer
+ (insert-file-contents file-name)
+ (setq file (make-temp-file "ox-texinfo-"))
+ (org-export-to-file 'texinfo file)
+ (setq clean-up t)))
+ (with-current-buffer (get-buffer-create " *package-vc doc*")
+ (erase-buffer)
+ (cond
+ ((/= 0 (call-process "makeinfo" nil t nil
+ "--no-split" file "-o" output))
+ (message "Failed to build manual %s, see buffer %S"
+ file (buffer-name)))
+ ((/= 0 (call-process "install-info" nil t nil
+ output (expand-file-name "dir")))
+ (message "Failed to install manual %s, see buffer %S"
+ output (buffer-name)))
+ ((kill-buffer))))
+ (when clean-up
+ (delete-file file)))))
(defun package-vc-install-dependencies (requirements)
"Install missing dependencies, and return missing ones.
[-- Attachment #3: Type: text/plain, Size: 11 bytes --]
> Joseph
^ permalink raw reply related [relevance 5%]
* bug#63337: [PATCH] package-vc--build-documentation: Fix relative @include statements
2023-05-10 6:51 5% ` Philip Kaludercic
@ 2023-05-11 2:04 5% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-05-12 6:51 5% ` Philip Kaludercic
2023-05-12 6:56 5% ` Philip Kaludercic
0 siblings, 2 replies; 63+ results
From: Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2023-05-11 2:04 UTC (permalink / raw)
To: Philip Kaludercic; +Cc: 63337, Eli Zaretskii
Philip Kaludercic <philipk@posteo.net> writes:
> Ok, do you have a few example repositories that we can use to test edge-cases?
I first noticed this issue when attempting to build the docs for
org-transclusion. Besides that, we could select a few package specs
containing :make at random from
https://git.savannah.gnu.org/cgit/emacs/elpa.git/tree/elpa-packages ?
Joseph
^ permalink raw reply [relevance 5%]
* bug#63336: [PATCH] package-vc: Process :make and :shell-command spec args
2023-05-10 6:35 5% ` Philip Kaludercic
@ 2023-05-11 1:37 9% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-05-14 7:44 5% ` Philip Kaludercic
2023-05-13 17:18 0% ` Philip Kaludercic
1 sibling, 1 reply; 63+ results
From: Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2023-05-11 1:37 UTC (permalink / raw)
To: Philip Kaludercic; +Cc: 63336
[-- Attachment #1: Type: text/plain, Size: 3378 bytes --]
Philip Kaludercic <philipk@posteo.net> writes:
> Joseph Turner <joseph@breatheoutbreathe.in> writes:
>
>>>> +(defun package-vc--make (pkg-spec dir)
>>>> + "Process :make and :shell-command spec arguments."
>>>> + (let ((target (plist-get pkg-spec :make))
>>>> + (cmd (plist-get pkg-spec :shell-command)))
>>>> + (when (or cmd target)
>>>> + (with-current-buffer (get-buffer-create " *package-vc make*")
>>> ^
>>> should the package name
>>> be mentioned here?
>>
>> I like this idea, but IIUC package-vc--make would then need to take an
>> extra arg, since pkg-spec doesn't contain the :name of the package. We
>> could also add :name to the pkg-spec plist?
>
> I wouldn't be in favour of that, I think that passing the name as a
> separate argument would be a better solution.
I agree.
>> For comparison, package-vc--build-documentation creates a buffer called
>> " *package-vc doc*" without the package name.
>
> The difference I see here is that documentation usually builds fine,
> while :make or :shell-command have a higher chance of failing because
> some software is missing, especially if people don't use :make the way
> it is used on the ELPA server but to build external dependencies (I'm
> thinking of mail clients like notmuch)
That makes sense to me. In the attached patch, I pass pkg-desc to
package-vc--make instead just name.
Want me to submit a separate patch which adds the package name to the
" *package-vc doc*" buffer name?
>>>> + target (buffer-name)))))))
>>>> +
>>>> (declare-function org-export-to-file "ox" (backend file))
>>>>
>>>> (defun package-vc--build-documentation (pkg-desc file)
>>>> @@ -486,6 +515,9 @@ documentation and marking the package as installed."
>>>> ;; Generate package file
>>>> (package-vc--generate-description-file pkg-desc pkg-file)
>>>>
>>>> + ;; Process :make and :shell-command arguments before building documentation
>>>> + (when package-vc-process-make (package-vc--make pkg-spec pkg-dir))
>>>
>>> Wasn't the plan to allow `package-vc-process-make' to either be a
>>> generic "build-anything" or a selective listing of packages where we
>>> allow :make and :shell-command to be executed?
>>
>> Let me know if the attached commit accomplishes what you had in mind.
>
> Yes, that (or rather the newest version from a different message) looks good.
>
>>>> +
>>>> ;; Detect a manual
>>>> (when (executable-find "install-info")
>>>> (dolist (doc-file (ensure-list (plist-get pkg-spec :doc)))
>>>
>>> Otherwise this looks good, but I haven't tried it out yet.
>>
>> I fixed up a couple other issues:
>>
>> - removed unnecessary dir arg to package-vc--make
>> - added function arg to the docstring for package-vc--make
>>
>> I'm not sure if the customization type for package-vc-process-make is
>> correct. Please double check that.
>>
>> Also, should users be able to run :make and :shell-command args defined
>> in a spec passed into package-vc-install?
>
> Yes, is that currently not supported?
Nevermind! It is supported. I didn't notice that package-vc--unpack adds
the user-defined pkg-spec to package-vc-selected-packages just before
calling package-vc--unpack-1.
Best,
Joseph
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-package-vc-Process-make-and-shell-command-spec-args.patch --]
[-- Type: text/x-diff, Size: 2941 bytes --]
From b27724197acd4ee72f9d336843f0e6ed9fcee87b Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Sat, 13 May 2023 10:05:04 -0700
Subject: [PATCH] package-vc: Process :make and :shell-command spec args
---
lisp/emacs-lisp/package-vc.el | 37 +++++++++++++++++++++++++++++++++++
1 file changed, 37 insertions(+)
diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
index beca0bd00e2..8529d1dad5c 100644
--- a/lisp/emacs-lisp/package-vc.el
+++ b/lisp/emacs-lisp/package-vc.el
@@ -344,6 +344,33 @@ asynchronously."
"\n")
nil pkg-file nil 'silent))))
+(defcustom package-vc-process-make nil
+ "Whether to process :make and :shell-command spec arguments.
+
+When set to a list of symbols (packages), run commands for only
+packages in the list. When `nil', never run commands. Otherwise
+when non-`nil', run commands for any package with :make or
+:shell-command specified.
+
+Package specs are loaded from trusted package archives."
+ :type '(choice (const :tag "Run for all packages" t)
+ (repeat :tag "Run only for selected packages" (symbol :tag "Package name"))
+ (const :tag "Never run" nil))
+ :version "30.1")
+
+(defun package-vc--make (pkg-spec pkg-desc)
+ "Process :make and :shell-command PKG-SPEC arguments for PKG-DESC."
+ (let ((target (plist-get pkg-spec :make))
+ (cmd (plist-get pkg-spec :shell-command)))
+ (when (or cmd target)
+ (with-current-buffer (get-buffer-create
+ (format " *package-vc make %s*" (package-desc-name pkg-desc)))
+ (erase-buffer)
+ (when (and cmd (/= 0 (call-process shell-file-name nil t nil shell-command-switch cmd)))
+ (warn "Failed to run %s, see buffer %S" cmd (buffer-name)))
+ (when (and target (/= 0 (apply #'call-process "make" nil t nil (if (consp target) target (list target)))))
+ (warn "Failed to make %s, see buffer %S" target (buffer-name)))))))
+
(declare-function org-export-to-file "ox" (backend file))
(defun package-vc--build-documentation (pkg-desc file)
@@ -486,6 +513,16 @@ documentation and marking the package as installed."
;; Generate package file
(package-vc--generate-description-file pkg-desc pkg-file)
+ ;; Process :make and :shell-command arguments before building documentation
+ (pcase package-vc-process-make
+ ((pred consp) ; When non-`nil' list, check if package is on the list.
+ (when (memq (package-desc-name pkg-desc) package-vc-process-make)
+ (package-vc--make pkg-spec pkg-desc)))
+ ('nil ; When `nil', do nothing.
+ nil)
+ (_ ; When otherwise non-`nil', run commands.
+ (package-vc--make pkg-spec pkg-desc)))
+
;; Detect a manual
(when (executable-find "install-info")
(dolist (doc-file (ensure-list (plist-get pkg-spec :doc)))
--
2.40.1
^ permalink raw reply related [relevance 9%]
* bug#63337: [PATCH] package-vc--build-documentation: Fix relative @include statements
2023-05-09 23:49 10% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2023-05-10 6:51 5% ` Philip Kaludercic
2023-05-11 2:04 5% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
0 siblings, 1 reply; 63+ results
From: Philip Kaludercic @ 2023-05-10 6:51 UTC (permalink / raw)
To: Joseph Turner; +Cc: 63337, Eli Zaretskii
Joseph Turner <joseph@breatheoutbreathe.in> writes:
> Eli Zaretskii <eliz@gnu.org> writes:
>
>>> From: Joseph Turner <joseph@breatheoutbreathe.in>
>>> Cc: Eli Zaretskii <eliz@gnu.org>, 63337@debbugs.gnu.org
>>> Date: Mon, 08 May 2023 12:05:51 -0700
>>>
>>> Note about the following two lines:
>>>
>>> + (file-path (expand-file-name file (package-desc-dir pkg-desc)))
>>> + (default-directory (expand-file-name (file-name-directory file-path)))
>>>
>>> (package-desc-dir pkg-desc) may return a relative path with or without a
>>> directory, e.g. "doc/manual.org" or "manual.org". In the latter case,
>>> (file-name-directory "manual.org") would return `nil' and
>>> (expand-file-name nil) would signal an error.
>>>
>>> Therefore, in the `file-path' `let'-binding, we first expand the return
>>> value of (package-desc-dir pkg-desc) to ensure that it contains a directory.
>>
>> Please don't use "path" for anything that is not a PATH-style list of
>> directory: the GNU Coding Standards frown on such usage. We use
>> file-name instead. For the same reasons, please don't give your
>> variables names that include "path" unless they are lists of
>> directories.
>
> Good to know, thank you! I changed `file-path' to `file-name'.
>
>>> --- a/lisp/emacs-lisp/package-vc.el
>>> +++ b/lisp/emacs-lisp/package-vc.el
>>> @@ -376,14 +376,17 @@ Package specs are loaded from trusted package archives."
>>> FILE can be an Org file, indicated by its \".org\" extension,
>>> otherwise it's assumed to be an Info file."
>>> (let* ((pkg-name (package-desc-name pkg-desc))
>>> - (default-directory (package-desc-dir pkg-desc))
>>> + (file-path (expand-file-name file (package-desc-dir pkg-desc)))
>>> + ;; `let'-bind `default-directory' to the directory containing the .org or .info FILE
>>> + ;; so that makeinfo can resolve relative @include statements in the docs directory.
>>> + (default-directory (expand-file-name (file-name-directory file-path)))
>>
>> There should be no reason to call expand-file-name in the last line,
>> since the argument of file-name-directory is already expanded.
>
> Good catch! Fixed.
>
>> Also, please make the comment lines shorter, preferably less than 75
>> columns.
>
> Done.
>
> Thank you!!
>
> Joseph
Ok, do you have a few example repositories that we can use to test edge-cases?
^ permalink raw reply [relevance 5%]
* bug#63336: [PATCH] package-vc: Process :make and :shell-command spec args
2023-05-07 18:47 8% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-05-08 8:42 0% ` Ruijie Yu via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2023-05-10 6:35 5% ` Philip Kaludercic
2023-05-11 1:37 9% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-05-13 17:18 0% ` Philip Kaludercic
1 sibling, 2 replies; 63+ results
From: Philip Kaludercic @ 2023-05-10 6:35 UTC (permalink / raw)
To: Joseph Turner; +Cc: 63336
Joseph Turner <joseph@breatheoutbreathe.in> writes:
>>> +(defun package-vc--make (pkg-spec dir)
>>> + "Process :make and :shell-command spec arguments."
>>> + (let ((target (plist-get pkg-spec :make))
>>> + (cmd (plist-get pkg-spec :shell-command)))
>>> + (when (or cmd target)
>>> + (with-current-buffer (get-buffer-create " *package-vc make*")
>> ^
>> should the package name
>> be mentioned here?
>
> I like this idea, but IIUC package-vc--make would then need to take an
> extra arg, since pkg-spec doesn't contain the :name of the package. We
> could also add :name to the pkg-spec plist?
I wouldn't be in favour of that, I think that passing the name as a
separate argument would be a better solution.
> For comparison, package-vc--build-documentation creates a buffer called
> " *package-vc doc*" without the package name.
The difference I see here is that documentation usually builds fine,
while :make or :shell-command have a higher chance of failing because
some software is missing, especially if people don't use :make the way
it is used on the ELPA server but to build external dependencies (I'm
thinking of mail clients like notmuch)
>>> + target (buffer-name)))))))
>>> +
>>> (declare-function org-export-to-file "ox" (backend file))
>>>
>>> (defun package-vc--build-documentation (pkg-desc file)
>>> @@ -486,6 +515,9 @@ documentation and marking the package as installed."
>>> ;; Generate package file
>>> (package-vc--generate-description-file pkg-desc pkg-file)
>>>
>>> + ;; Process :make and :shell-command arguments before building documentation
>>> + (when package-vc-process-make (package-vc--make pkg-spec pkg-dir))
>>
>> Wasn't the plan to allow `package-vc-process-make' to either be a
>> generic "build-anything" or a selective listing of packages where we
>> allow :make and :shell-command to be executed?
>
> Let me know if the attached commit accomplishes what you had in mind.
Yes, that (or rather the newest version from a different message) looks good.
>>> +
>>> ;; Detect a manual
>>> (when (executable-find "install-info")
>>> (dolist (doc-file (ensure-list (plist-get pkg-spec :doc)))
>>
>> Otherwise this looks good, but I haven't tried it out yet.
>
> I fixed up a couple other issues:
>
> - removed unnecessary dir arg to package-vc--make
> - added function arg to the docstring for package-vc--make
>
> I'm not sure if the customization type for package-vc-process-make is
> correct. Please double check that.
>
> Also, should users be able to run :make and :shell-command args defined
> in a spec passed into package-vc-install?
Yes, is that currently not supported?
> Best,
>
> Joseph
^ permalink raw reply [relevance 5%]
* bug#63337: [PATCH] package-vc--build-documentation: Fix relative @include statements
2023-05-09 4:36 0% ` Eli Zaretskii
@ 2023-05-09 23:49 10% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-05-10 6:51 5% ` Philip Kaludercic
0 siblings, 1 reply; 63+ results
From: Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2023-05-09 23:49 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: 63337, philipk
[-- Attachment #1: Type: text/plain, Size: 2167 bytes --]
Eli Zaretskii <eliz@gnu.org> writes:
>> From: Joseph Turner <joseph@breatheoutbreathe.in>
>> Cc: Eli Zaretskii <eliz@gnu.org>, 63337@debbugs.gnu.org
>> Date: Mon, 08 May 2023 12:05:51 -0700
>>
>> Note about the following two lines:
>>
>> + (file-path (expand-file-name file (package-desc-dir pkg-desc)))
>> + (default-directory (expand-file-name (file-name-directory file-path)))
>>
>> (package-desc-dir pkg-desc) may return a relative path with or without a
>> directory, e.g. "doc/manual.org" or "manual.org". In the latter case,
>> (file-name-directory "manual.org") would return `nil' and
>> (expand-file-name nil) would signal an error.
>>
>> Therefore, in the `file-path' `let'-binding, we first expand the return
>> value of (package-desc-dir pkg-desc) to ensure that it contains a directory.
>
> Please don't use "path" for anything that is not a PATH-style list of
> directory: the GNU Coding Standards frown on such usage. We use
> file-name instead. For the same reasons, please don't give your
> variables names that include "path" unless they are lists of
> directories.
Good to know, thank you! I changed `file-path' to `file-name'.
>> --- a/lisp/emacs-lisp/package-vc.el
>> +++ b/lisp/emacs-lisp/package-vc.el
>> @@ -376,14 +376,17 @@ Package specs are loaded from trusted package archives."
>> FILE can be an Org file, indicated by its \".org\" extension,
>> otherwise it's assumed to be an Info file."
>> (let* ((pkg-name (package-desc-name pkg-desc))
>> - (default-directory (package-desc-dir pkg-desc))
>> + (file-path (expand-file-name file (package-desc-dir pkg-desc)))
>> + ;; `let'-bind `default-directory' to the directory containing the .org or .info FILE
>> + ;; so that makeinfo can resolve relative @include statements in the docs directory.
>> + (default-directory (expand-file-name (file-name-directory file-path)))
>
> There should be no reason to call expand-file-name in the last line,
> since the argument of file-name-directory is already expanded.
Good catch! Fixed.
> Also, please make the comment lines shorter, preferably less than 75
> columns.
Done.
Thank you!!
Joseph
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-package-vc-build-documentation-Relative-include-.patch --]
[-- Type: text/x-diff, Size: 1583 bytes --]
From c670f47ef55b41265c064a2d4ab1e56c46e57272 Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Sat, 6 May 2023 14:49:43 -0700
Subject: [PATCH] Fix: (package-vc--build-documentation) Relative @include
statements
---
lisp/emacs-lisp/package-vc.el | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
index 95e12fc829a..efcfd635e98 100644
--- a/lisp/emacs-lisp/package-vc.el
+++ b/lisp/emacs-lisp/package-vc.el
@@ -377,14 +377,18 @@ Package specs are loaded from trusted package archives."
FILE can be an Org file, indicated by its \".org\" extension,
otherwise it's assumed to be an Info file."
(let* ((pkg-name (package-desc-name pkg-desc))
- (default-directory (package-desc-dir pkg-desc))
+ (file-name (expand-file-name file (package-desc-dir pkg-desc)))
+ ;; `let'-bind `default-directory' to the directory containing
+ ;; the .org or .info FILE so that makeinfo can resolve
+ ;; relative @include statements in the docs directory.
+ (default-directory (file-name-directory file-name))
(output (expand-file-name (format "%s.info" pkg-name)))
clean-up)
(when (string-match-p "\\.org\\'" file)
(require 'ox)
(require 'ox-texinfo)
(with-temp-buffer
- (insert-file-contents file)
+ (insert-file-contents file-name)
(setq file (make-temp-file "ox-texinfo-"))
(org-export-to-file 'texinfo file)
(setq clean-up t)))
--
2.39.2
^ permalink raw reply related [relevance 10%]
* bug#63337: [PATCH] package-vc--build-documentation: Fix relative @include statements
2023-05-08 19:05 9% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-05-09 1:34 5% ` Ruijie Yu via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2023-05-09 4:36 0% ` Eli Zaretskii
2023-05-09 23:49 10% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
1 sibling, 1 reply; 63+ results
From: Eli Zaretskii @ 2023-05-09 4:36 UTC (permalink / raw)
To: Joseph Turner; +Cc: 63337, philipk
> From: Joseph Turner <joseph@breatheoutbreathe.in>
> Cc: Eli Zaretskii <eliz@gnu.org>, 63337@debbugs.gnu.org
> Date: Mon, 08 May 2023 12:05:51 -0700
>
> Note about the following two lines:
>
> + (file-path (expand-file-name file (package-desc-dir pkg-desc)))
> + (default-directory (expand-file-name (file-name-directory file-path)))
>
> (package-desc-dir pkg-desc) may return a relative path with or without a
> directory, e.g. "doc/manual.org" or "manual.org". In the latter case,
> (file-name-directory "manual.org") would return `nil' and
> (expand-file-name nil) would signal an error.
>
> Therefore, in the `file-path' `let'-binding, we first expand the return
> value of (package-desc-dir pkg-desc) to ensure that it contains a directory.
Please don't use "path" for anything that is not a PATH-style list of
directory: the GNU Coding Standards frown on such usage. We use
file-name instead. For the same reasons, please don't give your
variables names that include "path" unless they are lists of
directories.
> --- a/lisp/emacs-lisp/package-vc.el
> +++ b/lisp/emacs-lisp/package-vc.el
> @@ -376,14 +376,17 @@ Package specs are loaded from trusted package archives."
> FILE can be an Org file, indicated by its \".org\" extension,
> otherwise it's assumed to be an Info file."
> (let* ((pkg-name (package-desc-name pkg-desc))
> - (default-directory (package-desc-dir pkg-desc))
> + (file-path (expand-file-name file (package-desc-dir pkg-desc)))
> + ;; `let'-bind `default-directory' to the directory containing the .org or .info FILE
> + ;; so that makeinfo can resolve relative @include statements in the docs directory.
> + (default-directory (expand-file-name (file-name-directory file-path)))
There should be no reason to call expand-file-name in the last line,
since the argument of file-name-directory is already expanded.
Also, please make the comment lines shorter, preferably less than 75
columns.
Thanks.
^ permalink raw reply [relevance 0%]
* bug#63337: [PATCH] package-vc--build-documentation: Fix relative @include statements
2023-05-09 1:34 5% ` Ruijie Yu via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2023-05-09 2:48 5% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
0 siblings, 0 replies; 63+ results
From: Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2023-05-09 2:48 UTC (permalink / raw)
To: Ruijie Yu; +Cc: 63337, philipk, eliz
Ruijie Yu <ruijie@netyu.xyz> writes:
> Joseph Turner via "Bug reports for GNU Emacs, the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org> writes:
>
>> + (file-path (expand-file-name file (package-desc-dir pkg-desc)))
>> + (default-directory (expand-file-name (file-name-directory file-path)))
>>
>> (package-desc-dir pkg-desc) may return a relative path with or without a
>> directory, e.g. "doc/manual.org" or "manual.org". In the latter case,
>> (file-name-directory "manual.org") would return `nil' and
>> (expand-file-name nil) would signal an error.
>
> In this case, can't you do this instead:
>
> (expand-file-name (or (file-name-directory ...) "."))
Yes, we could do this, but we make use of FILE-PATH anyway. We can't use
FILE after DEFAULT-DIRECTORY has been set the file containing FILE.
The choice is between
(file-name-directory file-path)
and
(or (file-name-directory file) ".")
I think the intent comes across more clearly in the former.
Joseph
^ permalink raw reply [relevance 5%]
* bug#63337: [PATCH] package-vc--build-documentation: Fix relative @include statements
2023-05-08 19:05 9% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2023-05-09 1:34 5% ` Ruijie Yu via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-05-09 2:48 5% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-05-09 4:36 0% ` Eli Zaretskii
1 sibling, 1 reply; 63+ results
From: Ruijie Yu via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2023-05-09 1:34 UTC (permalink / raw)
To: Joseph Turner; +Cc: 63337, philipk, eliz
Joseph Turner via "Bug reports for GNU Emacs, the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org> writes:
> + (file-path (expand-file-name file (package-desc-dir pkg-desc)))
> + (default-directory (expand-file-name (file-name-directory file-path)))
>
> (package-desc-dir pkg-desc) may return a relative path with or without a
> directory, e.g. "doc/manual.org" or "manual.org". In the latter case,
> (file-name-directory "manual.org") would return `nil' and
> (expand-file-name nil) would signal an error.
In this case, can't you do this instead:
(expand-file-name (or (file-name-directory ...) "."))
?
--
Best,
RY
^ permalink raw reply [relevance 5%]
* bug#63336: [PATCH] package-vc: Process :make and :shell-command spec args
2023-05-08 8:42 0% ` Ruijie Yu via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2023-05-08 19:38 9% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
0 siblings, 0 replies; 63+ results
From: Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2023-05-08 19:38 UTC (permalink / raw)
To: Ruijie Yu; +Cc: 63336, Philip Kaludercic
[-- Attachment #1: Type: text/plain, Size: 368 bytes --]
Ruijie Yu <ruijie@netyu.xyz> writes:
> Hello Joseph,
>
> On mobile so please excuse my brevity and top-posting.
>
> Minor remark on the defcustom type: I think you should move the current tag from "symbol" to its outer "repeat", and optionally tag "symbol" as something like "package name". WDYT?
Thanks, Ruijie! I think the defcustom type is now correct.
Joseph
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-package-vc-Process-make-and-shell-command-spec-args.patch --]
[-- Type: text/x-diff, Size: 2830 bytes --]
From 8f9238b841f6deec65fd52d696aa26caf1bf123a Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Sat, 6 May 2023 13:44:32 -0700
Subject: [PATCH] package-vc: Process :make and :shell-command spec args
---
lisp/emacs-lisp/package-vc.el | 36 +++++++++++++++++++++++++++++++++++
1 file changed, 36 insertions(+)
diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
index 421947b528d..95e12fc829a 100644
--- a/lisp/emacs-lisp/package-vc.el
+++ b/lisp/emacs-lisp/package-vc.el
@@ -344,6 +344,32 @@ asynchronously."
"\n")
nil pkg-file nil 'silent))))
+(defcustom package-vc-process-make nil
+ "Whether to process :make and :shell-command spec arguments.
+
+When set to a list of symbols (packages), run commands for only
+packages in the list. When `nil', never run commands. Otherwise
+when non-`nil', run commands for any package with :make or
+:shell-command specified.
+
+Package specs are loaded from trusted package archives."
+ :type '(choice (const :tag "Run for all packages" t)
+ (repeat :tag "Run only for selected packages" (symbol :tag "Package name"))
+ (const :tag "Never run" nil))
+ :version "30.1")
+
+(defun package-vc--make (pkg-spec)
+ "Process :make and :shell-command PKG-SPEC arguments."
+ (let ((target (plist-get pkg-spec :make))
+ (cmd (plist-get pkg-spec :shell-command)))
+ (when (or cmd target)
+ (with-current-buffer (get-buffer-create " *package-vc make*")
+ (erase-buffer)
+ (when (and cmd (/= 0 (call-process shell-file-name nil t nil shell-command-switch cmd)))
+ (warn "Failed to run %s, see buffer %S" cmd (buffer-name)))
+ (when (and target (/= 0 (apply #'call-process "make" nil t nil (if (consp target) target (list target)))))
+ (warn "Failed to make %s, see buffer %S" target (buffer-name)))))))
+
(declare-function org-export-to-file "ox" (backend file))
(defun package-vc--build-documentation (pkg-desc file)
@@ -486,6 +512,16 @@ documentation and marking the package as installed."
;; Generate package file
(package-vc--generate-description-file pkg-desc pkg-file)
+ ;; Process :make and :shell-command arguments before building documentation
+ (pcase package-vc-process-make
+ ((pred consp) ; When non-`nil' list, check if package is on the list.
+ (when (memq (package-desc-name pkg-desc) package-vc-process-make)
+ (package-vc--make pkg-spec)))
+ ('nil ; When `nil', do nothing.
+ nil)
+ (_ ; When otherwise non-`nil', run commands.
+ (package-vc--make pkg-spec)))
+
;; Detect a manual
(when (executable-find "install-info")
(dolist (doc-file (ensure-list (plist-get pkg-spec :doc)))
--
2.39.2
^ permalink raw reply related [relevance 9%]
* bug#63337: [PATCH] package-vc--build-documentation: Fix relative @include statements
2023-05-08 13:51 5% ` Philip Kaludercic
@ 2023-05-08 19:05 9% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-05-09 1:34 5% ` Ruijie Yu via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-05-09 4:36 0% ` Eli Zaretskii
0 siblings, 2 replies; 63+ results
From: Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2023-05-08 19:05 UTC (permalink / raw)
To: Philip Kaludercic; +Cc: 63337, Eli Zaretskii
[-- Attachment #1: Type: text/plain, Size: 1336 bytes --]
Philip Kaludercic <philipk@posteo.net> writes:
> It might be, but I'll have to look into that in more detail, that the
> first default-directory binding is not necessary if we pass
> (package-desc-dir pkg-desc) as the second argument to `expand-file-name'
> when binding `output'. Then this would all be simplified, and we could
> avoid the confusion you mention.
While this would mean binding default-directory only once, it still
requires two `let'-bindings.
The solution in the attached patch requires only one let-binding while
changing the behavior of package-vc--build-documentation slightly. Now
the output .info file is put inside the same directory as FILE, instead
of inside the directory returned by (package-desc-dir pkg-desc).
Note about the following two lines:
+ (file-path (expand-file-name file (package-desc-dir pkg-desc)))
+ (default-directory (expand-file-name (file-name-directory file-path)))
(package-desc-dir pkg-desc) may return a relative path with or without a
directory, e.g. "doc/manual.org" or "manual.org". In the latter case,
(file-name-directory "manual.org") would return `nil' and
(expand-file-name nil) would signal an error.
Therefore, in the `file-path' `let'-binding, we first expand the return
value of (package-desc-dir pkg-desc) to ensure that it contains a directory.
Best,
Joseph
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-package-vc-build-documentation-Relative-include-.patch --]
[-- Type: text/x-diff, Size: 1588 bytes --]
From 2cf2d522818c75ff5626324251bb74cdc3c36dc7 Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Sat, 6 May 2023 14:49:43 -0700
Subject: [PATCH] Fix: (package-vc--build-documentation) Relative @include
statements
---
lisp/emacs-lisp/package-vc.el | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
index 476c38916a8..65767cf043a 100644
--- a/lisp/emacs-lisp/package-vc.el
+++ b/lisp/emacs-lisp/package-vc.el
@@ -376,14 +376,17 @@ Package specs are loaded from trusted package archives."
FILE can be an Org file, indicated by its \".org\" extension,
otherwise it's assumed to be an Info file."
(let* ((pkg-name (package-desc-name pkg-desc))
- (default-directory (package-desc-dir pkg-desc))
+ (file-path (expand-file-name file (package-desc-dir pkg-desc)))
+ ;; `let'-bind `default-directory' to the directory containing the .org or .info FILE
+ ;; so that makeinfo can resolve relative @include statements in the docs directory.
+ (default-directory (expand-file-name (file-name-directory file-path)))
(output (expand-file-name (format "%s.info" pkg-name)))
clean-up)
(when (string-match-p "\\.org\\'" file)
(require 'ox)
(require 'ox-texinfo)
(with-temp-buffer
- (insert-file-contents file)
+ (insert-file-contents file-path)
(setq file (make-temp-file "ox-texinfo-"))
(org-export-to-file 'texinfo file)
(setq clean-up t)))
--
2.39.2
^ permalink raw reply related [relevance 9%]
* bug#63337: [PATCH] package-vc--build-documentation: Fix relative @include statements
2023-05-07 20:29 9% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2023-05-08 13:51 5% ` Philip Kaludercic
2023-05-08 19:05 9% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
0 siblings, 1 reply; 63+ results
From: Philip Kaludercic @ 2023-05-08 13:51 UTC (permalink / raw)
To: Joseph Turner; +Cc: 63337, Eli Zaretskii
Joseph Turner <joseph@breatheoutbreathe.in> writes:
> Philip Kaludercic <philipk@posteo.net> writes:
>
>> Eli Zaretskii <eliz@gnu.org> writes:
>>
>>>> Cc: 63337@debbugs.gnu.org
>>>> Date: Sun, 07 May 2023 11:40:46 -0700
>>>> From: Joseph Turner via "Bug reports for GNU Emacs,
>>>> the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
>>>>
>>>> > According to the docs, makeinfo has -I to append the search path, and -P
>>>> > to prepend. I don't know how well either of the two are supported, but
>>>> > assuming they are, shouldn't -P be preferred? Or wouldn't it have any
>>>> > effect?
>>>>
>>>> I am not sure what difference it would make. I don't know if the default
>>>> @include search path includes anything besides the working directory.
>>
>> I don't know that either, and I can imagine that certain versions of
>> makeinfo might be patched or this could change in the future.
>>
>>> It doesn't, according to the Texinfo manual. Only the current
>>> directory is searched.
>>>
>>>> In the attached diff, I have changed -I to -P.
>>>
>>> I think it's a mistake: the current directory should searched first.
>>> So -I is better.
>>
>> What do we mean by the current directory? When building the manual from
>> an org-file, we switch to a temporary directory (where the .org -> .texi
>> conversion is stored), so the "actual" directory is not the same as the
>> default-directory.
>
> AFAICT, makeinfo searches the default-directory. See attached patch,
> where we let-bind default-directory to the docs-directory. In this case,
> neither -I nor -P is necessary.
>
> It's a bit strange to let-bind default-directory twice in the same
> function, but we can't bind it at the top of the function, the
> insert-file-contents expects default-directory to be package-desc-dir.
It might be, but I'll have to look into that in more detail, that the
first default-directory binding is not necessary if we pass
(package-desc-dir pkg-desc) as the second argument to `expand-file-name'
when binding `output'. Then this would all be simplified, and we could
avoid the confusion you mention.
> Joseph
>
> From 7ddfd7ab08820eef159b21047194aaaf4c8841f7 Mon Sep 17 00:00:00 2001
> From: Joseph Turner <joseph@breatheoutbreathe.in>
> Date: Sat, 6 May 2023 14:49:43 -0700
> Subject: [PATCH] Fix: (package-vc--build-documentation) Relative @include
> statements
>
> ---
> lisp/emacs-lisp/package-vc.el | 23 +++++++++++++----------
> 1 file changed, 13 insertions(+), 10 deletions(-)
>
> diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
> index 476c38916a8..c25a96ed942 100644
> --- a/lisp/emacs-lisp/package-vc.el
> +++ b/lisp/emacs-lisp/package-vc.el
> @@ -377,6 +377,7 @@ FILE can be an Org file, indicated by its \".org\" extension,
> otherwise it's assumed to be an Info file."
> (let* ((pkg-name (package-desc-name pkg-desc))
> (default-directory (package-desc-dir pkg-desc))
> + (docs-directory (expand-file-name (file-name-directory file)))
> (output (expand-file-name (format "%s.info" pkg-name)))
> clean-up)
> (when (string-match-p "\\.org\\'" file)
> @@ -389,16 +390,18 @@ otherwise it's assumed to be an Info file."
> (setq clean-up t)))
> (with-current-buffer (get-buffer-create " *package-vc doc*")
> (erase-buffer)
> - (cond
> - ((/= 0 (call-process "makeinfo" nil t nil
> - "--no-split" file "-o" output))
> - (message "Failed to build manual %s, see buffer %S"
> - file (buffer-name)))
> - ((/= 0 (call-process "install-info" nil t nil
> - output (expand-file-name "dir")))
> - (message "Failed to install manual %s, see buffer %S"
> - output (buffer-name)))
> - ((kill-buffer))))
> + (let ((default-directory docs-directory))
> + ;; `let'-bind `default-directory' so that makeinfo resolves
> + ;; relative @include statements in the docs directory
> + (cond
> + ((/= 0 (call-process "makeinfo" nil t nil "--no-split" file "-o" output))
> + (message "Failed to build manual %s, see buffer %S"
> + file (buffer-name)))
> + ((/= 0 (call-process "install-info" nil t nil
> + output (expand-file-name "dir")))
> + (message "Failed to install manual %s, see buffer %S"
> + output (buffer-name)))
> + ((kill-buffer)))))
> (when clean-up
> (delete-file file))))
^ permalink raw reply [relevance 5%]
* bug#63336: [PATCH] package-vc: Process :make and :shell-command spec args
2023-05-07 18:47 8% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2023-05-08 8:42 0% ` Ruijie Yu via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-05-08 19:38 9% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-05-10 6:35 5% ` Philip Kaludercic
1 sibling, 1 reply; 63+ results
From: Ruijie Yu via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2023-05-08 8:42 UTC (permalink / raw)
To: Joseph Turner; +Cc: 63336, Philip Kaludercic
Hello Joseph,
On mobile so please excuse my brevity and top-posting.
Minor remark on the defcustom type: I think you should move the current tag from "symbol" to its outer "repeat", and optionally tag "symbol" as something like "package name". WDYT?
--
Best,
RY
> On May 8, 2023, at 04:29, Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors <bug-gnu-emacs@gnu.org> wrote:
>
> Thanks for the review!
>
> Philip Kaludercic <philipk@posteo.net> writes:
>
>> Joseph Turner <joseph@breatheoutbreathe.in> writes:
>>
>>> Hello!
>>>
>>> Here's a patch to support :make and :shell-command args as discussed:
>>>
>>> https://lists.gnu.org/archive/html/help-gnu-emacs/2023-04/msg00263.html
>>
>> Thanks!
>>
>>> Best,
>>>
>>> Joseph
>>>
>>> From c51161c51f11e6ffcba17758424596fe44f9d42a Mon Sep 17 00:00:00 2001
>>> From: Joseph Turner <joseph@breatheoutbreathe.in>
>>> Date: Sat, 6 May 2023 13:44:32 -0700
>>> Subject: [PATCH] package-vc: Process :make and :shell-command spec args
>>>
>>> ---
>>> lisp/emacs-lisp/package-vc.el | 32 ++++++++++++++++++++++++++++++++
>>> 1 file changed, 32 insertions(+)
>>>
>>> diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
>>> index 421947b528d..489610e2a1e 100644
>>> --- a/lisp/emacs-lisp/package-vc.el
>>> +++ b/lisp/emacs-lisp/package-vc.el
>>> @@ -344,6 +344,35 @@ asynchronously."
>>> "\n")
>>> nil pkg-file nil 'silent))))
>>>
>>> +(defcustom package-vc-process-make nil
>>> + "If non-nil, process :make and :shell-command spec arguments.
>>> +Package specs are loaded from trusted package archives."
>>> + :type 'boolean)
>>
>> As this patch is going to be added to Emacs 30, we should add
>>
>> :version "30.1"
>>
>> tags to this user option.
>
> Fixed.
>
>>> +(defun package-vc--call (destination program &rest args)
>>> + "Like ‘call-process’ for PROGRAM, DESTINATION, ARGS.
>> ^
>> You should replace these quotation marks with regular ASCII `marks', so
>> avoid byte-compiler warnings.
>
> Good catch.
>
>>> +The INFILE and DISPLAY arguments are fixed as nil."
>>> + (apply #'call-process program nil destination nil (delq nil args)))
>>
>> What is the motivation for this function? Is this where
>> process-isolation would be added in the future?
>
> In the attached patch, package-vc--call is replaced with call-process.
>
>>> +(defun package-vc--make (pkg-spec dir)
>>> + "Process :make and :shell-command spec arguments."
>>> + (let ((target (plist-get pkg-spec :make))
>>> + (cmd (plist-get pkg-spec :shell-command)))
>>> + (when (or cmd target)
>>> + (with-current-buffer (get-buffer-create " *package-vc make*")
>> ^
>> should the package name
>> be mentioned here?
>
> I like this idea, but IIUC package-vc--make would then need to take an
> extra arg, since pkg-spec doesn't contain the :name of the package. We
> could also add :name to the pkg-spec plist?
>
> For comparison, package-vc--build-documentation creates a buffer called
> " *package-vc doc*" without the package name.
>
>>> + (erase-buffer)
>>> + (when (and cmd
>>> + (/= 0 (package-vc--call t shell-file-name
>>> + shell-command-switch
>>> + cmd)))
>>> + (message "Failed to run %s, see buffer %S"
>>
>> Could `warn' be a better candidate here, instead of `message'?
>
> Done.
>
>>> + cmd (buffer-name)))
>>> + (when (and target
>>> + (/= 0 (apply #'package-vc--call t "make"
>>> + (if (consp target) target (list target)))))
>>> + (message "Failed to make %s, see buffer %S"
>
> And this message is changed to warn also.
>
>>> + target (buffer-name)))))))
>>> +
>>> (declare-function org-export-to-file "ox" (backend file))
>>>
>>> (defun package-vc--build-documentation (pkg-desc file)
>>> @@ -486,6 +515,9 @@ documentation and marking the package as installed."
>>> ;; Generate package file
>>> (package-vc--generate-description-file pkg-desc pkg-file)
>>>
>>> + ;; Process :make and :shell-command arguments before building documentation
>>> + (when package-vc-process-make (package-vc--make pkg-spec pkg-dir))
>>
>> Wasn't the plan to allow `package-vc-process-make' to either be a
>> generic "build-anything" or a selective listing of packages where we
>> allow :make and :shell-command to be executed?
>
> Let me know if the attached commit accomplishes what you had in mind.
>
>>> +
>>> ;; Detect a manual
>>> (when (executable-find "install-info")
>>> (dolist (doc-file (ensure-list (plist-get pkg-spec :doc)))
>>
>> Otherwise this looks good, but I haven't tried it out yet.
>
> I fixed up a couple other issues:
>
> - removed unnecessary dir arg to package-vc--make
> - added function arg to the docstring for package-vc--make
>
> I'm not sure if the customization type for package-vc-process-make is
> correct. Please double check that.
>
> Also, should users be able to run :make and :shell-command args defined
> in a spec passed into package-vc-install?
>
> Best,
>
> Joseph
>
> <0001-package-vc-Process-make-and-shell-command-spec-args.patch>
^ permalink raw reply [relevance 0%]
* bug#63337: [PATCH] package-vc--build-documentation: Fix relative @include statements
2023-05-07 19:19 0% ` Philip Kaludercic
@ 2023-05-07 20:29 9% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-05-08 13:51 5% ` Philip Kaludercic
0 siblings, 1 reply; 63+ results
From: Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2023-05-07 20:29 UTC (permalink / raw)
To: Philip Kaludercic; +Cc: 63337, Eli Zaretskii
[-- Attachment #1: Type: text/plain, Size: 1706 bytes --]
Philip Kaludercic <philipk@posteo.net> writes:
> Eli Zaretskii <eliz@gnu.org> writes:
>
>>> Cc: 63337@debbugs.gnu.org
>>> Date: Sun, 07 May 2023 11:40:46 -0700
>>> From: Joseph Turner via "Bug reports for GNU Emacs,
>>> the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
>>>
>>> > According to the docs, makeinfo has -I to append the search path, and -P
>>> > to prepend. I don't know how well either of the two are supported, but
>>> > assuming they are, shouldn't -P be preferred? Or wouldn't it have any
>>> > effect?
>>>
>>> I am not sure what difference it would make. I don't know if the default
>>> @include search path includes anything besides the working directory.
>
> I don't know that either, and I can imagine that certain versions of
> makeinfo might be patched or this could change in the future.
>
>> It doesn't, according to the Texinfo manual. Only the current
>> directory is searched.
>>
>>> In the attached diff, I have changed -I to -P.
>>
>> I think it's a mistake: the current directory should searched first.
>> So -I is better.
>
> What do we mean by the current directory? When building the manual from
> an org-file, we switch to a temporary directory (where the .org -> .texi
> conversion is stored), so the "actual" directory is not the same as the
> default-directory.
AFAICT, makeinfo searches the default-directory. See attached patch,
where we let-bind default-directory to the docs-directory. In this case,
neither -I nor -P is necessary.
It's a bit strange to let-bind default-directory twice in the same
function, but we can't bind it at the top of the function, the
insert-file-contents expects default-directory to be package-desc-dir.
Joseph
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-package-vc-build-documentation-Relative-include-.patch --]
[-- Type: text/x-diff, Size: 2344 bytes --]
From 7ddfd7ab08820eef159b21047194aaaf4c8841f7 Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Sat, 6 May 2023 14:49:43 -0700
Subject: [PATCH] Fix: (package-vc--build-documentation) Relative @include
statements
---
lisp/emacs-lisp/package-vc.el | 23 +++++++++++++----------
1 file changed, 13 insertions(+), 10 deletions(-)
diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
index 476c38916a8..c25a96ed942 100644
--- a/lisp/emacs-lisp/package-vc.el
+++ b/lisp/emacs-lisp/package-vc.el
@@ -377,6 +377,7 @@ FILE can be an Org file, indicated by its \".org\" extension,
otherwise it's assumed to be an Info file."
(let* ((pkg-name (package-desc-name pkg-desc))
(default-directory (package-desc-dir pkg-desc))
+ (docs-directory (expand-file-name (file-name-directory file)))
(output (expand-file-name (format "%s.info" pkg-name)))
clean-up)
(when (string-match-p "\\.org\\'" file)
@@ -389,16 +390,18 @@ otherwise it's assumed to be an Info file."
(setq clean-up t)))
(with-current-buffer (get-buffer-create " *package-vc doc*")
(erase-buffer)
- (cond
- ((/= 0 (call-process "makeinfo" nil t nil
- "--no-split" file "-o" output))
- (message "Failed to build manual %s, see buffer %S"
- file (buffer-name)))
- ((/= 0 (call-process "install-info" nil t nil
- output (expand-file-name "dir")))
- (message "Failed to install manual %s, see buffer %S"
- output (buffer-name)))
- ((kill-buffer))))
+ (let ((default-directory docs-directory))
+ ;; `let'-bind `default-directory' so that makeinfo resolves
+ ;; relative @include statements in the docs directory
+ (cond
+ ((/= 0 (call-process "makeinfo" nil t nil "--no-split" file "-o" output))
+ (message "Failed to build manual %s, see buffer %S"
+ file (buffer-name)))
+ ((/= 0 (call-process "install-info" nil t nil
+ output (expand-file-name "dir")))
+ (message "Failed to install manual %s, see buffer %S"
+ output (buffer-name)))
+ ((kill-buffer)))))
(when clean-up
(delete-file file))))
--
2.39.2
^ permalink raw reply related [relevance 9%]
* bug#63337: [PATCH] package-vc--build-documentation: Fix relative @include statements
2023-05-07 19:11 0% ` Eli Zaretskii
@ 2023-05-07 19:19 0% ` Philip Kaludercic
2023-05-07 20:29 9% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
0 siblings, 1 reply; 63+ results
From: Philip Kaludercic @ 2023-05-07 19:19 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: 63337, Joseph Turner
Eli Zaretskii <eliz@gnu.org> writes:
>> Cc: 63337@debbugs.gnu.org
>> Date: Sun, 07 May 2023 11:40:46 -0700
>> From: Joseph Turner via "Bug reports for GNU Emacs,
>> the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
>>
>> > According to the docs, makeinfo has -I to append the search path, and -P
>> > to prepend. I don't know how well either of the two are supported, but
>> > assuming they are, shouldn't -P be preferred? Or wouldn't it have any
>> > effect?
>>
>> I am not sure what difference it would make. I don't know if the default
>> @include search path includes anything besides the working directory.
I don't know that either, and I can imagine that certain versions of
makeinfo might be patched or this could change in the future.
> It doesn't, according to the Texinfo manual. Only the current
> directory is searched.
>
>> In the attached diff, I have changed -I to -P.
>
> I think it's a mistake: the current directory should searched first.
> So -I is better.
What do we mean by the current directory? When building the manual from
an org-file, we switch to a temporary directory (where the .org -> .texi
conversion is stored), so the "actual" directory is not the same as the
default-directory.
^ permalink raw reply [relevance 0%]
* bug#63337: [PATCH] package-vc--build-documentation: Fix relative @include statements
2023-05-07 18:40 10% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2023-05-07 19:11 0% ` Eli Zaretskii
2023-05-07 19:19 0% ` Philip Kaludercic
2023-05-13 8:41 5% ` Philip Kaludercic
1 sibling, 1 reply; 63+ results
From: Eli Zaretskii @ 2023-05-07 19:11 UTC (permalink / raw)
To: Joseph Turner; +Cc: 63337, philipk
> Cc: 63337@debbugs.gnu.org
> Date: Sun, 07 May 2023 11:40:46 -0700
> From: Joseph Turner via "Bug reports for GNU Emacs,
> the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
>
> > According to the docs, makeinfo has -I to append the search path, and -P
> > to prepend. I don't know how well either of the two are supported, but
> > assuming they are, shouldn't -P be preferred? Or wouldn't it have any
> > effect?
>
> I am not sure what difference it would make. I don't know if the default
> @include search path includes anything besides the working directory.
It doesn't, according to the Texinfo manual. Only the current
directory is searched.
> In the attached diff, I have changed -I to -P.
I think it's a mistake: the current directory should searched first.
So -I is better.
^ permalink raw reply [relevance 0%]
* bug#63336: [PATCH] package-vc: Process :make and :shell-command spec args
2023-05-07 9:03 5% ` Philip Kaludercic
@ 2023-05-07 18:47 8% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-05-08 8:42 0% ` Ruijie Yu via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-05-10 6:35 5% ` Philip Kaludercic
0 siblings, 2 replies; 63+ results
From: Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2023-05-07 18:47 UTC (permalink / raw)
To: Philip Kaludercic; +Cc: 63336
[-- Attachment #1: Type: text/plain, Size: 4915 bytes --]
Thanks for the review!
Philip Kaludercic <philipk@posteo.net> writes:
> Joseph Turner <joseph@breatheoutbreathe.in> writes:
>
>> Hello!
>>
>> Here's a patch to support :make and :shell-command args as discussed:
>>
>> https://lists.gnu.org/archive/html/help-gnu-emacs/2023-04/msg00263.html
>
> Thanks!
>
>> Best,
>>
>> Joseph
>>
>> From c51161c51f11e6ffcba17758424596fe44f9d42a Mon Sep 17 00:00:00 2001
>> From: Joseph Turner <joseph@breatheoutbreathe.in>
>> Date: Sat, 6 May 2023 13:44:32 -0700
>> Subject: [PATCH] package-vc: Process :make and :shell-command spec args
>>
>> ---
>> lisp/emacs-lisp/package-vc.el | 32 ++++++++++++++++++++++++++++++++
>> 1 file changed, 32 insertions(+)
>>
>> diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
>> index 421947b528d..489610e2a1e 100644
>> --- a/lisp/emacs-lisp/package-vc.el
>> +++ b/lisp/emacs-lisp/package-vc.el
>> @@ -344,6 +344,35 @@ asynchronously."
>> "\n")
>> nil pkg-file nil 'silent))))
>>
>> +(defcustom package-vc-process-make nil
>> + "If non-nil, process :make and :shell-command spec arguments.
>> +Package specs are loaded from trusted package archives."
>> + :type 'boolean)
>
> As this patch is going to be added to Emacs 30, we should add
>
> :version "30.1"
>
> tags to this user option.
Fixed.
>> +(defun package-vc--call (destination program &rest args)
>> + "Like ‘call-process’ for PROGRAM, DESTINATION, ARGS.
> ^
> You should replace these quotation marks with regular ASCII `marks', so
> avoid byte-compiler warnings.
Good catch.
>> +The INFILE and DISPLAY arguments are fixed as nil."
>> + (apply #'call-process program nil destination nil (delq nil args)))
>
> What is the motivation for this function? Is this where
> process-isolation would be added in the future?
In the attached patch, package-vc--call is replaced with call-process.
>> +(defun package-vc--make (pkg-spec dir)
>> + "Process :make and :shell-command spec arguments."
>> + (let ((target (plist-get pkg-spec :make))
>> + (cmd (plist-get pkg-spec :shell-command)))
>> + (when (or cmd target)
>> + (with-current-buffer (get-buffer-create " *package-vc make*")
> ^
> should the package name
> be mentioned here?
I like this idea, but IIUC package-vc--make would then need to take an
extra arg, since pkg-spec doesn't contain the :name of the package. We
could also add :name to the pkg-spec plist?
For comparison, package-vc--build-documentation creates a buffer called
" *package-vc doc*" without the package name.
>> + (erase-buffer)
>> + (when (and cmd
>> + (/= 0 (package-vc--call t shell-file-name
>> + shell-command-switch
>> + cmd)))
>> + (message "Failed to run %s, see buffer %S"
>
> Could `warn' be a better candidate here, instead of `message'?
Done.
>> + cmd (buffer-name)))
>> + (when (and target
>> + (/= 0 (apply #'package-vc--call t "make"
>> + (if (consp target) target (list target)))))
>> + (message "Failed to make %s, see buffer %S"
And this message is changed to warn also.
>> + target (buffer-name)))))))
>> +
>> (declare-function org-export-to-file "ox" (backend file))
>>
>> (defun package-vc--build-documentation (pkg-desc file)
>> @@ -486,6 +515,9 @@ documentation and marking the package as installed."
>> ;; Generate package file
>> (package-vc--generate-description-file pkg-desc pkg-file)
>>
>> + ;; Process :make and :shell-command arguments before building documentation
>> + (when package-vc-process-make (package-vc--make pkg-spec pkg-dir))
>
> Wasn't the plan to allow `package-vc-process-make' to either be a
> generic "build-anything" or a selective listing of packages where we
> allow :make and :shell-command to be executed?
Let me know if the attached commit accomplishes what you had in mind.
>> +
>> ;; Detect a manual
>> (when (executable-find "install-info")
>> (dolist (doc-file (ensure-list (plist-get pkg-spec :doc)))
>
> Otherwise this looks good, but I haven't tried it out yet.
I fixed up a couple other issues:
- removed unnecessary dir arg to package-vc--make
- added function arg to the docstring for package-vc--make
I'm not sure if the customization type for package-vc-process-make is
correct. Please double check that.
Also, should users be able to run :make and :shell-command args defined
in a spec passed into package-vc-install?
Best,
Joseph
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-package-vc-Process-make-and-shell-command-spec-args.patch --]
[-- Type: text/x-diff, Size: 2763 bytes --]
From f536b42492e1520cc8da4e92b5bb65552ba39e6a Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Sat, 6 May 2023 13:44:32 -0700
Subject: [PATCH] package-vc: Process :make and :shell-command spec args
---
lisp/emacs-lisp/package-vc.el | 35 +++++++++++++++++++++++++++++++++++
1 file changed, 35 insertions(+)
diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
index 421947b528d..476c38916a8 100644
--- a/lisp/emacs-lisp/package-vc.el
+++ b/lisp/emacs-lisp/package-vc.el
@@ -344,6 +344,31 @@ asynchronously."
"\n")
nil pkg-file nil 'silent))))
+(defcustom package-vc-process-make nil
+ "Whether to process :make and :shell-command spec arguments.
+
+When set to a list of symbols (packages), run commands for only
+packages in the list. When `nil', never run commands. Otherwise
+when non-`nil', run commands for any package with :make or
+:shell-command specified.
+
+Package specs are loaded from trusted package archives."
+ :type '(choice (boolean :tag "Run for all packages")
+ (repeat (symbol :tag "Run for only selected packages")))
+ :version "30.1")
+
+(defun package-vc--make (pkg-spec)
+ "Process :make and :shell-command PKG-SPEC arguments."
+ (let ((target (plist-get pkg-spec :make))
+ (cmd (plist-get pkg-spec :shell-command)))
+ (when (or cmd target)
+ (with-current-buffer (get-buffer-create " *package-vc make*")
+ (erase-buffer)
+ (when (and cmd (/= 0 (call-process shell-file-name nil t nil shell-command-switch cmd)))
+ (warn "Failed to run %s, see buffer %S" cmd (buffer-name)))
+ (when (and target (/= 0 (apply #'call-process "make" nil t nil (if (consp target) target (list target)))))
+ (warn "Failed to make %s, see buffer %S" target (buffer-name)))))))
+
(declare-function org-export-to-file "ox" (backend file))
(defun package-vc--build-documentation (pkg-desc file)
@@ -486,6 +511,16 @@ documentation and marking the package as installed."
;; Generate package file
(package-vc--generate-description-file pkg-desc pkg-file)
+ ;; Process :make and :shell-command arguments before building documentation
+ (pcase package-vc-process-make
+ ((pred consp) ; When non-`nil' list, check if package is on the list.
+ (when (memq (package-desc-name pkg-desc) package-vc-process-make)
+ (package-vc--make pkg-spec)))
+ ('nil ; When `nil', do nothing.
+ nil)
+ (_ ; When otherwise non-`nil', run commands.
+ (package-vc--make pkg-spec)))
+
;; Detect a manual
(when (executable-find "install-info")
(dolist (doc-file (ensure-list (plist-get pkg-spec :doc)))
--
2.39.2
^ permalink raw reply related [relevance 8%]
* bug#63337: [PATCH] package-vc--build-documentation: Fix relative @include statements
2023-05-07 9:58 5% ` Philip Kaludercic
2023-05-07 10:56 0% ` Eli Zaretskii
@ 2023-05-07 18:40 10% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-05-07 19:11 0% ` Eli Zaretskii
2023-05-13 8:41 5% ` Philip Kaludercic
1 sibling, 2 replies; 63+ results
From: Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2023-05-07 18:40 UTC (permalink / raw)
To: Philip Kaludercic; +Cc: 63337
[-- Attachment #1: Type: text/plain, Size: 2811 bytes --]
Philip Kaludercic <philipk@posteo.net> writes:
> Joseph Turner <joseph@breatheoutbreathe.in> writes:
>
>> Hello!
>>
>> Because package-vc--build-documentation exports the texinfo manual to a
>> temp file inside /tmp/ , any @include statements with relative paths
>> break the makeinfo call.
>>
>> I noticed this issue when attempting to use package-vc to install
>> org-transclusion, whose manual contains the line
>>
>> #+texinfo: @include fdl.texi
>>
>> See: https://raw.githubusercontent.com/nobiot/org-transclusion/main/docs/org-transclusion-manual.org
>>
>> The attached patch solves this problem by passing the -I flag to
>> makeinfo. From makeinfo --help:
>>
>> -I DIR append DIR to the @include search path.
>
> Good catch, this should be applied to emacs-29.
>
>> Best,
>>
>> Joseph
>>
>> From a41abce88ed3b833c5531208945474c9cd16284b Mon Sep 17 00:00:00 2001
>> From: Joseph Turner <joseph@breatheoutbreathe.in>
>> Date: Sat, 6 May 2023 14:49:43 -0700
>> Subject: [PATCH] Fix: (package-vc--build-documentation) Relative @include
>> statements
>>
>> ---
>> lisp/emacs-lisp/package-vc.el | 5 ++++-
>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
>> index 489610e2a1e..63c10285ca7 100644
>> --- a/lisp/emacs-lisp/package-vc.el
>> +++ b/lisp/emacs-lisp/package-vc.el
>> @@ -381,6 +381,7 @@ FILE can be an Org file, indicated by its \".org\" extension,
>> otherwise it's assumed to be an Info file."
>> (let* ((pkg-name (package-desc-name pkg-desc))
>> (default-directory (package-desc-dir pkg-desc))
>> + (docs-directory (expand-file-name (file-name-directory file)))
>> (output (expand-file-name (format "%s.info" pkg-name)))
>> clean-up)
>> (when (string-match-p "\\.org\\'" file)
>> @@ -395,7 +396,9 @@ otherwise it's assumed to be an Info file."
>> (erase-buffer)
>> (cond
>> ((/= 0 (call-process "makeinfo" nil t nil
>> - "--no-split" file "-o" output))
>> + "-I" docs-directory
>
> According to the docs, makeinfo has -I to append the search path, and -P
> to prepend. I don't know how well either of the two are supported, but
> assuming they are, shouldn't -P be preferred? Or wouldn't it have any
> effect?
I am not sure what difference it would make. I don't know if the default
@include search path includes anything besides the working directory.
In the attached diff, I have changed -I to -P.
>> + "--no-split" file
>> + "-o" output))
>> (message "Failed to build manual %s, see buffer %S"
>> file (buffer-name)))
>> ((/= 0 (call-process "install-info" nil t nil
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-package-vc-build-documentation-Relative-include-.patch --]
[-- Type: text/x-diff, Size: 1478 bytes --]
From a41abce88ed3b833c5531208945474c9cd16284b Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Sat, 6 May 2023 14:49:43 -0700
Subject: [PATCH] Fix: (package-vc--build-documentation) Relative @include
statements
---
lisp/emacs-lisp/package-vc.el | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
index 489610e2a1e..63c10285ca7 100644
--- a/lisp/emacs-lisp/package-vc.el
+++ b/lisp/emacs-lisp/package-vc.el
@@ -381,6 +381,7 @@ FILE can be an Org file, indicated by its \".org\" extension,
otherwise it's assumed to be an Info file."
(let* ((pkg-name (package-desc-name pkg-desc))
(default-directory (package-desc-dir pkg-desc))
+ (docs-directory (expand-file-name (file-name-directory file)))
(output (expand-file-name (format "%s.info" pkg-name)))
clean-up)
(when (string-match-p "\\.org\\'" file)
@@ -395,7 +396,9 @@ otherwise it's assumed to be an Info file."
(erase-buffer)
(cond
((/= 0 (call-process "makeinfo" nil t nil
- "--no-split" file "-o" output))
+ "-P" docs-directory
+ "--no-split" file
+ "-o" output))
(message "Failed to build manual %s, see buffer %S"
file (buffer-name)))
((/= 0 (call-process "install-info" nil t nil
--
2.39.2
^ permalink raw reply related [relevance 10%]
* bug#63337: [PATCH] package-vc--build-documentation: Fix relative @include statements
2023-05-07 9:58 5% ` Philip Kaludercic
@ 2023-05-07 10:56 0% ` Eli Zaretskii
2023-05-07 18:40 10% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
1 sibling, 0 replies; 63+ results
From: Eli Zaretskii @ 2023-05-07 10:56 UTC (permalink / raw)
To: Philip Kaludercic; +Cc: 63337, joseph
> Cc: 63337@debbugs.gnu.org
> From: Philip Kaludercic <philipk@posteo.net>
> Date: Sun, 07 May 2023 09:58:19 +0000
>
> Joseph Turner <joseph@breatheoutbreathe.in> writes:
>
> > Hello!
> >
> > Because package-vc--build-documentation exports the texinfo manual to a
> > temp file inside /tmp/ , any @include statements with relative paths
> > break the makeinfo call.
> >
> > I noticed this issue when attempting to use package-vc to install
> > org-transclusion, whose manual contains the line
> >
> > #+texinfo: @include fdl.texi
> >
> > See: https://raw.githubusercontent.com/nobiot/org-transclusion/main/docs/org-transclusion-manual.org
> >
> > The attached patch solves this problem by passing the -I flag to
> > makeinfo. From makeinfo --help:
> >
> > -I DIR append DIR to the @include search path.
>
> Good catch, this should be applied to emacs-29.
Fine by me.
^ permalink raw reply [relevance 0%]
* bug#63337: [PATCH] package-vc--build-documentation: Fix relative @include statements
2023-05-06 20:54 10% bug#63337: [PATCH] package-vc--build-documentation: Fix relative @include statements Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2023-05-07 9:58 5% ` Philip Kaludercic
2023-05-07 10:56 0% ` Eli Zaretskii
2023-05-07 18:40 10% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
0 siblings, 2 replies; 63+ results
From: Philip Kaludercic @ 2023-05-07 9:58 UTC (permalink / raw)
To: Joseph Turner; +Cc: 63337
Joseph Turner <joseph@breatheoutbreathe.in> writes:
> Hello!
>
> Because package-vc--build-documentation exports the texinfo manual to a
> temp file inside /tmp/ , any @include statements with relative paths
> break the makeinfo call.
>
> I noticed this issue when attempting to use package-vc to install
> org-transclusion, whose manual contains the line
>
> #+texinfo: @include fdl.texi
>
> See: https://raw.githubusercontent.com/nobiot/org-transclusion/main/docs/org-transclusion-manual.org
>
> The attached patch solves this problem by passing the -I flag to
> makeinfo. From makeinfo --help:
>
> -I DIR append DIR to the @include search path.
Good catch, this should be applied to emacs-29.
> Best,
>
> Joseph
>
> From a41abce88ed3b833c5531208945474c9cd16284b Mon Sep 17 00:00:00 2001
> From: Joseph Turner <joseph@breatheoutbreathe.in>
> Date: Sat, 6 May 2023 14:49:43 -0700
> Subject: [PATCH] Fix: (package-vc--build-documentation) Relative @include
> statements
>
> ---
> lisp/emacs-lisp/package-vc.el | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
> index 489610e2a1e..63c10285ca7 100644
> --- a/lisp/emacs-lisp/package-vc.el
> +++ b/lisp/emacs-lisp/package-vc.el
> @@ -381,6 +381,7 @@ FILE can be an Org file, indicated by its \".org\" extension,
> otherwise it's assumed to be an Info file."
> (let* ((pkg-name (package-desc-name pkg-desc))
> (default-directory (package-desc-dir pkg-desc))
> + (docs-directory (expand-file-name (file-name-directory file)))
> (output (expand-file-name (format "%s.info" pkg-name)))
> clean-up)
> (when (string-match-p "\\.org\\'" file)
> @@ -395,7 +396,9 @@ otherwise it's assumed to be an Info file."
> (erase-buffer)
> (cond
> ((/= 0 (call-process "makeinfo" nil t nil
> - "--no-split" file "-o" output))
> + "-I" docs-directory
According to the docs, makeinfo has -I to append the search path, and -P
to prepend. I don't know how well either of the two are supported, but
assuming they are, shouldn't -P be preferred? Or wouldn't it have any
effect?
> + "--no-split" file
> + "-o" output))
> (message "Failed to build manual %s, see buffer %S"
> file (buffer-name)))
> ((/= 0 (call-process "install-info" nil t nil
--
Philip Kaludercic
^ permalink raw reply [relevance 5%]
* bug#63336: [PATCH] package-vc: Process :make and :shell-command spec args
2023-05-06 20:39 9% bug#63336: [PATCH] package-vc: Process :make and :shell-command spec args Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2023-05-07 9:03 5% ` Philip Kaludercic
2023-05-07 18:47 8% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
0 siblings, 1 reply; 63+ results
From: Philip Kaludercic @ 2023-05-07 9:03 UTC (permalink / raw)
To: Joseph Turner; +Cc: 63336
Joseph Turner <joseph@breatheoutbreathe.in> writes:
> Hello!
>
> Here's a patch to support :make and :shell-command args as discussed:
>
> https://lists.gnu.org/archive/html/help-gnu-emacs/2023-04/msg00263.html
Thanks!
> Best,
>
> Joseph
>
> From c51161c51f11e6ffcba17758424596fe44f9d42a Mon Sep 17 00:00:00 2001
> From: Joseph Turner <joseph@breatheoutbreathe.in>
> Date: Sat, 6 May 2023 13:44:32 -0700
> Subject: [PATCH] package-vc: Process :make and :shell-command spec args
>
> ---
> lisp/emacs-lisp/package-vc.el | 32 ++++++++++++++++++++++++++++++++
> 1 file changed, 32 insertions(+)
>
> diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
> index 421947b528d..489610e2a1e 100644
> --- a/lisp/emacs-lisp/package-vc.el
> +++ b/lisp/emacs-lisp/package-vc.el
> @@ -344,6 +344,35 @@ asynchronously."
> "\n")
> nil pkg-file nil 'silent))))
>
> +(defcustom package-vc-process-make nil
> + "If non-nil, process :make and :shell-command spec arguments.
> +Package specs are loaded from trusted package archives."
> + :type 'boolean)
As this patch is going to be added to Emacs 30, we should add
:version "30.1"
tags to this user option.
> +(defun package-vc--call (destination program &rest args)
> + "Like ‘call-process’ for PROGRAM, DESTINATION, ARGS.
^
You should replace these quotation marks with regular ASCII `marks', so
avoid byte-compiler warnings.
> +The INFILE and DISPLAY arguments are fixed as nil."
> + (apply #'call-process program nil destination nil (delq nil args)))
What is the motivation for this function? Is this where
process-isolation would be added in the future?
> +(defun package-vc--make (pkg-spec dir)
> + "Process :make and :shell-command spec arguments."
> + (let ((target (plist-get pkg-spec :make))
> + (cmd (plist-get pkg-spec :shell-command)))
> + (when (or cmd target)
> + (with-current-buffer (get-buffer-create " *package-vc make*")
^
should the package name
be mentioned here?
> + (erase-buffer)
> + (when (and cmd
> + (/= 0 (package-vc--call t shell-file-name
> + shell-command-switch
> + cmd)))
> + (message "Failed to run %s, see buffer %S"
Could `warn' be a better candidate here, instead of `message'?
> + cmd (buffer-name)))
> + (when (and target
> + (/= 0 (apply #'package-vc--call t "make"
> + (if (consp target) target (list target)))))
> + (message "Failed to make %s, see buffer %S"
> + target (buffer-name)))))))
> +
> (declare-function org-export-to-file "ox" (backend file))
>
> (defun package-vc--build-documentation (pkg-desc file)
> @@ -486,6 +515,9 @@ documentation and marking the package as installed."
> ;; Generate package file
> (package-vc--generate-description-file pkg-desc pkg-file)
>
> + ;; Process :make and :shell-command arguments before building documentation
> + (when package-vc-process-make (package-vc--make pkg-spec pkg-dir))
Wasn't the plan to allow `package-vc-process-make' to either be a
generic "build-anything" or a selective listing of packages where we
allow :make and :shell-command to be executed?
> +
> ;; Detect a manual
> (when (executable-find "install-info")
> (dolist (doc-file (ensure-list (plist-get pkg-spec :doc)))
Otherwise this looks good, but I haven't tried it out yet.
--
Philip Kaludercic
^ permalink raw reply [relevance 5%]
* bug#63337: [PATCH] package-vc--build-documentation: Fix relative @include statements
@ 2023-05-06 20:54 10% Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-05-07 9:58 5% ` Philip Kaludercic
0 siblings, 1 reply; 63+ results
From: Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2023-05-06 20:54 UTC (permalink / raw)
To: 63337; +Cc: Philip Kaludercic
[-- Attachment #1: Type: text/plain, Size: 607 bytes --]
Hello!
Because package-vc--build-documentation exports the texinfo manual to a
temp file inside /tmp/ , any @include statements with relative paths
break the makeinfo call.
I noticed this issue when attempting to use package-vc to install
org-transclusion, whose manual contains the line
#+texinfo: @include fdl.texi
See: https://raw.githubusercontent.com/nobiot/org-transclusion/main/docs/org-transclusion-manual.org
The attached patch solves this problem by passing the -I flag to
makeinfo. From makeinfo --help:
-I DIR append DIR to the @include search path.
Best,
Joseph
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-package-vc-build-documentation-Relative-include-.patch --]
[-- Type: text/x-diff, Size: 1478 bytes --]
From a41abce88ed3b833c5531208945474c9cd16284b Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Sat, 6 May 2023 14:49:43 -0700
Subject: [PATCH] Fix: (package-vc--build-documentation) Relative @include
statements
---
lisp/emacs-lisp/package-vc.el | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
index 489610e2a1e..63c10285ca7 100644
--- a/lisp/emacs-lisp/package-vc.el
+++ b/lisp/emacs-lisp/package-vc.el
@@ -381,6 +381,7 @@ FILE can be an Org file, indicated by its \".org\" extension,
otherwise it's assumed to be an Info file."
(let* ((pkg-name (package-desc-name pkg-desc))
(default-directory (package-desc-dir pkg-desc))
+ (docs-directory (expand-file-name (file-name-directory file)))
(output (expand-file-name (format "%s.info" pkg-name)))
clean-up)
(when (string-match-p "\\.org\\'" file)
@@ -395,7 +396,9 @@ otherwise it's assumed to be an Info file."
(erase-buffer)
(cond
((/= 0 (call-process "makeinfo" nil t nil
- "--no-split" file "-o" output))
+ "-I" docs-directory
+ "--no-split" file
+ "-o" output))
(message "Failed to build manual %s, see buffer %S"
file (buffer-name)))
((/= 0 (call-process "install-info" nil t nil
--
2.39.2
^ permalink raw reply related [relevance 10%]
* bug#63336: [PATCH] package-vc: Process :make and :shell-command spec args
@ 2023-05-06 20:39 9% Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-05-07 9:03 5% ` Philip Kaludercic
0 siblings, 1 reply; 63+ results
From: Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2023-05-06 20:39 UTC (permalink / raw)
To: 63336; +Cc: Philip Kaludercic
[-- Attachment #1: Type: text/plain, Size: 167 bytes --]
Hello!
Here's a patch to support :make and :shell-command args as discussed:
https://lists.gnu.org/archive/html/help-gnu-emacs/2023-04/msg00263.html
Best,
Joseph
[-- Attachment #2: 0001-package-vc-Process-make-and-shell-command-spec-args.patch --]
[-- Type: text/x-diff, Size: 2582 bytes --]
From c51161c51f11e6ffcba17758424596fe44f9d42a Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Sat, 6 May 2023 13:44:32 -0700
Subject: [PATCH] package-vc: Process :make and :shell-command spec args
---
lisp/emacs-lisp/package-vc.el | 32 ++++++++++++++++++++++++++++++++
1 file changed, 32 insertions(+)
diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
index 421947b528d..489610e2a1e 100644
--- a/lisp/emacs-lisp/package-vc.el
+++ b/lisp/emacs-lisp/package-vc.el
@@ -344,6 +344,35 @@ asynchronously."
"\n")
nil pkg-file nil 'silent))))
+(defcustom package-vc-process-make nil
+ "If non-nil, process :make and :shell-command spec arguments.
+Package specs are loaded from trusted package archives."
+ :type 'boolean)
+
+(defun package-vc--call (destination program &rest args)
+ "Like ‘call-process’ for PROGRAM, DESTINATION, ARGS.
+The INFILE and DISPLAY arguments are fixed as nil."
+ (apply #'call-process program nil destination nil (delq nil args)))
+
+(defun package-vc--make (pkg-spec dir)
+ "Process :make and :shell-command spec arguments."
+ (let ((target (plist-get pkg-spec :make))
+ (cmd (plist-get pkg-spec :shell-command)))
+ (when (or cmd target)
+ (with-current-buffer (get-buffer-create " *package-vc make*")
+ (erase-buffer)
+ (when (and cmd
+ (/= 0 (package-vc--call t shell-file-name
+ shell-command-switch
+ cmd)))
+ (message "Failed to run %s, see buffer %S"
+ cmd (buffer-name)))
+ (when (and target
+ (/= 0 (apply #'package-vc--call t "make"
+ (if (consp target) target (list target)))))
+ (message "Failed to make %s, see buffer %S"
+ target (buffer-name)))))))
+
(declare-function org-export-to-file "ox" (backend file))
(defun package-vc--build-documentation (pkg-desc file)
@@ -486,6 +515,9 @@ documentation and marking the package as installed."
;; Generate package file
(package-vc--generate-description-file pkg-desc pkg-file)
+ ;; Process :make and :shell-command arguments before building documentation
+ (when package-vc-process-make (package-vc--make pkg-spec pkg-dir))
+
;; Detect a manual
(when (executable-find "install-info")
(dolist (doc-file (ensure-list (plist-get pkg-spec :doc)))
--
2.39.2
^ permalink raw reply related [relevance 9%]
* bug#60643: 29.0.50; set-buffer-major-mode resets buffer local variables
2023-01-08 6:00 0% ` Eli Zaretskii
2023-01-08 9:09 5% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2023-01-08 9:09 5% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
1 sibling, 0 replies; 63+ results
From: Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2023-01-08 9:09 UTC (permalink / raw)
To: Eli Zaretskii, ruijie; +Cc: 60643
Hello!
Eli Zaretskii <eliz@gnu.org> writes:
> Changing the major mode kills all buffer-local variables. If you don't
> want some variable to be killed, give it the permanent-local property.
Ruijie Yu <ruijie@netyu.xyz> writes:
> There is another concept called "permanent local variables" which you
> might be thinking about when looking at "local variables", see info node
> `(elisp) Standard Properties' for more information.
Thank you both for your help!! This solves it:
(put 'test-var 'permanent-local t)
Joseph
^ permalink raw reply [relevance 5%]
* bug#60643: 29.0.50; set-buffer-major-mode resets buffer local variables
2023-01-08 6:00 0% ` Eli Zaretskii
@ 2023-01-08 9:09 5% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-01-08 9:09 5% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
1 sibling, 0 replies; 63+ results
From: Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2023-01-08 9:09 UTC (permalink / raw)
To: Eli Zaretskii, ruijie; +Cc: 60643-done
Hello!
Eli Zaretskii <eliz@gnu.org> writes:
> Changing the major mode kills all buffer-local variables. If you don't
> want some variable to be killed, give it the permanent-local property.
Ruijie Yu <ruijie@netyu.xyz> writes:
> There is another concept called "permanent local variables" which you
> might be thinking about when looking at "local variables", see info node
> `(elisp) Standard Properties' for more information.
Thank you both for your help!! This solves it:
(put 'test-var 'permanent-local t)
Joseph
^ permalink raw reply [relevance 5%]
* bug#60643: 29.0.50; set-buffer-major-mode resets buffer local variables
2023-01-08 4:48 5% bug#60643: 29.0.50; set-buffer-major-mode resets buffer local variables Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-01-08 5:45 5% ` Ruijie Yu via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2023-01-08 6:00 0% ` Eli Zaretskii
2023-01-08 9:09 5% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-01-08 9:09 5% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
1 sibling, 2 replies; 63+ results
From: Eli Zaretskii @ 2023-01-08 6:00 UTC (permalink / raw)
To: Joseph Turner; +Cc: 60643
> Date: Sat, 07 Jan 2023 20:48:54 -0800
> From: Joseph Turner via "Bug reports for GNU Emacs,
> the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
>
> It appears that set-buffer-major-mode resets buffer local variables.
>
> To reproduce with `emacs -Q`:
>
> (defvar-local test-var nil)
> (setq test-var t)
> (message "%s" test-var) ;; t
> ;; (set-auto-mode)
> (set-buffer-major-mode (current-buffer))
> (message "%s" test-var) ;; nil
>
> Is this behavior expected?
Yes, I think so. Changing the major mode kills all buffer-local
variables. If you don't want some variable to be killed, give it the
permanent-local property.
^ permalink raw reply [relevance 0%]
* bug#60643: 29.0.50; set-buffer-major-mode resets buffer local variables
2023-01-08 4:48 5% bug#60643: 29.0.50; set-buffer-major-mode resets buffer local variables Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2023-01-08 5:45 5% ` Ruijie Yu via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-01-08 6:00 0% ` Eli Zaretskii
1 sibling, 0 replies; 63+ results
From: Ruijie Yu via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2023-01-08 5:45 UTC (permalink / raw)
To: Joseph Turner; +Cc: 60643
Joseph Turner via "Bug reports for GNU Emacs, the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org> writes:
> Hello!
>
> It appears that set-buffer-major-mode resets buffer local variables.
>
> To reproduce with `emacs -Q`:
>
> (defvar-local test-var nil)
> (setq test-var t)
> (message "%s" test-var) ;; t
> ;; (set-auto-mode)
> (set-buffer-major-mode (current-buffer))
> (message "%s" test-var) ;; nil
>
> Is this behavior expected?
>
> Thank you!!
>
> Joseph
>
> In GNU Emacs 29.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version
> 3.24.30, cairo version 1.16.0)
> Windowing system distributor 'The X.Org Foundation', version 11.0.12011000
> System Description: Debian GNU/Linux 11 (bullseye)
>
> Configured using:
> 'configure
> CONFIG_SHELL=/gnu/store/4y5m9lb8k3qkb1y9m02sw9w9a6hacd16-bash-minimal-5.1.8/bin/bash
> SHELL=/gnu/store/4y5m9lb8k3qkb1y9m02sw9w9a6hacd16-bash-minimal-5.1.8/bin/bash
> --prefix=/gnu/store/4mnib031vflf88ms8w7kyfahcbv1k9vc-emacs-next-29.0.50-3.22e8a77
> --enable-fast-install --with-modules --with-cairo
> --with-native-compilation --disable-build-details'
>
> Configured features:
> ACL CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GPM GSETTINGS HARFBUZZ JPEG
> JSON LCMS2 LIBOTF LIBSELINUX LIBSYSTEMD LIBXML2 M17N_FLT MODULES
> NATIVE_COMP NOTIFY INOTIFY PDUMPER PNG RSVG SECCOMP SOUND SQLITE3
> THREADS TIFF TOOLKIT_SCROLL_BARS X11 XDBE XIM XINPUT2 XPM GTK3 ZLIB
There is another concept called "permanent local variables" which you
might be thinking about when looking at "local variables", see info node
`(elisp) Standard Properties' for more information.
Best,
RY
^ permalink raw reply [relevance 5%]
* bug#60643: 29.0.50; set-buffer-major-mode resets buffer local variables
@ 2023-01-08 4:48 5% Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-01-08 5:45 5% ` Ruijie Yu via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-01-08 6:00 0% ` Eli Zaretskii
0 siblings, 2 replies; 63+ results
From: Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2023-01-08 4:48 UTC (permalink / raw)
To: 60643
Hello!
It appears that set-buffer-major-mode resets buffer local variables.
To reproduce with `emacs -Q`:
(defvar-local test-var nil)
(setq test-var t)
(message "%s" test-var) ;; t
;; (set-auto-mode)
(set-buffer-major-mode (current-buffer))
(message "%s" test-var) ;; nil
Is this behavior expected?
Thank you!!
Joseph
In GNU Emacs 29.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version
3.24.30, cairo version 1.16.0)
Windowing system distributor 'The X.Org Foundation', version 11.0.12011000
System Description: Debian GNU/Linux 11 (bullseye)
Configured using:
'configure
CONFIG_SHELL=/gnu/store/4y5m9lb8k3qkb1y9m02sw9w9a6hacd16-bash-minimal-5.1.8/bin/bash
SHELL=/gnu/store/4y5m9lb8k3qkb1y9m02sw9w9a6hacd16-bash-minimal-5.1.8/bin/bash
--prefix=/gnu/store/4mnib031vflf88ms8w7kyfahcbv1k9vc-emacs-next-29.0.50-3.22e8a77
--enable-fast-install --with-modules --with-cairo
--with-native-compilation --disable-build-details'
Configured features:
ACL CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GPM GSETTINGS HARFBUZZ JPEG
JSON LCMS2 LIBOTF LIBSELINUX LIBSYSTEMD LIBXML2 M17N_FLT MODULES
NATIVE_COMP NOTIFY INOTIFY PDUMPER PNG RSVG SECCOMP SOUND SQLITE3
THREADS TIFF TOOLKIT_SCROLL_BARS X11 XDBE XIM XINPUT2 XPM GTK3 ZLIB
^ permalink raw reply [relevance 5%]
* bug#60346: VC refresh error "Recursive load"
2022-12-26 21:26 4% bug#60346: VC refresh error "Recursive load" Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2022-12-27 14:05 0% ` Eli Zaretskii
0 siblings, 0 replies; 63+ results
From: Eli Zaretskii @ 2022-12-27 14:05 UTC (permalink / raw)
To: Joseph Turner, Stefan Monnier; +Cc: 60346
> Date: Mon, 26 Dec 2022 13:26:17 -0800
> From: Joseph Turner via "Bug reports for GNU Emacs,
> the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
>
> I wanted to ignore byte-compiled files because edebug was refusing to
> step into byte-compiled functions.
>
> I ran the following:
>
> emacs -Q --eval="(setq load-suffixes '(\".el\"))"
>
> This broke basic functionality, throwing a "Recursive load" error. For
> example, `M-x find-file ~/.emacs.d/init.el` gives:
>
> VC refresh error: (error "Recursive load" "/gnu/store/x8nykb09aqmp2j6k74dpgc4jvbk8c2bl-emacs-next-29.0.50-3.22e8a77/share/emacs/29.0.50/lisp/jka-compr.el.gz" "/gnu/store/x8nykb09aqmp2j6k74dpgc4jvbk8c2bl-emacs-next-29.0.50-3.22e8a77/share/emacs/29.0.50/lisp/jka-compr.el.gz" "/gnu/store/x8nykb09aqmp2j6k74dpgc4jvbk8c2bl-emacs-next-29.0.50-3.22e8a77/share/emacs/29.0.50/lisp/jka-compr.el.gz" "/gnu/store/x8nykb09aqmp2j6k74dpgc4jvbk8c2bl-emacs-next-29.0.50-3.22e8a77/share/emacs/29.0.50/lisp/jka-compr.el.gz" "/gnu/store/x8nykb09aqmp2j6k74dpgc4jvbk8c2bl-emacs-next-29.0.50-3.22e8a77/share/emacs/29.0.50/lisp/jka-compr.el.gz" "/gnu/store/x8nykb09aqmp2j6k74dpgc4jvbk8c2bl-emacs-next-29.0.50-3.22e8a77/share/emacs/29.0.50/lisp/vc/vc-git.el.gz")
> load-with-code-conversion: Recursive load: "/gnu/store/x8nykb09aqmp2j6k74dpgc4jvbk8c2bl-emacs-next-29.0.50-3.22e8a77/share/emacs/29.0.50/lisp/jka-compr.el.gz", "/gnu/store/x8nykb09aqmp2j6k74dpgc4jvbk8c2bl-emacs-next-29.0.50-3.22e8a77/share/emacs/29.0.50/lisp/jka-compr.el.gz", "/gnu/store/x8nykb09aqmp2j6k74dpgc4jvbk8c2bl-emacs-next-29.0.50-3.22e8a77/share/emacs/29.0.50/lisp/jka-compr.el.gz", "/gnu/store/x8nykb09aqmp2j6k74dpgc4jvbk8c2bl-emacs-next-29.0.50-3.22e8a77/share/emacs/29.0.50/lisp/jka-compr.el.gz", "/gnu/store/x8nykb09aqmp2j6k74dpgc4jvbk8c2bl-emacs-next-29.0.50-3.22e8a77/share/emacs/29.0.50/lisp/jka-compr.el.gz", "/gnu/store/x8nykb09aqmp2j6k74dpgc4jvbk8c2bl-emacs-next-29.0.50-3.22e8a77/share/emacs/29.0.50/lisp/vc/vc-git.el.gz"
>
> Emacs also breaks when load-suffixes are reordered:
>
> emacs -Q --eval="(setq load-suffixes '(\".el\" \".so\" \".elc\"))"
>
> I am running Guix on a foreign distro Debian Bullseye. Shall I send this
> issue to the Guix mailing list instead?
I'm not sure we want to support this kind of changes in the order or
contents of load-suffixes. It sounds to me like the order is there
for as reason, and no part of Emacs expects these lists to be
reordered, let alone have some extensions removed from them. I think
the only valid changes are adding extensions to the end.
Stefan, WDYT?
^ permalink raw reply [relevance 0%]
* bug#60346: VC refresh error "Recursive load"
@ 2022-12-26 21:26 4% Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
2022-12-27 14:05 0% ` Eli Zaretskii
0 siblings, 1 reply; 63+ results
From: Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2022-12-26 21:26 UTC (permalink / raw)
To: 60346
Hello,
I wanted to ignore byte-compiled files because edebug was refusing to
step into byte-compiled functions.
I ran the following:
emacs -Q --eval="(setq load-suffixes '(\".el\"))"
This broke basic functionality, throwing a "Recursive load" error. For
example, `M-x find-file ~/.emacs.d/init.el` gives:
VC refresh error: (error "Recursive load" "/gnu/store/x8nykb09aqmp2j6k74dpgc4jvbk8c2bl-emacs-next-29.0.50-3.22e8a77/share/emacs/29.0.50/lisp/jka-compr.el.gz" "/gnu/store/x8nykb09aqmp2j6k74dpgc4jvbk8c2bl-emacs-next-29.0.50-3.22e8a77/share/emacs/29.0.50/lisp/jka-compr.el.gz" "/gnu/store/x8nykb09aqmp2j6k74dpgc4jvbk8c2bl-emacs-next-29.0.50-3.22e8a77/share/emacs/29.0.50/lisp/jka-compr.el.gz" "/gnu/store/x8nykb09aqmp2j6k74dpgc4jvbk8c2bl-emacs-next-29.0.50-3.22e8a77/share/emacs/29.0.50/lisp/jka-compr.el.gz" "/gnu/store/x8nykb09aqmp2j6k74dpgc4jvbk8c2bl-emacs-next-29.0.50-3.22e8a77/share/emacs/29.0.50/lisp/jka-compr.el.gz" "/gnu/store/x8nykb09aqmp2j6k74dpgc4jvbk8c2bl-emacs-next-29.0.50-3.22e8a77/share/emacs/29.0.50/lisp/vc/vc-git.el.gz")
load-with-code-conversion: Recursive load: "/gnu/store/x8nykb09aqmp2j6k74dpgc4jvbk8c2bl-emacs-next-29.0.50-3.22e8a77/share/emacs/29.0.50/lisp/jka-compr.el.gz", "/gnu/store/x8nykb09aqmp2j6k74dpgc4jvbk8c2bl-emacs-next-29.0.50-3.22e8a77/share/emacs/29.0.50/lisp/jka-compr.el.gz", "/gnu/store/x8nykb09aqmp2j6k74dpgc4jvbk8c2bl-emacs-next-29.0.50-3.22e8a77/share/emacs/29.0.50/lisp/jka-compr.el.gz", "/gnu/store/x8nykb09aqmp2j6k74dpgc4jvbk8c2bl-emacs-next-29.0.50-3.22e8a77/share/emacs/29.0.50/lisp/jka-compr.el.gz", "/gnu/store/x8nykb09aqmp2j6k74dpgc4jvbk8c2bl-emacs-next-29.0.50-3.22e8a77/share/emacs/29.0.50/lisp/jka-compr.el.gz", "/gnu/store/x8nykb09aqmp2j6k74dpgc4jvbk8c2bl-emacs-next-29.0.50-3.22e8a77/share/emacs/29.0.50/lisp/vc/vc-git.el.gz"
Emacs also breaks when load-suffixes are reordered:
emacs -Q --eval="(setq load-suffixes '(\".el\" \".so\" \".elc\"))"
I am running Guix on a foreign distro Debian Bullseye. Shall I send this
issue to the Guix mailing list instead?
Thank you!
Joseph
^ permalink raw reply [relevance 4%]
Results 401-463 of 463 prev (newer) | reverse | options above
-- pct% links below jump to the message on this page, permalinks otherwise --
2022-12-26 21:26 4% bug#60346: VC refresh error "Recursive load" Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
2022-12-27 14:05 0% ` Eli Zaretskii
2023-01-08 4:48 5% bug#60643: 29.0.50; set-buffer-major-mode resets buffer local variables Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-01-08 5:45 5% ` Ruijie Yu via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-01-08 6:00 0% ` Eli Zaretskii
2023-01-08 9:09 5% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-01-08 9:09 5% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-05-06 20:39 9% bug#63336: [PATCH] package-vc: Process :make and :shell-command spec args Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-05-07 9:03 5% ` Philip Kaludercic
2023-05-07 18:47 8% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-05-08 8:42 0% ` Ruijie Yu via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-05-08 19:38 9% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-05-10 6:35 5% ` Philip Kaludercic
2023-05-11 1:37 9% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-05-14 7:44 5% ` Philip Kaludercic
2023-05-14 8:08 8% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-05-14 19:30 5% ` Philip Kaludercic
2023-05-14 23:01 8% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-05-15 9:12 5% ` Philip Kaludercic
2023-05-15 19:03 5% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-05-16 19:29 ` Philip Kaludercic
2023-05-16 21:08 5% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-05-17 14:07 5% ` Philip Kaludercic
2023-05-13 17:18 0% ` Philip Kaludercic
2023-05-06 20:54 10% bug#63337: [PATCH] package-vc--build-documentation: Fix relative @include statements Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-05-07 9:58 5% ` Philip Kaludercic
2023-05-07 10:56 0% ` Eli Zaretskii
2023-05-07 18:40 10% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-05-07 19:11 0% ` Eli Zaretskii
2023-05-07 19:19 0% ` Philip Kaludercic
2023-05-07 20:29 9% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-05-08 13:51 5% ` Philip Kaludercic
2023-05-08 19:05 9% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-05-09 1:34 5% ` Ruijie Yu via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-05-09 2:48 5% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-05-09 4:36 0% ` Eli Zaretskii
2023-05-09 23:49 10% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-05-10 6:51 5% ` Philip Kaludercic
2023-05-11 2:04 5% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-05-12 6:51 5% ` Philip Kaludercic
2023-05-12 7:14 ` Eli Zaretskii
2023-05-12 7:35 ` Philip Kaludercic
2023-05-13 5:54 5% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-05-12 6:56 5% ` Philip Kaludercic
2023-05-13 5:47 5% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-05-13 8:41 5% ` Philip Kaludercic
2023-05-13 16:38 10% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-05-13 17:14 5% ` Philip Kaludercic
2023-05-13 18:31 5% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-05-15 3:57 10% bug#63509: [PATCH] Make copy-tree work with records Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-05-15 11:26 0% ` Eli Zaretskii
2023-05-15 17:59 7% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-05-18 10:53 0% ` Eli Zaretskii
2023-05-18 19:05 7% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-05-19 6:07 0% ` Eli Zaretskii
2023-05-15 5:56 10% bug#63513: [PATCH] Make persist-defvar work with records and hash tables Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-05-15 11:31 0% ` Eli Zaretskii
2023-05-23 20:14 9% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-05-23 19:32 5% bug#63671: Add function to test equality of " Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-05-24 9:27 Mattias Engdegård
2023-05-24 17:27 5% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-05-24 20:01 5% ` Ihor Radchenko
2023-05-24 20:34 4% ` Mattias Engdegård
2023-05-25 2:44 5% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-05-25 6:31 5% ` Ihor Radchenko
2023-05-25 7:22 5% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-05-25 7:22 5% ` Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).