#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: new Priority: minor | Milestone: Component: Cactus | Version: Resolution: | Keywords: --------------------------+-------------------------------------------------

Comment (by knarf):

That is where I had the text about the version strings from. :) I don't know if we could actually reuse the code though. And we don't need most of the possible dependencies in Debian, so it would be a stripped-down version.

#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: new Priority: minor | Milestone: Component: Cactus | Version: Resolution: | Keywords: --------------------------+-------------------------------------------------

Comment (by hinder):

I don't like version numbers because they impose a linear structure, and we sometimes have different branches (both explicitly in the repository, and implicitly with locally applied patches).

What about saying that thorns provide certain "features", and other thorns require these features? We might even consider re-using the capabilities mechanism for this, though it is a low-level technical detail and maybe best not exposed to the user. For example, for the recent READS/WRITES changes to the flesh, we could say that the flesh now supports READS_WRITES, and thorns could declare that they need READS_WRITES. If the flesh does not support it, the user gets an error message saying that they need a (probably newer) version of the flesh which supports READS_WRITES.

This is relevant now because if we merge some of the development branches of Kranc into master and regenerate McLachlan, a very recent flesh will be needed for the READS/WRITES support.

Do we want to depend on features/versions of "implementations" or of "capabilities"?

Rather than adding new capabilities, maybe we could make these parameters of the capability. For example, we could depend on Cactus{READS_WRITES,...}. Maybe every so often we could bundle these capabilities up into a version number, perhaps corresponding to a release.

#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: new Priority: minor | Milestone: Component: Cactus | Version: Resolution: | Keywords: --------------------------+-------------------------------------------------

Comment (by knarf):

Using capabilities for this sounds a good idea, but we would need to specify that a thorn depends on a specific implementation providing some capability - otherwise we couldn't produce a good error message in case it is missing. Having the implementation name and the name of the required capability should be enough to output "Need another (probably newer) version of implementation X (provided by thorn Y) for thorn Z".

One advantage of this over #error for example is that it would catch problems in the CST stage, not at compile time when the developer is already off for a coffee, and the error itself can be buried in a lot of output due to a parallel build.

#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: review Priority: minor | Milestone: Component: Cactus | Version: Resolution: | Keywords: --------------------------+------------------------------------------------- Changes (by knarf):

* status: new => review

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

#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: review Priority: minor | Milestone: ET_2016_05 Component: Cactus | Version: Resolution: | Keywords: --------------------------+------------------------------------------------- Changes (by knarf):

* status: reopened => review

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

#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: review Priority: minor | Milestone: ET_2016_05 Component: Cactus | Version: Resolution: | Keywords: --------------------------+-------------------------------------------------

Comment (by knarf):

pull request: https://bitbucket.org/cactuscode/cactus/pull-requests/19 /introduce-versions-for-capabilities/diff

#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: review Priority: minor | Milestone: ET_2016_05 Component: Cactus | Version: Resolution: | Keywords: --------------------------+-------------------------------------------------

Comment (by hinder):

In the version number comparison, is 1.8.10 considered to be newer or older than 1.8.9?

#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: review Priority: minor | Milestone: ET_2016_05 Component: Cactus | Version: Resolution: | Keywords: --------------------------+-------------------------------------------------

Comment (by knarf):

Replying to [comment:17 rhaas]:

I am still somewhat unhappy with the non-explicit loop termination that

always exists the loop through an return in the loop body. I added a version that (I personally, others may well differ) find more explicit and intuitive (at least the loop termination condition, if one want to have just shorter code then one can get rid of four more lines by assigning the result of the regex to a list).

Your versions looks fine to me, too. Nicer, in fact.

As far as whether I prefer that version numbers should be required to

start with a numerical or a non-numerical prefix, I'd say "neither should be required" ie. both OpenMPI's {{{v2.x-dev-411-gb0ff2a1}}} as well as {{{1.8.15-patch1}}} should be supported (and I think CompareVersion supports all of these).

We could allow any start, but (using your example) an explicit 'v' in front of the version only begs for misunderstandings (is the version 2.x, or v2.x?).

Minor issue: I would rename the CheckVersionStrings procedure to just

CheckVersions to avoid confusion whether the procedure checks versions of capabilities or verifies that a version string is valid.

What about CheckVersionFulfilled?

Documentation and comments need to be made consistent with what the code

actually does before this is applied but otherwise I have no objection to this. Is there also support for CONFLICTS?

Sure.

About the Perl-idioms: Some people like them, some don't. I find they interrupt my code-reading whenever I encounter them. But whatever.

#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: review Priority: minor | Milestone: ET_2016_05 Component: Cactus | Version: Resolution: | Keywords: --------------------------+-------------------------------------------------

Comment (by knarf):

The pull request has been updated with a documentation update. Please review.

#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

#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: reviewed_ok Priority: minor | Milestone: ET_2016_05 Component: Cactus | Version: Resolution: | Keywords: --------------------------+------------------------------------------------- Changes (by rhaas):

* status: review => reviewed_ok