cl-defstruct-based package.el, now with ert tests and no external tar!

cl-defstruct-based package.el, now with ert tests and no external tar!

[Daniel Hackney]

See http://thread.gmane.org/gmane.emacs.devel/157754 for the previous
discussion.

I removed the use of an external "tar" command from my package.el
rewrite and humbly submit it once again for consideration.

https://github.com/haxney/emacs/tree/package-rewrite

It passes all the tests I've given it and is greatly cleaned up from
current version of package.el.

If it is accepted into the trunk, I would like to backport it to 24.3
so that improvements can be made and distributed to people (such as
package signing). We would obviously have to be very careful to avoid
breaking people's existing setups.

--
Daniel Hackney

Re: cl-defstruct-based package.el, now with ert tests and no external tar!

[Stefan Monnier]

> https://github.com/haxney/emacs/tree/package-rewrite

Could you send a patch for it (or a command to run to get the patch,
assuming I only have a Bazaar branch of Emacs)?


        Stefan

Re: cl-defstruct-based package.el, now with ert tests and no external tar!

[Dmitry Gutov]

Stefan Monnier <monnier@iro.umontreal.ca> writes:
>> https://github.com/haxney/emacs/tree/package-rewrite
>
> Could you send a patch for it (or a command to run to get the patch,
> assuming I only have a Bazaar branch of Emacs)?

It doesn't seem to apply cleanly to trunk, but here's the diff:

https://github.com/haxney/emacs/compare/3a5be1e2bf8b8b096f7c656c99f644b7dd0c4bab...package-rewrite.diff

It also has no ChangeLog entries (currently; the contents of the above
URL will update if/when Daniel adds them).

--Dmitry

Re: cl-defstruct-based package.el, now with ert tests and no external tar!

[Stefan Monnier]

> It doesn't seem to apply cleanly to trunk, but here's the diff:

> https://github.com/haxney/emacs/compare/3a5be1e2bf8b8b096f7c656c99f644b7dd0c4bab...package-rewrite.diff

Thanks,


        Stefan

Re: cl-defstruct-based package.el, now with ert tests and no external tar!

[Daniel Hackney]

Dmitry Gutov <dgutov@yandex.ru> wrote:
> It doesn't seem to apply cleanly to trunk, but here's the diff:
>
> https://github.com/haxney/emacs/compare/3a5be1e2bf8b8b096f7c656c99f644b7dd0c4bab...package-rewrite.diff
>
> It also has no ChangeLog entries (currently; the contents of the above
> URL will update if/when Daniel adds them).

I guess I'll have to rebase it off of trunk. Sorry about not giving a
usable diff.

I realized that I have a few things left to do for the patch:

 - Create ChangeLog entries
 - Update package-x.el. I haven't touched it and it is going to be
   horribly broken.

A question: should I submit this as a single large patch or break it
up? It doesn't make much sense to have it broken up, since it is really
an all-or-nothing overhaul, but if people want it broken up, I can
certainly do so.

--
Daniel Hackney

Re: cl-defstruct-based package.el, now with ert tests and no external tar!

[Ted Zlatanov]

On Fri, 5 Apr 2013 16:51:00 -0400 Daniel Hackney <dan@haxney.org> wrote: 

DH> Dmitry Gutov <dgutov@yandex.ru> wrote:
>> It doesn't seem to apply cleanly to trunk, but here's the diff:
>> 
>> https://github.com/haxney/emacs/compare/3a5be1e2bf8b8b096f7c656c99f644b7dd0c4bab...package-rewrite.diff
>> 
>> It also has no ChangeLog entries (currently; the contents of the above
>> URL will update if/when Daniel adds them).

DH> I guess I'll have to rebase it off of trunk. Sorry about not giving a
DH> usable diff.

DH> I realized that I have a few things left to do for the patch:

DH>  - Create ChangeLog entries
DH>  - Update package-x.el. I haven't touched it and it is going to be
DH>    horribly broken.

DH> A question: should I submit this as a single large patch or break it
DH> up? It doesn't make much sense to have it broken up, since it is really
DH> an all-or-nothing overhaul, but if people want it broken up, I can
DH> certainly do so.

In the end it should be a single commit to the Bazaar repo, and there's
no reason to break it up for your audience here.

Ted

Re: cl-defstruct-based package.el, now with ert tests and no external tar!

[Stefan Monnier]

> In the end it should be a single commit to the Bazaar repo, and there's
> no reason to break it up for your audience here.

I've been looking at the patch, trying to understand what it does, and
I think I'd first really need to see a good description of what it does,
because obviously it doesn't just switch to using a defstruct.  It also
does things like renaming functions and various minor refactorings that
don't seem directly related (they don't necessarily seem bad either,
but I wasn't able to understand all of what's going on from just
looking at the patch).

I did notice that package-desc-from-define uses ignore-errors where the
previous code did not.  `ignore-errors' can be handy, but it should be
used with restraint because it too easily hides problems.  Since the
previous code didn't use them, I don't see why we'd need them now.


        Stefan

Re: cl-defstruct-based package.el, now with ert tests and no external tar!

[Daniel Hackney]

I forgot to respond on-list. Here's what I said:

Stefan Monnier <monnier@iro.umontreal.ca> wrote:

> I'd like to move forward on this but haven't heard from you about the
> comments below.  I just now realize that I only sent it to emacs-devel,
> so maybe you just didn't see it.

Sorry, I haven't had much time to work on this recently. I'll try to
get some done now.

>>>>>> "Stefan" == Stefan Monnier <monnier@iro.umontreal.ca> writes:
>
>>> In the end it should be a single commit to the Bazaar repo, and there's
>>> no reason to break it up for your audience here.

Agreed.

>> I've been looking at the patch, trying to understand what it does, and
>> I think I'd first really need to see a good description of what it does,
>> because obviously it doesn't just switch to using a defstruct.  It also
>> does things like renaming functions and various minor refactorings that
>> don't seem directly related (they don't necessarily seem bad either,
>> but I wasn't able to understand all of what's going on from just
>> looking at the patch).

I'll do a better job of explaining the whole patch. Should I include
that in some sort of file in the repo or just on the mailing list?
Would a detailed-ish explanation of the changes and rationale be
appropriate for the ChangeLog or NEWS files?

>> I did notice that package-desc-from-define uses ignore-errors where the
>> previous code did not.  `ignore-errors' can be handy, but it should be
>> used with restraint because it too easily hides problems.  Since the
>> previous code didn't use them, I don't see why we'd need them now.

