#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).