bug#42237: [PATCH] Clean up Tramp version check

bug#42237: [PATCH] Clean up Tramp version check

[Nicholas Drozd]

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

I actually saw the "not fit for" error message in the wild. The source
looked a little strange, so I cleaned it up. This shouldn't change any
behavior.

[-- Attachment #2: 0001-Clean-up-Tramp-version-check.patch --]
[-- Type: text/x-patch, Size: 2411 bytes --]

From 37169e549094ea426d6429f088515e5fc416785c Mon Sep 17 00:00:00 2001
From: Nick Drozd <nicholasdrozd@gmail.com>
Date: Sat, 4 Jul 2020 11:42:49 -0500
Subject: [PATCH] Clean up Tramp version check

* lisp/net/trampver.el
(tramp-minimum-emacs-version, tramp-check-version): New constant, new function
* test/lisp/net/tramp-tests.el
(tramp-test46-version): New test
---
 lisp/net/trampver.el         | 16 ++++++++++------
 test/lisp/net/tramp-tests.el |  8 ++++++++
 2 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/lisp/net/trampver.el b/lisp/net/trampver.el
index 8d21133b3b..42d984dae3 100644
--- a/lisp/net/trampver.el
+++ b/lisp/net/trampver.el
@@ -69,12 +69,16 @@ tramp-repository-version
 	   (emacs-repository-get-version dir))))
   "The repository revision of the Tramp sources.")
 
-;; Check for Emacs version.
-(let ((x   (if (not (string-lessp emacs-version "25.1"))
-      "ok"
-    (format "Tramp 2.5.0-pre is not fit for %s"
-            (replace-regexp-in-string "\n" "" (emacs-version))))))
-  (unless (string-equal "ok" x) (error "%s" x)))
+(defconst tramp-minimum-emacs-version "25.1"
+  "The minimum Emacs version required by Tramp.")
+
+(defun tramp-check-version ()
+  "Error if `emacs-version' is less than `tramp-minimum-emacs-version'."
+  (when (string-lessp emacs-version tramp-minimum-emacs-version)
+    (error "Tramp 2.5.0-pre is not fit for %s"
+           (replace-regexp-in-string "\n" "" (emacs-version)))))
+
+(tramp-check-version)
 
 ;; Tramp versions integrated into Emacs.  If a user option declares a
 ;; `:package-version' which doesn't belong to an integrated Tramp
diff --git a/test/lisp/net/tramp-tests.el b/test/lisp/net/tramp-tests.el
index 1f24ba2786..ff112e9c82 100644
--- a/test/lisp/net/tramp-tests.el
+++ b/test/lisp/net/tramp-tests.el
@@ -6557,6 +6557,14 @@ tramp-test45-unload
 	  (ignore-errors (all-completions "tramp" (symbol-value x)))
 	  (ert-fail (format "Hook `%s' still contains Tramp function" x))))))
 
+(ert-deftest tramp-test46-version ()
+  "Check that the version check checks out."
+  (tramp-check-version)
+  (let ((emacs-version "24.3"))
+    (should-error (tramp-check-version)))
+  (let ((tramp-minimum-emacs-version "30.1"))
+    (should-error (tramp-check-version))))
+
 (defun tramp-test-all (&optional interactive)
   "Run all tests for \\[tramp].
 If INTERACTIVE is non-nil, the tests are run interactively."
-- 
2.17.1

bug#42237: [PATCH] Clean up Tramp version check

[Michael Albinus]

Nicholas Drozd <nicholasdrozd@gmail.com> writes:

Hi Nicholas,

Thanks for working on Tramp.

> I actually saw the "not fit for" error message in the wild. The source
> looked a little strange, so I cleaned it up. This shouldn't change any
> behavior.

Well, this code snippet has its history. Initially, it was (and still
is) used in the Tramp git repository, file aclocal.m4. And that code
snippet is also propagated to trampver.el via template trampver.el.in.

If you want to improve the code, please change only these two files. The
Tramp git repository is located at
<git://git.savannah.gnu.org/tramp.git> (see also the Tramp manual).

> +(ert-deftest tramp-test46-version ()
> +  "Check that the version check checks out."
> +  (tramp-check-version)
> +  (let ((emacs-version "24.3"))
> +    (should-error (tramp-check-version)))
> +  (let ((tramp-minimum-emacs-version "30.1"))
> +    (should-error (tramp-check-version))))

This is a nice extension :-)

bug#42237: [PATCH] Clean up Tramp version check

[Michael Albinus]

Nicholas Drozd <nicholasdrozd@gmail.com> writes:

Hi Nicholas,