I forget why I did this. I'll look into it now.

I also remembered that I haven't touched package-x.el. This would be
kind of important. I'll write tests for it as well.

About package-x.el, is the HTML and RSS updating functionality actually
used? Currently, the only way to access the functionality is calling
`package-maint-add-news-item' or the non-interactive
`package-upload-buffer-internal' directly. GNU ELPA clearly doesn't use
the version in package-x.el, as the HTML generated is not what
package-x produces. Can I consider it unused and delete it? If not,
I'll refactor it with the rest of the code.

--
Daniel Hackney

Re: cl-defstruct-based package.el, now with ert tests and no external tar!

[Dmitry Gutov]

Hi Daniel,

Any news on this?

I've been holding off on Bug#13291 both because it would complicate the
rebase of this changeset, and also because I'd like to use the test
harness you have here.

Daniel Hackney <dan@haxney.org> writes:
>>> I've been looking at the patch, trying to understand what it does, and
>>> I think I'd first really need to see a good description of what it does,
>>> because obviously it doesn't just switch to using a defstruct.  It also
>>> does things like renaming functions and various minor refactorings that
>>> don't seem directly related (they don't necessarily seem bad either,
>>> but I wasn't able to understand all of what's going on from just
>>> looking at the patch).
>
> I'll do a better job of explaining the whole patch. Should I include
> that in some sort of file in the repo or just on the mailing list?
> Would a detailed-ish explanation of the changes and rationale be
> appropriate for the ChangeLog or NEWS files?

You can start with ChangeLog files. When writing a changelog entry, one
usually briefly describes and sometimes justifies the mechanical
transformations being performed on the code, and it helps other people
understand the changes. I'd hold off on writing a separate extensive
description until receiving follow-up questions.

I see you've also already started on NEWS entries.

> About package-x.el, is the HTML and RSS updating functionality actually
> used? Currently, the only way to access the functionality is calling
> `package-maint-add-news-item' or the non-interactive
> `package-upload-buffer-internal' directly. GNU ELPA clearly doesn't use
> the version in package-x.el, as the HTML generated is not what
> package-x produces.

Probably not. Melpa and Elpakit don't seem to be using it either.

> Can I consider it unused and delete it? If not, I'll refactor it with
> the rest of the code.

Personally, I'd delete them, but that's not my call. Maybe decorate them
with FIXMEs, leave them unfixed and see if anyone complains up until the
pretest?

Re: cl-defstruct-based package.el, now with ert tests and no external tar!

[Daniel Hackney]

Dmitry Gutov <dgutov@yandex.ru> wrote:
> Any news on this?

I was finishing up on exams, so this had to be put on hold. I'm
starting up on it again. I think it's close to a mergable state, so
I'll finish up the NEWS entry and submit the patch for merging.

> I've been holding off on Bug#13291 both because it would complicate
> the rebase of this changeset, and also because I'd like to use the
> test harness you have here.

Sounds good. I'll try to get things on my end totally finished so
development can resume on the new version.

> Daniel Hackney <dan@haxney.org> writes:
>> I'll do a better job of explaining the whole patch. Should I include
>> that in some sort of file in the repo or just on the mailing list?
>> Would a detailed-ish explanation of the changes and rationale be
>> appropriate for the ChangeLog or NEWS files?
>
> You can start with ChangeLog files. When writing a changelog entry,
> one usually briefly describes and sometimes justifies the mechanical
> transformations being performed on the code, and it helps other people
> understand the changes.

Since it is a complete refactoring, what should I say? Should I include
a line for each new or changed function, or simply refer people to the
NEWS file?

> I see you've also already started on NEWS entries.

How does it look so far? Any suggestions on how to improve it?

>> About package-x.el, is the HTML and RSS updating functionality actually
>> used? Currently, the only way to access the functionality is calling
>> `package-maint-add-news-item' or the non-interactive
>> `package-upload-buffer-internal' directly. GNU ELPA clearly doesn't use
>> the version in package-x.el, as the HTML generated is not what
>> package-x produces.
>
> Probably not. Melpa and Elpakit don't seem to be using it either.
>
>> Can I consider it unused and delete it? If not, I'll refactor it with
>> the rest of the code.
>
> Personally, I'd delete them, but that's not my call. Maybe decorate them
> with FIXMEs, leave them unfixed and see if anyone complains up until the
> pretest?

I did some porting and they should continue to work, although they don't
do much of anything useful, just like before.

One other change I made, which probably deserves some discussion, is
that I created a folder "test/automated/data" which is intended to hold
test-related data. I created a subfolder "package" (so it is
"test/automated/data/package") which contains a test "archive-contents"
file, as well as test elisp files. My hope is that such a directory
could be useful for other tests which need example data.

I also added a rule in the "Makefile.in" to skip byte-compilation of the
files in "data".

--
Daniel Hackney

Re: cl-defstruct-based package.el, now with ert tests and no external tar!

[Ted Zlatanov]

On Tue, 4 Jun 2013 17:25:06 -0400 Daniel Hackney <dan@haxney.org> wrote: 

DH> One other change I made, which probably deserves some discussion, is
DH> that I created a folder "test/automated/data" which is intended to hold
DH> test-related data. I created a subfolder "package" (so it is
DH> "test/automated/data/package") which contains a test "archive-contents"
DH> file, as well as test elisp files. My hope is that such a directory
DH> could be useful for other tests which need example data.

DH> I also added a rule in the "Makefile.in" to skip byte-compilation of the
DH> files in "data".

