Opened 9 years ago
Closed 9 years ago
#12530 closed enhancement (fixed)
Improve the sagecombinat script to support guards for developpers versions of Sage
Reported by:  nthiery  Owned by:  leif 

Priority:  blocker  Milestone:  sage5.0 
Component:  scripts  Keywords:  
Cc:  sagecombinat  Merged in:  sage5.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 sagecombinat script to support guards for developpers versions. It also makes sure to switch to the sagecombinat directory before doing any call to mercurial.
Note: starting from sage 5.0.beta4, the guards used in the sagecombinat queue assume that this updated sagecombinat script is used.
To be applied in <SAGE_ROOT>/local/bin:
Attachments (1)
Change History (15)
comment:1 Changed 9 years ago by
 Reviewers set to Florent Hivert
 Status changed from new to needs_review
comment:2 followup: ↓ 3 Changed 9 years ago by
comment:3 in reply to: ↑ 2 Changed 9 years ago by
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 followup: ↓ 5 Changed 9 years ago by
 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/SageInstall/sage5.0.beta4/local/bin/sagecombinat", line 402, in <module> qselect_backward_compatibility_patches() File "/home/data/SageInstall/sage5.0.beta4/local/bin/sagecombinat", line 303, in qselect_backward_compatibility_patches hg_all_guards())) File "/home/data/SageInstall/sage5.0.beta4/local/bin/sagecombinat", line 294, in is_newversion_guard if cmp_sage_versions(guard, sage_version) > 0: File "/home/data/SageInstall/sage5.0.beta4/local/bin/sagecombinat", line 200, in cmp_sage_versions return cmp(encode_sage_version_for_comparison(version1), encode_sage_version_for_comparison(version2)) File "/home/data/SageInstall/sage5.0.beta4/local/bin/sagecombinat", 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/sage5.0.beta4 $ cd local/bin
Adding a print:
diff git a/sagecombinat b/sagecombinat  a/sagecombinat +++ b/sagecombinat @@ 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_weakrefcachedfunctiondr.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
How adding the following diff to the script ?

sagecombinat
diff git a/sagecombinat b/sagecombinat
a b def encode_sage_version_for_comparison(v 171 171 version = version.replace("alpha", "3.") 172 172 version = version.replace("beta", "2.") 173 173 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) 175 178 176 179 def cmp_sage_versions(version1, version2): 177 180 """
comment:6 followup: ↓ 7 Changed 9 years ago by
 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 ; followup: ↓ 10 Changed 9 years ago by
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 followup: ↓ 9 Changed 9 years ago by
Hi Nicolas,
I'm sorry to report that your patch is failing again: it seems to have worked for me on a already installed sagecombinat for a sage.5.0.beta4
. To check I removed the sagecombinat
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
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 sagecombinat for a
sage.5.0.beta4
. To check I removed thesagecombinat
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
 Summary changed from Improve the sagecombinat script to support guards for developpers versions to Improve the sagecombinat 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?
Changed 9 years ago by
comment:11 Changed 9 years ago by
Here is the diff to implement throwing an error by default, and a warning under f::
%#diff diff git a/sagecombinat b/sagecombinat  a/sagecombinat +++ b/sagecombinat @@ 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
The same, nicer looking:

sagecombinat
diff git a/sagecombinat b/sagecombinat
a b import re 5 5 from optparse import OptionParser 6 6 from sys import stderr 7 7 from subprocess import Popen, PIPE 8 from warnings import warn 8 9 9 10 if "SAGE_ROOT" in os.environ: 10 11 sage = os.environ["SAGE_ROOT"]+"/sage" … … def qselect_backward_compatibility_patch 294 295 if not re.match(version_regexp+"$", guard): 295 296 if re.match(version_regexp, guard): 296 297 # 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) 298 303 return False 299 304 if cmp_sage_versions(guard, sage_version) > 0: 300 305 info("Keep backward compatibility patches for sage "+re.sub("_",".",guard))
comment:13 Changed 9 years ago by
 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 sagecombinat 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
 Merged in set to sage5.0.beta5
 Resolution set to fixed
 Status changed from positive_review to closed
Your
version_regexp
won't match strings like "Sage Version 5.0" (no alpha, beta, or rc). Is that an issue?