Opened 9 years ago

Closed 9 years ago

#12530 closed enhancement (fixed)

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

Reported by: nthiery Owned by: leif
Priority: blocker Milestone: sage-5.0
Component: scripts Keywords:
Cc: sage-combinat Merged in: sage-5.0.beta5
Authors: Nicolas M. Thiéry Reviewers: Florent Hivert
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description

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:

Attachments (1)

trac_12530-sage_combinat_script-guards-nt.patch (8.7 KB) - added by nthiery 9 years ago.

Download all attachments as: .zip

Change History (15)

comment:1 Changed 9 years ago by nthiery

  • Reviewers set to Florent Hivert
  • Status changed from new to needs_review

comment:2 follow-up: Changed 9 years ago by jhpalmieri

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

comment:3 in reply to: ↑ 2 Changed 9 years ago by 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!

comment:4 follow-up: Changed 9 years ago by hivert

  • Status changed from needs_review to needs_work

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 ?

comment:5 in reply to: ↑ 4 Changed 9 years ago by hivert

How adding the following diff to the script ?

  • sage-combinat

    diff --git a/sage-combinat b/sage-combinat
    a b def encode_sage_version_for_comparison(v 
    171171    version = version.replace("alpha", "-3.")
    172172    version = version.replace("beta", "-2.")
    173173    version = version.replace("rc", "-1.")
    174     return [int(s) for s in re.split("\.|_", version)]+[0]
     174    try:
     175        res = [int(s) for s in re.split("\.|_", version)]+[0]
     176    except ValueError:
     177        error("Invalid guard in the mercurial queue: %s"%version)
    175178
    176179def cmp_sage_versions(version1, version2):
    177180    """

comment:6 follow-up: Changed 9 years ago by nthiery

  • Status changed from needs_work to needs_review

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.

comment:7 in reply to: ↑ 6 ; follow-up: Changed 9 years ago by 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 ?

comment:8 follow-up: Changed 9 years ago by 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

comment:9 in reply to: ↑ 8 Changed 9 years ago by 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.

comment:10 in reply to: ↑ 7 Changed 9 years ago by nthiery

  • Summary changed from Improve the sage-combinat script to support guards for developpers versions to Improve the sage-combinat script to support guards for developpers versions of Sage

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?

comment:11 Changed 9 years ago by 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))

comment:12 Changed 9 years ago by nthiery

The same, nicer looking:

  • sage-combinat

    diff --git a/sage-combinat b/sage-combinat
    a b import re 
    55from optparse import OptionParser
    66from sys import stderr
    77from subprocess import Popen, PIPE
     8from warnings import warn
    89
    910if "SAGE_ROOT" in os.environ:
    1011    sage = os.environ["SAGE_ROOT"]+"/sage"
    def qselect_backward_compatibility_patch 
    294295        if not re.match(version_regexp+"$", guard):
    295296            if  re.match(version_regexp, guard):
    296297                # Catch erroneous guards like 4_3_3:
    297                 error("Invalid version guard in the mercurial queue: %s"%guard)
     298                message = "Invalid version guard in the mercurial queue: %s"%guard
     299                if options.force:
     300                    warn(message)
     301                else:
     302                    error(message)
    298303            return False
    299304        if cmp_sage_versions(guard, sage_version) > 0:
    300305            info("Keep backward compatibility patches for sage "+re.sub("_",".",guard))

comment:13 Changed 9 years ago by hivert

  • Priority changed from major to blocker
  • Status changed from needs_review to positive_review

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

comment:14 Changed 9 years ago by jdemeyer

  • Merged in set to sage-5.0.beta5
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.