> This turned out to be quite a piece of code indeed. I think this
> change respects all the existing requirements. As I said before, this
> shouldn't change any behavior, apart from adding the new function and
> variable. Feel free to reject.

Thanks. However,

> +  (let ((tramp-minimum-emacs-version "30.1"))
> +    (should-error (tramp-check-version))))

this does not work as expected. What is this check good for? I
understand your intention, but do we need to test this case?

Best regards, Michael.

bug#42237: [PATCH] Clean up Tramp version check

[Nicholas Drozd]

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

> Thanks. However,
>
> > +  (let ((tramp-minimum-emacs-version "30.1"))
> > +    (should-error (tramp-check-version))))
>
> this does not work as expected. What is this check good for? I
> understand your intention, but do we need to test this case?

That test has already justified its existence, for it has revealed a
flaw in the implementation! TRAMP_EMACS_REQUIRED_VERSION was getting
substituted in two separate places, so tramp-check-version wasn't
checking against tramp-minimum-emacs-version, but rather a hardcoded
value. I've attached a new version of the patch. Hopefully this one is
right.

[-- Attachment #2: 0001-Clean-up-Tramp-version-check.patch --]
[-- Type: text/x-patch, Size: 3062 bytes --]

From 1b47fd85fef2590a7da252d0a2f5d9dcc3ace81c Mon Sep 17 00:00:00 2001
From: Nick Drozd <nicholasdrozd@gmail.com>
Date: Sat, 11 Jul 2020 10:02:27 -0500
Subject: [PATCH] Clean up Tramp version check

* aclocal.m4
(TRAMP_EMACS_VERSION_CHECK): Rewrite
* trampver.el.in
(tramp-minimum-emacs-version): New variable
(tramp-check-version): New function
(tramp-tests.el): New test
---
 aclocal.m4          | 17 +++++++++++++----
 lisp/trampver.el.in |  4 +---
 test/tramp-tests.el |  8 ++++++++
 3 files changed, 22 insertions(+), 7 deletions(-)

diff --git a/aclocal.m4 b/aclocal.m4
index d4a82cf9..4940fbef 100644
--- a/aclocal.m4
+++ b/aclocal.m4
@@ -75,10 +75,19 @@ AC_DEFUN(AC_EMACS_INFO, [
   AC_SUBST(TRAMP_EMACS_REQUIRED_VERSION)
   dnl Starting with Emacs 26.1, we could use `string-version-lessp'.
   TRAMP_EMACS_VERSION_CHECK="\
-  (if (not (string-lessp emacs-version \"${TRAMP_EMACS_REQUIRED_VERSION}\"))
-      \"ok\"
-    (format \"${PACKAGE_STRING} is not fit for %s\"
-            (replace-regexp-in-string \"\\n\" \"\" (emacs-version))))"
+(progn  ; <-- don't touch this progn
+  (defconst tramp-minimum-emacs-version \"${TRAMP_EMACS_REQUIRED_VERSION}\"
+    \"The minimum Emacs version required by Tramp.\")
+
+  (defun tramp-check-version ()
+    \"Error if `emacs-version' is less than `tramp-minimum-emacs-version'.\"
+    (progn  ; <-- don't touch this progn
+      (when (string-lessp emacs-version tramp-minimum-emacs-version)
+        (error \"${PACKAGE_VERSION} is not fit for %s\"
+               (replace-regexp-in-string \"\n\" \"\" (emacs-version))))
+      \"ok\"))
+
+  (tramp-check-version))"
   AC_SUBST(TRAMP_EMACS_VERSION_CHECK)
 
   AC_MSG_CHECKING([for $EMACS version])
diff --git a/lisp/trampver.el.in b/lisp/trampver.el.in
index f487b332..04dd4006 100644
--- a/lisp/trampver.el.in
+++ b/lisp/trampver.el.in
@@ -69,9 +69,7 @@
 	   (emacs-repository-get-version dir))))
   "The repository revision of the Tramp sources.")
 
-;; Check for Emacs version.
-(let ((x @TRAMP_EMACS_VERSION_CHECK@))
-  (unless (string-equal "ok" x) (error "%s" x)))
+@TRAMP_EMACS_VERSION_CHECK@
 
 ;; Tramp versions integrated into Emacs.  If a user option declares a
 ;; `:package-version' which doesn't belong to an integrated Tramp
diff --git a/test/tramp-tests.el b/test/tramp-tests.el
index 51722210..f0f8d079 100644
--- a/test/tramp-tests.el
+++ b/test/tramp-tests.el
@@ -6670,6 +6670,14 @@ Since it unloads Tramp, it shall be the last test to run."
 	  (ignore-errors (all-completions "tramp" (symbol-value x)))
 	  (ert-fail (format "Hook `%s' still contains Tramp function" x))))))
 
+(ert-deftest tramp-test47-version ()
+  "Check that the version check checks out."
+  (should (string-equal "ok" (tramp-check-version)))
+  (let ((emacs-version "20.3"))
+    (should-error (tramp-check-version)))
+  (let ((tramp-minimum-emacs-version "40.1"))
+    (should-error (tramp-check-version))))
+
 (defun tramp-test-all (&optional interactive)
   "Run all tests for \\[tramp].
 If INTERACTIVE is non-nil, the tests are run interactively."
-- 
2.17.1

bug#42237: [PATCH] Clean up Tramp version check

[Michael Albinus]

Nicholas Drozd <nicholasdrozd@gmail.com> writes:

Hi Nicholas,

> I've attached a new version of the patch. Hopefully this one is right.

Still problems:

--8<---------------cut here---------------start------------->8---
# ./configure
configure: Tramp 2.5.0-pre
checking for gmake... no
checking for make... make
checking for reasonable make version... ok
checking whether make sets $(MAKE)... yes
checking for emacs... /usr/local/bin/emacs
./configure: command substitution: line 1880: unexpected EOF while looking for matching `''
./configure: command substitution: line 1881: syntax error: unexpected end of file
checking for /usr/local/bin/emacs version... ok
...
--8<---------------cut here---------------end--------------->8---

And this:

--8<---------------cut here---------------start------------->8---
# ./configure --with-emacs=~/src/emacs-25/src/emacs
configure: Tramp 2.5.0-pre
checking for gmake... no
checking for make... make
checking for reasonable make version... ok
checking whether make sets $(MAKE)... yes
checking for ~/src/emacs-25/src/emacs... no
configure: error: no not found
--8<---------------cut here---------------end--------------->8---

Best regards, Michael.

bug#42237: [PATCH] Clean up Tramp version check

[Michael Albinus]

Michael Albinus <michael.albinus@gmx.de> writes:

Hi Nicholas,

> # ./configure --with-emacs=~/src/emacs-25/src/emacs
> configure: Tramp 2.5.0-pre
> checking for gmake... no
> checking for make... make
> checking for reasonable make version... ok
> checking whether make sets $(MAKE)... yes
> checking for ~/src/emacs-25/src/emacs... no
> configure: error: no not found

Sorry, this wasn't caused by your patch. I've fixed this error.

Best regards, Michael.

bug#42237: [PATCH] Clean up Tramp version check

[Stefan Kangas]

Nicholas Drozd <nicholasdrozd@gmail.com> writes:

> I've attached a new version of the patch. Hopefully this one is right.

Should the patch here be installed?  Michael mentioned an issue that
then turned out to be unrelated.

Best regards,
Stefan Kangas

bug#42237: [PATCH] Clean up Tramp version check

[Michael Albinus]

Stefan Kangas <stefan@marxist.se> writes:

Hi Stefan,

> Nicholas Drozd <nicholasdrozd@gmail.com> writes:
>
>> I've attached a new version of the patch. Hopefully this one is right.
>
> Should the patch here be installed?  Michael mentioned an issue that
> then turned out to be unrelated.

This patch is intended for the *Tramp* git repo. No need to do smething
for Emacs (yet).

> Best regards,
> Stefan Kangas

Best regards, Michael.

bug#42237: [PATCH] Clean up Tramp version check

[Michael Albinus]

Hi Nicholas,

Are you still working on this?

Best regards, Michael.

bug#42237: [PATCH] Clean up Tramp version check

[Nicholas Drozd]

Hi Michael, sorry about the delay. I swear I had this working before,
but now the new test isn't passing. Because this is just a clean-up
change that doesn't actually fix anything, I don't want to put any
more effort into it. Feel free to try and salvage the patch or to
trash it.

On Sun, Oct 18, 2020 at 10:13 AM Michael Albinus <michael.albinus@gmx.de> wrote:
>
> Hi Nicholas,
>
> Are you still working on this?
>
> Best regards, Michael.

bug#42237: [PATCH] Clean up Tramp version check

[Michael Albinus]

Nicholas Drozd <nicholasdrozd@gmail.com> writes:

> Hi Michael,

Hi Nicholas,

> sorry about the delay. I swear I had this working before,
> but now the new test isn't passing. Because this is just a clean-up
> change that doesn't actually fix anything, I don't want to put any
> more effort into it. Feel free to try and salvage the patch or to
> trash it.

Thanks for the feedback, I'm closing the bug. Fell free to come back if
you want to continue.

Best regards, Michael.