Improve the sage-combinat script to support guards for developpers versions of Sage

[nthiery]

The attached patch improves the sage-combinat script to support guards for developpers versions. It also makes sure to switch to the sage-combinat directory before doing any call to mercurial.

Note: starting from sage 5.0.beta4, the guards used in the sage-combinat queue assume that this updated sage-combinat script is used.

To be applied in <SAGE_ROOT>/local/bin:

[jhpalmieri]

Your version_regexp won't match strings like "Sage Version 5.0" (no alpha, beta, or rc). Is that an issue?

[nthiery]

Replying to jhpalmieri:

Your version_regexp won't match strings like "Sage Version 5.0" (no alpha, beta, or rc). Is that an issue?

Good catch; I forgot to copy paste the latest version of the regexp in the code. Fixed in the new patch. Thanks!

[hivert]

Hi Nicolas,

I tried this patch after installing a sage_5.0.beta4 and if broke with the following error message. I'm investigating::

[...]
Skip backward compatibility patches for sage 4.3.3
Traceback (most recent call last):
  File "/home/data/Sage-Install/sage-5.0.beta4/local/bin/sage-combinat", line 402, in <module>
    qselect_backward_compatibility_patches()
  File "/home/data/Sage-Install/sage-5.0.beta4/local/bin/sage-combinat", line 303, in qselect_backward_compatibility_patches
    hg_all_guards()))
  File "/home/data/Sage-Install/sage-5.0.beta4/local/bin/sage-combinat", line 294, in is_newversion_guard
    if cmp_sage_versions(guard, sage_version) > 0:
  File "/home/data/Sage-Install/sage-5.0.beta4/local/bin/sage-combinat", line 200, in cmp_sage_versions
    return cmp(encode_sage_version_for_comparison(version1), encode_sage_version_for_comparison(version2))
  File "/home/data/Sage-Install/sage-5.0.beta4/local/bin/sage-combinat", line 174, in encode_sage_version_for_comparison
    return [int(s) for s in re.split("\.|_", version)]+[0]
ValueError: invalid literal for int() with base 10: '3:'
popcorn-*e/sage-5.0.beta4 $ cd local/bin  

Adding a print:

