[ET Trac] [Einstein Toolkit] #241: Thorns should be able to specify implementation versions, and other thorns should be able to depend on those

[Einstein Toolkit]

#241: Thorns should be able to specify implementation versions, and other thorns
should be able to depend on those
--------------------------+-------------------------------------------------
  Reporter:  knarf        |       Owner:            
      Type:  enhancement  |      Status:  reopened  
  Priority:  minor        |   Milestone:  ET_2016_05
 Component:  Cactus       |     Version:            
Resolution:               |    Keywords:            
--------------------------+-------------------------------------------------

Comment (by knarf):

 Thanks for your review.

 Replying to [comment:10 rhaas]:
 > * this patch contains commented out debugging code. Please remove.

 This code was already present. All I did is extend it a little to also
 print the version. We can remove it as separate commit later, but I
 actually found it quite useful.

 > * please add an explicit {{{return 0}}} at the end of
 CompareVersionStrings if the the loop is actually expected to ever end

 The loop is expected to end, by returning from within. A 'return'
 statement at the end would never be reached.

 > * CompareVersionStrings contains an infinite loop if none of the if
 statements in it trigger (ie would would seem to happen if the two version
 strings are exactly identical?). If there is not infinite loop, it is
 confusing logic and requires are comment.

 I added a comment.

 > * please provide patches from git via "git format-patch" rather than
 "git diff"

 That would require me committing first. Of course I could do that and hope
 nobody messes with that file in the meantime to avoid git-conflict hell
 for me. :)

 > * this patch does not include a documentation update

 It doesn't. I first would like this to be approved before I add
 documentation, or I likely have to change both.

 > * the patch removes a space " " from the allowed capabilities names.

 Capability names already are not allowed to contain spaces.

 > * rather than the "last_req" logic I would try and use perl's pattern
 matching capabilities like this
 > {{{
 > $a = "a b c (1.0) d (2.0) e f";
 > while($a =~ m/ *([a-z]+) +(\([^)]+\))?/g) {
 >   $cap = $1;
 >   $ver = ($2 or "all versions");
 >   print "cap: $cap in version $ver\n";
 > }
 > }}}

 Neat. I use this now.

 > * the version number that is parsed by ParseProvideBlock seems to be
 required to start with a number. This is strange and different from what
 CompareVersionStrings help text implies "Compares two version strings:
 first non-numeric prefix lexically, next numeric prefix of remainder
 numerically, and so on."

 It does require this indeed, and indeed, CompareVersionStrings starts with
 non-numeric prefixes. We could change that either way; which do you
 suggest? It's not a bug, just an oddity.

 > I notice we are still lacking a category "reviewed and not approved".

 re-opening is fine in that case, or "won't" fix for a more final message.

 Replying to [comment:11 eschnett]:
 > Does this handle version numbers such as {1.8.15-patch1} or {1.0.1k} or
 {8c} or {0.9-rc2}? OpenMPI's nightly snapshots are even more crazy and
 look like {v2.x-dev-411-gb0ff2a1}.

 Given the way it is described above and the way I implemented it, all of
 the ones you mention should work (in the way I would intend to use them).
 That is not to say that someone couldn't come up with some scheme that
 wouldn't, but then that's always the case (say, someone could add the
 literal name of the month of a build in a version or such).

-- 
Ticket URL: <https://trac.einsteintoolkit.org/ticket/241#comment:14>
Einstein Toolkit <http://einsteintoolkit.org>
The Einstein Toolkit