#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: --------------------------+------------------------------------------------- Changes (by rhaas):
* status: review => reopened
Comment:
* this patch contains commented out debugging code. Please remove. * please add an explicit {{{return 0}}} at the end of CompareVersionStrings if the the loop is actually expected to ever end * 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. * please provide patches from git via "git format-patch" rather than "git diff" * this patch does not include a documentation update * the patch removes a space " " from the allowed capabilities names. While I am fine with this, this should be mentioned and stated that this does not cause any problems with currently existing thorns or (better) that other parts of the infrastructure currently would have failed if spaces had been used so that there are guaranteed to be no thorns with spaces in capability names out in the wild. * 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"; } }}} * 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."
I think the patch is mostly fine. There are some minor points where I am asking for clarification and suggesting if possible a different approach. This patch needs to add documentation of this feature to the User guide though before it can be approved.
I notice we are still lacking a category "reviewed and not approved".