Unit tests are very nice.  Maybe the GNU ELPA could also have a "sample"
area for testing packages (outside of the main packages) with a remote
URL, SSL certificates, signing, etc. to duplicate the real interaction
users will experience as much as possible.

Ted

Re: cl-defstruct-based package.el, now with ert tests and no external tar!

[Dmitry Gutov]

Daniel Hackney <dan@haxney.org> writes:
>> You can start with ChangeLog files. When writing a changelog entry,
>> one usually briefly describes and sometimes justifies the mechanical
>> transformations being performed on the code, and it helps other people
>> understand the changes.
>
> Since it is a complete refactoring, what should I say? Should I include
> a line for each new or changed function

I think so: http://www.gnu.org/prep/standards/html_node/Change-Logs.html

>> I see you've also already started on NEWS entries.
>
> How does it look so far? Any suggestions on how to improve it?

Actually, I'm not entirely sure the NEWS entries are appropriate in this
case.

Compare: "file named NEWS which contains a list of user-visible changes
worth mentioning"
(http://www.gnu.org/prep/standards/html_node/NEWS-File.html).

and

"User-facing commands have not changed and existing archives and files
will continue to work as before;"

You can put the justification in the email message accompanying
the patch, and then the person installing the patch can add the link to
the message in the ChangeLog. Alternatively, you can submit the patch as
a bug report, then the ChangeLog entry can reference it, this seems to
be the most proper option.

> One other change I made, which probably deserves some discussion, is
> that I created a folder "test/automated/data" which is intended to hold
> test-related data. I created a subfolder "package" (so it is
> "test/automated/data/package") which contains a test "archive-contents"
> file, as well as test elisp files. My hope is that such a directory
> could be useful for other tests which need example data.
>
> I also added a rule in the "Makefile.in" to skip byte-compilation of the
> files in "data".

Sounds good to me. I would probably make it test/automated/package/data
(and put the package tests inside automated/package, to bring them
closer), but I guess that would make the respective rule(s) in
Makefile.in more complicated, if/when other packages do the same.

Re: cl-defstruct-based package.el, now with ert tests and no external tar!

[Stefan Monnier]

I installed a patch which includes a part of your patch.


        Stefan

Re: cl-defstruct-based package.el, now with ert tests and no external tar!

[Stefan Monnier]

> I installed a patch which includes a part of your patch.

The last patch I installed includes further parts of your patch, tho
heavily reworked.
I think overall, this integrates most, if not all of your changes.

Trying to merge your patch with the current tip gives me a "residue" of
the following (fully untested, most probably broken) patch, FWIW.


        Stefan


Using changes with id "33".
Message: package.el patch from Hackney
 M  lisp/emacs-lisp/package.el
=== modified file 'lisp/emacs-lisp/package.el'
--- a/lisp/emacs-lisp/package.el	2013-06-21 04:19:53 +0000
+++ b/lisp/emacs-lisp/package.el	2013-06-21 04:20:02 +0000
@@ -418,6 +418,12 @@
 	  (pop str-list))
       (apply 'concat (nreverse str-list)))))
 
+(defun package-desc-install-dir (desc)
+  "Return the install directory of DESC."
+  (file-name-as-directory
+   (expand-file-name (package-desc-full-name desc)
+                     package-user-dir)))
+
 (defun package-load-descriptor (pkg-dir)
   "Load the description file in directory PKG-DIR."
   (let ((pkg-file (expand-file-name (package--description-file pkg-dir)
@@ -586,27 +592,26 @@
 ;; From Emacs 22, but changed so it adds to load-path.
 (defun package-autoload-ensure-default-file (file)
   "Make sure that the autoload file FILE exists and if not create it."
-  (unless (file-exists-p file)
-    (write-region
-     (concat ";;; " (file-name-nondirectory file)
-	     " --- automatically extracted autoloads\n"
-	     ";;\n"
-	     ";;; Code:\n"
-             "(add-to-list 'load-path (or (file-name-directory #$) (car load-path)))\n"
-	     "\f\n;; Local Variables:\n"
-	     ";; version-control: never\n"
-	     ";; no-byte-compile: t\n"
-	     ";; no-update-autoloads: t\n"
-	     ";; End:\n"
-	     ";;; " (file-name-nondirectory file)
-	     " ends here\n")
-     nil file))
-  file)
+  (write-region
+   (concat ";;; " (file-name-nondirectory file)
+           " --- automatically extracted autoloads\n"
+           ";;\n"
+           ";;; Code:\n"
+           "(add-to-list 'load-path (or (file-name-directory #$) (car load-path)))\n"
+           "\f\n;; Local Variables:\n"
+           ";; version-control: never\n"
+           ";; no-byte-compile: t\n"
+           ";; no-update-autoloads: t\n"
+           ";; End:\n"
+           ";;; " (file-name-nondirectory file)
+           " ends here\n")
+   nil file))
 
-(defun package-generate-autoloads (name pkg-dir)
-  (require 'autoload)         ;Load before we let-bind generated-autoload-file!
-  (let* ((auto-name (format "%s-autoloads.el" name))
-	 ;;(ignore-name (concat name "-pkg.el"))
+(defun package-generate-autoloads (desc)
+  "Generate autoloads for package DESC."
+  (require 'autoload)         ;; Load before we let-bind generated-autoload-file!
+  (let* ((auto-name (format "%s-autoloads.el" (package-desc-name desc)))
+         (pkg-dir (package-desc-install-dir desc))
 	 (generated-autoload-file (expand-file-name auto-name pkg-dir))
 	 (version-control 'never))
     (package-autoload-ensure-default-file generated-autoload-file)
@@ -621,10 +626,8 @@
 (declare-function tar-header-link-type "tar-mode" (tar-header) t)
 
 (defun package-untar-buffer (dir)
-  "Untar the current buffer.
-This uses `tar-untar-buffer' from Tar mode.  All files should
-untar into a directory named DIR; otherwise, signal an error."
-  (require 'tar-mode)
+  "Untar the current buffer into DIR.
+This uses `tar-untar-buffer' from Tar mode."
   (tar-mode)
   ;; Make sure everything extracts into DIR.
   (let ((regexp (concat "\\`" (regexp-quote (expand-file-name dir)) "/"))
@@ -764,16 +767,15 @@
 
 (defvar package--initialized nil)
 
-(defun package-installed-p (package &optional min-version)
-  "Return true if PACKAGE, of MIN-VERSION or newer, is installed.
-MIN-VERSION should be a version list."
-  (unless package--initialized (error "package.el is not yet initialized!"))
-  (let ((pkg-desc (assq package package-alist)))
+(defun package-installed-p (name &optional min-version)
+  "Return true if NAME, of MIN-VERSION or newer, is installed.
+NAME must be a symbol and MIN-VERSION must be a version list."
+  (let ((pkg-desc (assq name package-alist)))
     (if pkg-desc
 	(version-list-<= min-version
 			 (package-desc-version (cdr pkg-desc)))
       ;; Also check built-in packages.
-      (package-built-in-p package min-version))))
+      (package-built-in-p name min-version))))
 
 (defun package-compute-transaction (package-list requirements)
   "Return a list of packages to be installed, including PACKAGE-LIST.
@@ -863,8 +865,6 @@
   "Re-read archive contents for ARCHIVE.
 If successful, set the variable `package-archive-contents'.
 If the archive version is too new, signal an error."
-  ;; Version 1 of 'archive-contents' is identical to our internal
-  ;; representation.
   (let* ((contents-file (format "archives/%s/archive-contents" archive))
 	 (contents (package--read-archive-file contents-file)))
     (when contents
@@ -917,7 +917,7 @@
                   (delq existing-package
                         package-archive-contents)))))))
 
-(defun package-download-transaction (package-list)
+(defun package-install-transaction (package-list)
   "Download and install all the packages in PACKAGE-LIST.
 PACKAGE-LIST should be a list of package names (symbols).
 This function assumes that all package requirements in
@@ -953,7 +953,9 @@
          (error "Package `%s' is not available for installation"
                 name))
        (list pkg-desc))))
-  (package-download-transaction
+  (unless package--initialized
+    (package-initialize t))
+  (package-install-transaction
    ;; FIXME: Use (list pkg-desc) instead of just the name.
    (package-compute-transaction (list (package-desc-name pkg-desc))
                                 (package-desc-reqs pkg-desc))))
@@ -980,9 +982,9 @@
   (unless (re-search-forward "^;;; \\([^ ]*\\)\\.el ---[ \t]*\\(.*?\\)[ \t]*\\(-\\*-.*-\\*-[ \t]*\\)?$" nil t)
     (error "Packages lacks a file header"))
   (let ((file-name (match-string-no-properties 1))
-	(desc      (match-string-no-properties 2))
-	(start     (line-beginning-position)))
-    (unless (search-forward (concat ";;; " file-name ".el ends here"))
+        (summary   (match-string-no-properties 2))
+        (start     (line-beginning-position)))
+    (unless (search-forward (format ";;; %s.el ends here"  file-name))
       (error "Package lacks a terminating comment"))
     ;; Try to include a trailing newline.
     (forward-line)
@@ -999,8 +1001,8 @@
 	(error
 	 "Package lacks a \"Version\" or \"Package-Version\" header"))
       (package-desc-from-define
-       file-name pkg-version desc
-       (if requires-str (package-read-from-string requires-str))
+       file-name pkg-version summary
+       (package-read-from-string requirements)
        :kind 'single))))
 
 (defun package-tar-file-info ()
@@ -1057,16 +1059,19 @@
     (package-install-from-buffer)))
 
 (defun package-delete (pkg-desc)
-  (let ((dir (package-desc-dir pkg-desc)))
-    (if (string-equal (file-name-directory dir)
-		      (file-name-as-directory
-		       (expand-file-name package-user-dir)))
-	(progn
-	  (delete-directory dir t t)
-	  (message "Package `%s' deleted." (package-desc-full-name pkg-desc)))
+  (let ((dir (package-desc-dir pkg-desc))
+        (full-name (package-desc-full-name pkg-desc)))
+    (cond
+     ((not (stringp dir))
+      (message "Package `%s' already deleted." full-name))
+     ((string-equal (file-name-directory dir)
+                    (file-name-as-directory
+                     (expand-file-name package-user-dir)))
+      (delete-directory dir t t)
+      (message "Package `%s' deleted." full-name))
+     (t
       ;; Don't delete "system" packages
-      (error "Package `%s' is a system package, not deleting"
-	     (package-desc-full-name pkg-desc)))))
+      (error "Package `%s' is a system package, not deleting" full-name))))
 
 (defun package-archive-base (desc)
   "Return the archive containing the package NAME."
@@ -1230,7 +1235,7 @@
 	(dolist (req reqs)
 	  (setq name (car req)
 		vers (cadr req)
-		text (format "%s-%s" (symbol-name name)
+		text (format "%s-%s" name
 			     (package-version-join vers)))
 	  (cond (first (setq first nil))
 		((>= (+ 2 (current-column) (length text))
@@ -1526,7 +1531,7 @@
   (let (installed available upgrades)
     ;; Build list of installed/available packages in this buffer.
     (dolist (entry tabulated-list-entries)
-      ;; ENTRY is (PKG-DESC [NAME VERSION STATUS DOC])
+      ;; ENTRY is (PKG-DESC [NAME VERSION-STRING STATUS DOC])
       (let ((pkg-desc (car entry))
 	    (status (aref (cadr entry) 2)))
 	(cond ((equal status "installed")
@@ -1621,12 +1626,10 @@
 		(package-delete elt)
 	      (error (message (cadr err)))))
 	(error "Aborted")))
-    ;; If we deleted anything, regenerate `package-alist'.  This is done
-    ;; automatically if we installed a package.
-    (and delete-list (null install-list)
-	 (package-initialize))
     (if (or delete-list install-list)
-	(package-menu--generate t t)
+        (progn
+          (package-initialize)
+          (package-menu--generate t t))
       (message "No operations specified."))))
 
 (defun package-menu--version-predicate (A B)
@@ -1698,15 +1701,16 @@
 	(package-menu--generate nil t))
       ;; The package menu buffer has keybindings.  If the user types
       ;; `M-x list-packages', that suggests it should become current.
-      (switch-to-buffer buf))
+      (switch-to-buffer buf)
 
-    (let ((upgrades (package-menu--find-upgrades)))
-      (if upgrades
-	  (message "%d package%s can be upgraded; type `%s' to mark %s for upgrading."
-		   (length upgrades)
-		   (if (= (length upgrades) 1) "" "s")
-		   (substitute-command-keys "\\[package-menu-mark-upgrades]")
-		   (if (= (length upgrades) 1) "it" "them"))))))
+      (let ((upgrades (package-menu--find-upgrades)))
+        (if upgrades
+            (message "%d package%s can be upgraded; type `%s' to mark %s for upgrading."
+                     (length upgrades)
+                     (if (= (length upgrades) 1) "" "s")
+                     (substitute-command-keys "\\[package-menu-mark-upgrades]")
+                     (if (= (length upgrades) 1) "it" "them"))))
+      buf)))
 
 ;;;###autoload
 (defalias 'package-list-packages 'list-packages)

Re: cl-defstruct-based package.el, now with ert tests and no external tar!

[Dmitry Gutov]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> I installed a patch which includes a part of your patch.
>
> The last patch I installed includes further parts of your patch, tho
> heavily reworked.
> I think overall, this integrates most, if not all of your changes.

AFAICT, you've omitted the whole "automated testing" part of the
changes, which was one of the main things I liked personally.

Any particular reason?

Re: cl-defstruct-based package.el, now with ert tests and no external tar!

[Stefan Monnier]

> AFAICT, you've omitted the whole "automated testing" part of the
> changes, which was one of the main things I liked personally.

I indeed focused on the package.el part itself, since that was the
problematic part (where there was no clear description of what the
patch was trying to do).

> Any particular reason?

No, oversight only.  Feel free to add it.


        Stefan

Re: cl-defstruct-based package.el, now with ert tests and no external tar!

[Sebastian Wiesner]

2013/6/5 Dmitry Gutov <dgutov@yandex.ru>:
> Daniel Hackney <dan@haxney.org> writes:
>>> You can start with ChangeLog files. When writing a changelog entry,
>>> one usually briefly describes and sometimes justifies the mechanical
>>> transformations being performed on the code, and it helps other people
>>> understand the changes.
>>
>> Since it is a complete refactoring, what should I say? Should I include
>> a line for each new or changed function
>
> I think so: http://www.gnu.org/prep/standards/html_node/Change-Logs.html
>
>>> I see you've also already started on NEWS entries.
>>
>> How does it look so far? Any suggestions on how to improve it?
>
> Actually, I'm not entirely sure the NEWS entries are appropriate in this
> case.
>
> Compare: "file named NEWS which contains a list of user-visible changes
> worth mentioning"
> (http://www.gnu.org/prep/standards/html_node/NEWS-File.html).
>
> and
>
> "User-facing commands have not changed and existing archives and files
> will continue to work as before;"

The API *has* changed.  We (that is, the Carton team [1]) “use” this
API.  We need to port Carton to the new API, and NEWS entries
explaining the changes would help.

[1]: https://github.com/rejeep/carton

Re: cl-defstruct-based package.el, now with ert tests and no external tar!

[Dmitry Gutov]

On 21.06.2013 18:56, Stefan Monnier wrote:
>> AFAICT, you've omitted the whole "automated testing" part of the
>> changes, which was one of the main things I liked personally.
>
> I indeed focused on the package.el part itself, since that was the
> problematic part (where there was no clear description of what the
> patch was trying to do).
>
>> Any particular reason?
>
> No, oversight only.  Feel free to add it.

Just an update, I've tried to bring the tests over today, but they're 
quite dependent on the implementation.

I still have 6/10 failures, so far.

One thing I've noticed that you omitted is the inclusion of the 
"commentary" field in the package-desc struct.

Not sure what the justification for its inclusion was, but I've had to 
remove the related assertions from the test file.

Daniel:

One thing I definitely didn't like is the usage of raw cl-struct vectors 
as reference values to compare against. These are implementation details 
of cl-defstruct, and we have no business setting them in stone in a test 
suite for code that just uses defstruct.

Relying on an external program (ginstall-info) is also not nice, for the 
usual alternative-OS-related reasons.

Re: cl-defstruct-based package.el, now with ert tests and no external tar!

[Stefan Monnier]

> The API *has* changed.

It probably has, depends what you mean by API.  Since all the code is
accessible, any change can break some external package.

If something broke for you, then please say so explicitly, and we'll see
whether we think it could/should be fixed, or whether you're just out
of luck.

> We (that is, the Carton team [1]) “use” this API.

What API?

> [1]: https://github.com/rejeep/carton

I don't understand what this does.  Could you give me some description
of what is the benefit of such a tool, either for an end-user or for
a developer of a package?

The page you link to shows commands you can run, but they don't seem to
correspond to anything I've had to do or felt like doing w.r.t to either
creating or installing packages.

IOW, I must be missing something,


        Stefan

Re: cl-defstruct-based package.el, now with ert tests and no external tar!

[Sebastian Wiesner]

2013/6/25 Stefan Monnier <monnier@iro.umontreal.ca>:
>> The API *has* changed.
>
> It probably has, depends what you mean by API.  Since all the code is
> accessible, any change can break some external package.
>
> If something broke for you, then please say so explicitly, and we'll see
> whether we think it could/should be fixed, or whether you're just out
> of luck.

Well, the list is long:

- "package-archive-contents" and "package-alist" have different
contents now, because the package descriptors have changed,
- "package-delete" takes a single argument only, but used to take two,
- "package-obsolete-alist" is gone,
- "package-install" doesn't accept a package name anymore,
- etc.

Since most of these changes come from the introducing of
"package-desc" struct, I know that I am out of luck.

Interestingly, all the internal API that we use doesn't seem to have
changed.  "package-menu--generate" and "package-menu--find-upgrades"
work as before, and not less awkward to use.

>> We (that is, the Carton team [1]) “use” this API.
>
> What API?

Well, the package.el API, that is, "package-install",
"package-delete", "package-alist", and unfortunately a number of
internal functions ("package--*"), too, because the public API is
somewhat limited.

>> [1]: https://github.com/rejeep/carton
>
> I don't understand what this does.  Could you give me some description
> of what is the benefit of such a tool, either for an end-user or for
> a developer of a package?

Carton creates isolated and independent package environments.

For an end user, it's just a declarative way to specify all packages
used in her configuration, installable with a single shell command,
but for a package developer, it maintains an isolated, automated and
repeatable package environment for testing.

Say, I develop a package "foo" which depends on dash.el and s.el, and
also on ERT and Ecukes for unit and integration tests.  I manage these
dependencies through Carton, and use "carton exec" to run my test
suite.  Now

- my "~/.emacs.d" packages are separated from the packages used to run
the tests of "foo",
- so my "~/.emacs.d" is not cluttered with all sorts of development
dependencies from various packages I write,
- and the test environment of "foo" is clean of any unwanted packages,
so I cannot accidentally use a function from a package that's present
in my "~/.emacs.d" but not specified as dependency,
- the test environment of "foo" is repeatable, and can be set up with
just a single shell command, whether on my personal laptop, the
desktop system of a new contributor, or a CI service like Travis CI,
- etc.

Essentially, it's Bundler, but for Emacs Lisp.

Re: cl-defstruct-based package.el, now with ert tests and no external tar!

[Stefan Monnier]

> Well, the list is long:

Just to clarify: the reason why all this has changed without any
discussion is because there was not supposed to be any API.
`package.el' was only meant to provide an interactive UI, pretty much
(plus a few functions, basically the autoloaded ones).

> - "package-archive-contents" and "package-alist" have different
> contents now, because the package descriptors have changed,

Right.  This won't be "fixed".

> - "package-delete" takes a single argument only, but used to take two,

We could definitely "fix this".

> - "package-obsolete-alist" is gone,

Won't be "fixed" either.

> - "package-install" doesn't accept a package name anymore,

It does (again).

> Since most of these changes come from the introducing of
> "package-desc" struct, I know that I am out of luck.

Indeed.

>> What API?
> Well, the package.el API, that is, "package-install",
> "package-delete", "package-alist", and unfortunately a number of
> internal functions ("package--*"), too, because the public API is
> somewhat limited.

Tell us what you need, then, and we can improve it.  As mentioned, what
you think as the public API doesn't even really exist, so the design is
very much open.

> For an end user, it's just a declarative way to specify all packages
> used in her configuration,

I'd like to move in that direction: instead of letting the user say
"install foo" and "uninstall bar", I'd like her to configure the "list
of installed packages" (so adding an element installs it along with any
dependencies, and removing from the list uninstalls the package (if it
was installed) along with any of the dependencies which aren't needed
any more).

> installable with a single shell command, but for a package developer,
> it maintains an isolated, automated and repeatable package environment
> for testing.

As you've discovered, Emacs's development is very messy, so it's no
wonder I've never felt the need for such a controlled testing environment.

> Essentially, it's Bundler, but for Emacs Lisp.

That tells me more about what is Bundler than about what is Carton
(I've never used Ruby or any of its tools) ;-)


        Stefan

Re: cl-defstruct-based package.el, now with ert tests and no external tar!

[Sebastian Wiesner]

2013/6/25 Stefan Monnier <monnier@iro.umontreal.ca>:
>> Well, the list is long:
>
> Just to clarify: the reason why all this has changed without any
> discussion is because there was not supposed to be any API.
> `package.el' was only meant to provide an interactive UI, pretty much
> (plus a few functions, basically the autoloaded ones).

That is pretty unfortunate…

>
>> - "package-archive-contents" and "package-alist" have different
>> contents now, because the package descriptors have changed,
>
> Right.  This won't be "fixed".
>
>> - "package-delete" takes a single argument only, but used to take two,
>
> We could definitely "fix this".

I didn't say you should “fix it”, and I don't think you should
actually.  The “new API” is much better than the old one with two
arguments.  And it's not too hard to handle this situation in Carton:
We'll just test for the presence of a "package-desc" constructor, and
use the new API in this case, or the old one otherwise.

I just said that things would have been easier for us, if we had been
made aware of these changes, e.g. by the NEWS file or some other kind
of announcement.  Had I not followed this list, we would not have
noticed these changes at all, and would probably have reported
pointless bugs for them.

>> - "package-install" doesn't accept a package name anymore,
>
> It does (again).

Fine, thank you :)

>> Since most of these changes come from the introducing of
>> "package-desc" struct, I know that I am out of luck.
>
> Indeed.
>
>>> What API?
>> Well, the package.el API, that is, "package-install",
>> "package-delete", "package-alist", and unfortunately a number of
>> internal functions ("package--*"), too, because the public API is
>> somewhat limited.
>
> Tell us what you need, then, and we can improve it.  As mentioned, what
> you think as the public API doesn't even really exist, so the design is
> very much open.

Well, we need an API to

- install packages by name (e.g. "package-install")
- update the package archive contents (e.g. "package-refresh-contents"),
- and to get a list of upgradable and obsolete packages (uhm, don't
know, we currently call all sorts of internal "package-menu--*"
functions).

Another point on the wishlist would be to control all the output of
package.el, mostly to suppress all sorts of byte-compiler and autoload
generation messages which are meaningless to the user, unless there is
an error.

>> For an end user, it's just a declarative way to specify all packages
>> used in her configuration,
>
> I'd like to move in that direction: instead of letting the user say
> "install foo" and "uninstall bar", I'd like her to configure the "list
> of installed packages" (so adding an element installs it along with any
> dependencies, and removing from the list uninstalls the package (if it
> was installed) along with any of the dependencies which aren't needed
> any more).
>
>> installable with a single shell command, but for a package developer,
>> it maintains an isolated, automated and repeatable package environment
>> for testing.
>
> As you've discovered, Emacs's development is very messy, so it's no
> wonder I've never felt the need for such a controlled testing environment.
>
>> Essentially, it's Bundler, but for Emacs Lisp.
>
> That tells me more about what is Bundler than about what is Carton
> (I've never used Ruby or any of its tools) ;-)

Oh, sorry, I didn't know this, or I would have used some other
analogy.  I just made the experience that people usually don't get
what Carton does until I drop the magical word "bundler" at which
point lights go on and everything's clear :)

What languages do you use?  They might probably have similar tools…

Re: cl-defstruct-based package.el, now with ert tests and no external tar!

[Stefan Monnier]

>> Just to clarify: the reason why all this has changed without any
>> discussion is because there was not supposed to be any API.
>> `package.el' was only meant to provide an interactive UI, pretty much
>> (plus a few functions, basically the autoloaded ones).
> That is pretty unfortunate…

Agreed.  But that's what we have to work with.

>>> - "package-archive-contents" and "package-alist" have different
>>> contents now, because the package descriptors have changed,
>> Right.  This won't be "fixed".
>>> - "package-delete" takes a single argument only, but used to take two,
>> We could definitely "fix this".

> I didn't say you should “fix it”, and I don't think you should
> actually.

I thought so (it might have been worthwhile if it had been the last
incompatibility left).

> I just said that things would have been easier for us, if we had been
> made aware of these changes, e.g. by the NEWS file or some other kind
> of announcement.  Had I not followed this list, we would not have
> noticed these changes at all, and would probably have reported
> pointless bugs for them.

It would have been easier for us if you had announced Carton here (or
on gnu.emacs.sources or some such place), so we'd have known that
someone was using the actual code rather than the UI ;-)

>> Tell us what you need, then, and we can improve it.  As mentioned, what
>> you think as the public API doesn't even really exist, so the design is
>> very much open.

> Well, we need an API to
> - install packages by name (e.g. "package-install")

Done ;-)

> - update the package archive contents
>   (e.g. "package-refresh-contents"),

Done ;-)

> - and to get a list of upgradable and obsolete packages (uhm, don't
> know, we currently call all sorts of internal "package-menu--*"
> functions).

While the previous two sound "clear enough", this one has many
more options.  One possibility is to provide a `package-list' function
which returns all known packages.  Then you can use the (new)
package-desc-dir to know if it's an installed packages or a package from
an archive, and package-desc-status to figure out if it's obsolete (tho
making this function return symbols rather than strings would be
preferable if used like this).

> Another point on the wishlist would be to control all the output of
> package.el, mostly to suppress all sorts of byte-compiler and autoload
> generation messages which are meaningless to the user, unless there is
> an error.

I like for users to see the warnings, since it can give them a sense of
potential problems (e.g. if something is obsolete), and in the absence
of a maintainer (unmaintained packages are pretty much the rules rather
than the exception in Emacsland), it can be very useful: they may not be
able to fix it themselves, but at least they may bring it up so someone
else can fix it.

But having more control over this output (and prettifying it) would
indeed be very welcome.  It is fairly intimidating/overwhelming as
it stands.
Maybe we could try and make it possible to redirect it to a particular
buffer, so you can later on clean it up before displaying it.

> What languages do you use?  They might probably have similar tools…

Agda, Coq, Haskell, C, Elisp?


        Stefan

Re: cl-defstruct-based package.el, now with ert tests and no external tar!

[Sebastian Wiesner]

2013/6/25 Stefan Monnier <monnier@iro.umontreal.ca>:
>> I just said that things would have been easier for us, if we had been
>> made aware of these changes, e.g. by the NEWS file or some other kind
>> of announcement.  Had I not followed this list, we would not have
>> noticed these changes at all, and would probably have reported
>> pointless bugs for them.
>
> It would have been easier for us if you had announced Carton here (or
> on gnu.emacs.sources or some such place), so we'd have known that
> someone was using the actual code rather than the UI ;-)

Well, you know now :)  Seriously, we didn't know that Carton was
getting so popular, and we can hardly announce each and every package
we write on this list, can't we?

>> - and to get a list of upgradable and obsolete packages (uhm, don't
>> know, we currently call all sorts of internal "package-menu--*"
>> functions).
>
> While the previous two sound "clear enough", this one has many
> more options.  One possibility is to provide a `package-list' function
> which returns all known packages.  Then you can use the (new)
> package-desc-dir to know if it's an installed packages or a package from
> an archive, and package-desc-status to figure out if it's obsolete (tho
> making this function return symbols rather than strings would be
> preferable if used like this).

That sound like a good idea.  It's a simple API that would fit all our
needs, and allow for much more cool tricks.

>> Another point on the wishlist would be to control all the output of
>> package.el, mostly to suppress all sorts of byte-compiler and autoload
>> generation messages which are meaningless to the user, unless there is
>> an error.
>
> I like for users to see the warnings, since it can give them a sense of
> potential problems (e.g. if something is obsolete), and in the absence
> of a maintainer (unmaintained packages are pretty much the rules rather
> than the exception in Emacsland), it can be very useful: they may not be
> able to fix it themselves, but at least they may bring it up so someone
> else can fix it.

Carton mostly targets developers for use in their packages.  These
have enough to do with warnings from their own packages, they can't
care for others.  On Github, many developers use it to run tests on
Travis CI.  In this case, the tons of output clutter the build log,
and make it hard to see whats actually going on.

> But having more control over this output (and prettifying it) would
> indeed be very welcome.  It is fairly intimidating/overwhelming as
> it stands.
> Maybe we could try and make it possible to redirect it to a particular
> buffer, so you can later on clean it up before displaying it.

That would be nice.  For now, we are thinking about just spawning
Emacs subprocesses to install packages, and catch and filter the
output of that subprocesses.

>> What languages do you use?  They might probably have similar tools…
>
> Agda, Coq, Haskell, C, Elisp?

Can "cabal" install packages locally?

Re: cl-defstruct-based package.el, now with ert tests and no external tar!

[Daniel Hackney]

Dmitry Gutov <dgutov@yandex.ru> wrote:
> Just an update, I've tried to bring the tests over today, but they're quite
> dependent on the implementation.
>
> I still have 6/10 failures, so far.

That's odd. Make sure you remove "finder-inf.el{,c}" and re-make. Its
format has changed and it needs to be re-generated.

> Daniel:
>
> One thing I definitely didn't like is the usage of raw cl-struct vectors as
> reference values to compare against. These are implementation details of
> cl-defstruct, and we have no business setting them in stone in a test suite
> for code that just uses defstruct.

Good point. The raw vectors should be replaced with calls to
`package-desc-create' with the appropriate arguments.

> Relying on an external program (ginstall-info) is also not nice, for the
> usual alternative-OS-related reasons.

My rationale behind that was to have the multi-file package be in
"source form": all the files in the repo would be actual user-created
source files, and not generated files of any kind. That is probably
not necessary for testing, so the multi-file package directory could
be replaced with a single .tar. This means that all of the
package-building functions (and parts of `with-package-test') could be
removed as well.

-- 
Daniel Hackney

Re: cl-defstruct-based package.el, now with ert tests and no external tar!

[Daniel Hackney]

Stefan Monnier <monnier@iro.umontreal.ca> wrote:
>> Well, the list is long:
>
> Just to clarify: the reason why all this has changed without any
> discussion is because there was not supposed to be any API.
> `package.el' was only meant to provide an interactive UI, pretty much
> (plus a few functions, basically the autoloaded ones).

Part of the motivation for my changes was to give package.el more of
an API which would be usable by developers. There is certainly plenty
more which could be done, like having more of the refresh and
mark-upgradable functionality present in non-UI functions.

>>> What API?
>> Well, the package.el API, that is, "package-install",
>> "package-delete", "package-alist", and unfortunately a number of
>> internal functions ("package--*"), too, because the public API is
>> somewhat limited.

I separated things according to the principle of foo-public and
foo--private, so (theoretically) libraries using package.el should
only have to use package-foo functions and variables and none of
package--bar.

> Tell us what you need, then, and we can improve it.  As mentioned, what
> you think as the public API doesn't even really exist, so the design is
> very much open.

I intended to define a public API which would be solid enough to be
considered stable(ish).

>> Essentially, it's Bundler, but for Emacs Lisp.

I had thought about/proposed something along similar lines when
looking into version-parsing libraries [1], but was told that since
Emacs is not a production environment, it's not worth worrying too
much about exact version compatibility ranges (as in "package foo
depends on package bar >= 1.2.3 and < 1.3.0). Given that MELPA uses
simple date stamps from development versions, fine-grained control
over dependency graphs does not seem to be that important.

But who knows, maybe the presence of better version management will
enable more complex dependencies.

[1] http://www.haxney.org/2010/03/comparing-emacs-version-parsing.html

-- 
Daniel Hackney

Re: cl-defstruct-based package.el, now with ert tests and no external tar!

[Stefan Monnier]

>>> What languages do you use?  They might probably have similar tools…
>> Agda, Coq, Haskell, C, Elisp?
> Can "cabal" install packages locally?

Not sure what that means.  But I use it as little as possible anyway (I
rarely do more than read some byte sequence from a file and write some
other byte-sequence to another file).


        Stefan

Re: cl-defstruct-based package.el, now with ert tests and no external tar!

[Dmitry Gutov]

On 26.06.2013 1:49, Daniel Hackney wrote:
> Dmitry Gutov <dgutov@yandex.ru> wrote:
>> I still have 6/10 failures, so far.
>
> That's odd. Make sure you remove "finder-inf.el{,c}" and re-make. Its
> format has changed and it needs to be re-generated.

I, of course, mean that the problems arise when trying to run your tests 
with the code that is currently in Emacs trunk. Stefan ported the main 
features, but the API is not identical.

Currently at 5/10.

> Good point. The raw vectors should be replaced with calls to
> `package-desc-create' with the appropriate arguments.

Already done. :) Locally.

> That is probably
> not necessary for testing, so the multi-file package directory could
> be replaced with a single .tar. This means that all of the
> package-building functions (and parts of `with-package-test') could be
> removed as well.

Good idea.

Re: cl-defstruct-based package.el, now with ert tests and no external tar!

[Nic Ferrier]

Sebastian Wiesner <lunaryorn@gmail.com> writes:

> 2013/6/25 Stefan Monnier <monnier@iro.umontreal.ca>:

>>> [1]: https://github.com/rejeep/carton
>>
>> I don't understand what this does.  Could you give me some description
>> of what is the benefit of such a tool, either for an end-user or for
>> a developer of a package?
>
> Carton creates isolated and independent package environments.

My own elpakit does much the same thing and is based on parts of
package.el as well.

https://github.com/nicferrier/elpakit


Nic

Re: cl-defstruct-based package.el, now with ert tests and no external tar!

[Dmitry Gutov]

I've checked in the tests, along with some tweaks and previously 
discussed simplifications. I've also taken the liberty to change the 
commentary, because the test suite hasn't eaten my package dir even once 
so far, and let-binding `package-alist' around the body of the big scary 
macro seems to make sure that the list of installed packages is also 
kept intact.

Thanks for your work, even though it hasn't been adopted in its 
entirety. If you feel strongly about some feature that hasn't been taken 
in, it should be easier to make a smaller, targeted patch now.

Comments and suggestions welcome, especially of the "why do the tests 
have to do that thing to make things work?" variety.