diff --git a/sage-combinat b/sage-combinat
--- a/sage-combinat
+++ b/sage-combinat
@@ -168,6 +168,7 @@ def encode_sage_version_for_comparison(v
         sage: e("4.7.2.beta10") < e("4.7.2.rc0")
         True
     """
+    print "DEBUG:::: %s"%version
     version = version.replace("alpha", "-3.")
     version = version.replace("beta", "-2.")
     version = version.replace("rc", "-1.")

Shows

Skip backward compatibility patches for sage 4.3.2
DEBUG:::: 4_3_3
DEBUG:::: 5.0.beta4
Skip backward compatibility patches for sage 4.3.3
DEBUG:::: 4_3_3:
Traceback (most recent call last):
[...]
ValueError: invalid literal for int() with base 10: '3:'

The problem is in the queue: the following guard is invalid

trac_6520_weakref-cached-function-dr.patch    #+4_3_3: needs rebase

I think we should throw an error in this case. What do you think ?

[hivert]

How adding the following diff to the script ?

sage-combinat

@@ -171 +171 @@
     version = version.replace("alpha", "-3.")
     version = version.replace("beta", "-2.")
     version = version.replace("rc", "-1.")
-    return [int(s) for s in re.split("\.|_", version)]+[0]
+    try:
+        res = [int(s) for s in re.split("\.|_", version)]+[0]
+    except ValueError:
+        error("Invalid guard in the mercurial queue: %s"%version)
 
 def cmp_sage_versions(version1, version2):
     """

[nthiery]

Oops, the guard should not have matched the regexp in the first place; I had forgotten the "$" at the end of it for an exact match. The updated patch still complains as you suggest, since this is likely to be a typicall typo.

I used the occasion to run pyflakes and clean up the imports.

[hivert]

Replying to nthiery:

Oops, the guard should not have matched the regexp in the first place; I had forgotten the "$" at the end of it for an exact match. The updated patch still complains as you suggest, since this is likely to be a typicall typo.

Should this be a warning or an error ? I think I'd rather have an error than a warning. The reason is that the warning is issued in the middle of a lot of info message and is likely to go unnoticed. Why do you prefer warning ?

[hivert]

Hi Nicolas,

I'm sorry to report that your patch is failing again: it seems to have worked for me on a already installed sage-combinat for a sage.5.0.beta4. To check I removed the sage-combinat directory and now:

Skip backward compatibility patches for sage 4.8.alpha0
Keep backward compatibility patches for sage 5.0
Keep backward compatibility patches for sage 5.0
Skip backward compatibility patches for sage 5.0.beta0
Skip backward compatibility patches for sage 5.0.beta2
Skip backward compatibility patches for sage 5.0.beta4

[nthiery]

Replying to hivert:

I'm sorry to report that your patch is failing again: it seems to have worked for me on a already installed sage-combinat for a sage.5.0.beta4. To check I removed the sage-combinat directory and now:

Skip backward compatibility patches for sage 4.8.alpha0
Keep backward compatibility patches for sage 5.0
Keep backward compatibility patches for sage 5.0
Skip backward compatibility patches for sage 5.0.beta0
Skip backward compatibility patches for sage 5.0.beta2
Skip backward compatibility patches for sage 5.0.beta4

It's actually the queue that is wrong :-) The release of 5.0 comes after that of 5.0.beta4, so it's normal to apply a patch marked 5.0 on sage 5.0.beta4. I'll go check the queue now.

[nthiery]

Replying to hivert:

Replying to nthiery:

Oops, the guard should not have matched the regexp in the first place; I had forgotten the "$" at the end of it for an exact match. The updated patch still complains as you suggest, since this is likely to be a typicall typo.

Should this be a warning or an error ? I think I'd rather have an error than a warning. The reason is that the warning is issued in the middle of a lot of info message and is likely to go unnoticed. Why do you prefer warning ?

I pondered about this too. My rationale was the following: imagine a beginner installs the queue and runs into this issue; if the queue is really screwed up, their will be an error later one anyway. Otherwise, he could be lucky and have the queue apply reasonnably and be able to get to work; so why blocking him.

That being said, it is indeed buried in the middle of a bunch of info, though quite visible for someone scrolling up.

So I don't know. Maybe that would be a use case for throwing an error by default, and a warning under -f. What do you think?

[nthiery]

Here is the diff to implement throwing an error by default, and a warning under -f::

%#diff
diff --git a/sage-combinat b/sage-combinat
--- a/sage-combinat
+++ b/sage-combinat
@@ -5,6 +5,7 @@ import re
 from optparse import OptionParser
 from sys import stderr
 from subprocess import Popen, PIPE
+from warnings import warn
 
 if "SAGE_ROOT" in os.environ:
     sage = os.environ["SAGE_ROOT"]+"/sage"
@@ -294,7 +295,11 @@ def qselect_backward_compatibility_patch
         if not re.match(version_regexp+"$", guard):
             if  re.match(version_regexp, guard):
                 # Catch erroneous guards like 4_3_3:
-                error("Invalid version guard in the mercurial queue: %s"%guard)
+                message = "Invalid version guard in the mercurial queue: %s"%guard
+                if options.force:
+                    warn(message)
+                else:
+                    error(message)
             return False
         if cmp_sage_versions(guard, sage_version) > 0:
             info("Keep backward compatibility patches for sage "+re.sub("_",".",guard))

[nthiery]

The same, nicer looking:

sage-combinat

@@ -5 +5 @@
 from optparse import OptionParser
 from sys import stderr
 from subprocess import Popen, PIPE
+from warnings import warn
 
 if "SAGE_ROOT" in os.environ:
     sage = os.environ["SAGE_ROOT"]+"/sage"
@@ -294 +295 @@
         if not re.match(version_regexp+"$", guard):
             if  re.match(version_regexp, guard):
                 # Catch erroneous guards like 4_3_3:
-                error("Invalid version guard in the mercurial queue: %s"%guard)
+                message = "Invalid version guard in the mercurial queue: %s"%guard
+                if options.force:
+                    warn(message)
+                else:
+                    error(message)
             return False
         if cmp_sage_versions(guard, sage_version) > 0:
             info("Keep backward compatibility patches for sage "+re.sub("_",".",guard))

[hivert]

The patch is working as expected (at least for me) and is needed for the sage-combinat queue to applied on the various 5.0 sage. Therefore I put a positive review and set it as a blocker.

Florent