bug#48137: 27.2; `package-install-file' fails when loading a package file with DOS line endings

[Ioannis Kappas <ioannis.kappas@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 1463 bytes --]

On Sat, May 29, 2021 at 3:09 PM Eli Zaretskii <eliz@gnu.org> wrote:
>
> > From: Stefan Monnier <monnier@iro.umontreal.ca>
> > Cc: Ioannis Kappas <ioannis.kappas@gmail.com>,  48137@debbugs.gnu.org
> > Date: Sat, 29 May 2021 09:59:01 -0400
> >
> > >> Please find below a patch to read package headers from a temporarily
> > >> decoded buffer, while keeping the literal buffer (which will be used
> > >> to install the package) intact.
> > >
> > > Stefan, any comments?
> >
> > No particular comment from me, except: this is tricky enough that
> > it would benefit from some regression tests.
>
> Thanks.  Yes, that's a good idea.  Ioannis, could you perhaps add some
> tests, both using existing packages and perhaps also some you concoct
> just for testing purposes?

Please find attached a patch with the earlier fix and two new ert
package tests. `package-test-install-file' tests the installation of
single .el file and a .tar archive packages as referenced from the
test harness. `package-test-install-file-EOLs' rewrites the single .el
package multiple time with different EOL conventions ('unix, 'dos and
'mac), installs them and confirms that they are written verbatim
without modifications to the package directory by comparing that the
md5 hashes of the installing package vs the installed package are the
same.

All package tests pass locally both on GNU/Linux and MS-Windows.

Please feel free to scrutinize the patch and/or make modifications.

Thanks

[-- Attachment #2: emacs-bug48137.patch --]
[-- Type: application/octet-stream, Size: 5390 bytes --]

diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
index 5df9b53657..1b8f3304ba 100644
--- a/lisp/emacs-lisp/package.el
+++ b/lisp/emacs-lisp/package.el
@@ -2195,8 +2195,22 @@ package-install-from-buffer
             ((derived-mode-p 'tar-mode)
              (package-tar-file-info))
             (t
-             (save-excursion
-              (package-buffer-info)))))
+             ;; Package headers should be parsed from decoded text
+             ;; (see Bug#48137) where possible.
+             (if (and (eq buffer-file-coding-system 'no-conversion)
+                      buffer-file-name)
+                 (let* ((package-buffer (current-buffer))
+                        (decoding-system
+                         (car (find-operation-coding-system 'insert-file-contents
+                                                            (cons buffer-file-name
+                                                                  package-buffer)))))
+                   (with-temp-buffer
+                     (insert-buffer-substring package-buffer)
+                     (decode-coding-region (point-min) (point-max) decoding-system)
+                     (package-buffer-info)))
+
+               (save-excursion
+                 (package-buffer-info))))))
          (name (package-desc-name pkg-desc)))
     ;; Download and install the dependencies.
     (let* ((requires (package-desc-reqs pkg-desc))
@@ -2222,6 +2236,7 @@ package-install-file
           (setq default-directory file)
           (dired-mode))
       (insert-file-contents-literally file)
+      (set-visited-file-name file)
       (when (string-match "\\.tar\\'" file) (tar-mode)))
     (package-install-from-buffer)))
 
diff --git a/test/lisp/emacs-lisp/package-tests.el b/test/lisp/emacs-lisp/package-tests.el
index 67d647d3b9..2943579955 100644
--- a/test/lisp/emacs-lisp/package-tests.el
+++ b/test/lisp/emacs-lisp/package-tests.el
@@ -263,6 +263,74 @@ package-test-install-single
       (should (file-exists-p autoloads-file))
       (should-not (get-file-buffer autoloads-file)))))
 
+(ert-deftest package-test-install-file ()
+  "Install files with `package-install-file'."
+  (with-package-test (:basedir (ert-resource-directory))
+    (package-initialize)
+    (let* ((pkg-el "simple-single-1.3.el")
+           (source-file (expand-file-name pkg-el (ert-resource-directory))))
+      (should-not (package-installed-p 'simple-single))
+      (package-install-file source-file)
+      (should (package-installed-p 'simple-single))
+      (package-delete (cadr (assq 'simple-single package-alist)))
+      (should-not (package-installed-p 'simple-single)))
+
+    (let* ((pkg-el "multi-file-0.2.3.tar")
+           (source-file (expand-file-name pkg-el (ert-resource-directory))))
+      (package-initialize)
+      (should-not (package-installed-p 'multie-file))
+      (package-install-file source-file)
+      (should (package-installed-p 'multi-file))
+      (package-delete (cadr (assq 'multi-file package-alist))))
+    ))
+
+(ert-deftest package-test-install-file-EOLs ()
+  "Install same file multiple time with `package-install-file'
+but with a different end of line convention (bug#48137)."
+  (with-package-test (:basedir (ert-resource-directory))
+    (package-initialize)
+    (let* ((pkg-el "simple-single-1.3.el")
+           (source-file (expand-file-name pkg-el (ert-resource-directory))))
+
+      (with-temp-buffer
+        (insert-file-contents source-file)
+
+        (let (hashes)
+          (dolist (coding '(unix dos mac) hashes)
+            (let* ((eol-file (expand-file-name pkg-el package-test-user-dir)))
+              ;; save package with this EOL convention.
+              (set-buffer-file-coding-system coding)
+              (write-region (point-min) (point-max) eol-file)
+
+              (should-not (package-installed-p 'simple-single))
+              (package-install-file eol-file)
+              (should (package-installed-p 'simple-single))
+
+              ;; check the package file has been installed unmodified.
+              (let ((eol-hash (with-temp-buffer
+                                (insert-file-contents-literally eol-file)
+                                (buffer-hash))))
+                ;; also perform an additional check that the package
+                ;; file created with this EOL convention is different
+                ;; than all the others created so far.
+                (should-not (member eol-hash hashes))
+                (setq hashes (cons eol-hash hashes))
+
+                (let* ((descr (cadr (assq 'simple-single package-alist)))
+                       (pkg-dir (package-desc-dir descr))
+                       (dest-file (expand-file-name "simple-single.el" pkg-dir ))
+                       (dest-hash (with-temp-buffer
+                                    (insert-file-contents-literally dest-file)
+                                    (buffer-hash))))
+
+                  (should (string= dest-hash eol-hash))))
+
+              (package-delete (cadr (assq 'simple-single package-alist)))
+              (should-not (package-installed-p 'simple-single))
+              (delete-file eol-file)
+              (should-not (file-exists-p eol-file))
+              )))))))
+
 (ert-deftest package-test-install-dependency ()
   "Install a package which includes a dependency."
   (with-package-test ()