Thorns should be able to specify implementation versions, and other thorns should be able to depend on those

Issue #241 closed
Frank Löffler created an issue

Cactus thorns implement 'implementations', and other thorns can then depend on particular implementations being present. However, quite often those are updated, and incompatibilities occur. Usually this is resolved by the developer in both thorns - the implementing and the using thorn. However, a user might just update one of them, and Cactus would not catch this error. Thus, I suggest to think about, and implement how to:

  • specify implementation versions e.g. IMPLEMENTS: Cactus (4.0) The version string within () would have be sorted in an intelligent way to allow comparisons [1].

  • specify dependencies on particular versions of other implementations In particular I suggest the following dependencies: depends, and conflicts, both with comparisons < <= == >= > != e.g. DEPENDS: Cactus (>= 4.0) DEPENDS: BadImplementation (!=3.14) <-- this introduces a dependency CONFLICTS: BadImplementation (==3.14) <-- this doesn't introduce a depencency

[1] First the initial part of each string consisting entirely of non-digit characters is determined. These two parts (one of which may be empty) are compared lexically. If a difference is found it is returned. The lexical comparison is a comparison of ASCII values modified so that all the letters sort earlier than all the non-letters. Then the initial part of the remainder of each string which consists entirely of digit characters is determined. The numerical values of these two parts are compared, and any difference found is returned as the result of the comparison. For these purposes an empty string (which can only occur at the end of one or both version strings being compared) counts as zero. These two steps (comparing and removing initial non-digit strings and initial digit strings) are repeated until a difference is found or both strings are exhausted.

Keyword:

Comments (28)

  1. Erik Schnetter
    • removed comment

    There are other systems that use such dependencies, e.g. Debian packages. Can we reuse such a system for simplicity?

  2. Frank Löffler reporter
    • removed comment

    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.

  3. Barry Wardell
    • removed comment

    I imagine Debian needs to be generic in the version strings they support because they have no control over the versions strings of most packages. In the case of Cactus, we can set more strict rules on what a version string is which would simplify things quite a lot. The most straightforward would be to enforce that an interface version number is just an integer or maybe a decimal number. Is there a reason to allow more complicated version strings?

  4. Ian Hinder
    • removed comment

    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.

  5. Erik Schnetter
    • removed comment

    Since this is only supposed to catch errors, and since (presumably) this will only be a problem during transition, we can use a less official mechanism -- something that provides a clear error message suffices.

    For example, we can add a particular #define to the flesh (CCTK_HAVE_SCHEDULE_READS_WRITES), and check this in a thorn, outputting an #error if the flesh is too old. This will give a clear, concise error message.

  6. Frank Löffler reporter
    • removed comment

    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.

  7. Frank Löffler reporter
    • removed comment

    The attached patch introduces versions for capabilities. These can be specified in configuration.ccl as in this example:

    PROVIDES LORENE
    {
      VERSION 2014
      SCRIPT src/detect.sh
      LANG bash
      OPTIONS LORENE_DIR LORENE_EXTRA_LIB_DIRS LORENE_EXTRA_LIBS LORENE_INSTALL_DIR
    }
    

    and can be used in the same file, as in this example:

    REQUIRES LORENE (>=0.0.1)
    

    (The version numbers here only serve as example. They are obviously not correlated here.)

    Versions can contain alphanumeric characters, and ".+-:". Non-numeric and numeric parts are compared from left to right, lexically and numerically respectively. Valid operators for requirements are << <= = >= >>. < and > are avoided to make it clear that this is strict less or greater than.

  8. Roland Haas
    • changed status to open
    • removed 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".

  9. Erik Schnetter
    • removed comment

    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}.

  10. Frank Löffler reporter
    • removed comment

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

  11. Roland Haas
    • removed comment

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

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

    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.

    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?

  12. Ian Hinder
    • removed comment

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

  13. Roland Haas
    • removed comment

    Ian: 1.8.10 is considered newer than 1.8.9. Each numerical part (decimal points are not numeric) is compared independently as an integer(ie. this is the old Unix convention for version numbers, where versions such as 1.08 make no sense [NB: 08 would be identical to 8 and not the invalid octal number 8]).

  14. Frank Löffler reporter
    • removed comment

    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.

  15. Frank Löffler reporter
    • removed comment

    Replying to [comment:17 rhaas]:

    Is there also support for CONFLICTS?

    I am not aware of such a mechanism in Cactus.

  16. Frank Löffler reporter
    • removed comment

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

  17. Roland Haas
    • removed comment

    I commented inline in the pull request. Summary of comments:

    I would start with "All capabilities have an optional version attached". I still think that allowing the version string to start with anything is better. The reason being that (to me) the "natural" way to give a version to an ExternalLibrary is to use version_of_upstream_code-N where N is the version of our own scripts in the ExternalLibrary (this is more or less how Debian seems to do this). So if OpenMPI names there versions v17.5-experimental:2015 then I'd name the version of HDF5 that includes that tarball v17.5-experimental:2015-1.

    I did not catch this in the perl code, but I think we should allow spaces in the same location that C would ie surrounding operators and parenthesis and not require that there is no space between the comparison operator and the version number. Is there any benefit to the restriction?

    • \item[$=$] Requesting capability exactly equal to given version This should be "==", should it?

    non-digit characters is determined. The two parts are compared lexically, and "the two parts" was confusing me when I first read it. You are really referring to the second parts of each version string, but on first reading it "the two parts" sounded like the first and second part (ie the numerical and the non-numerical) part of one of the version strings.

    I would try and make sure that "numerical" makes it clear that "integer" is meant and not also floating point ie that "1.0.0" is 5 parts: 3 numerical and 2 non-numerical.

    I would use (for reasons outlined above)

    (>=2.43.2dev-1)
    

    to give an example of a version number that we'd like to see used.

  18. Frank Löffler reporter
    • changed status to open
    • removed comment

    I fixed some things, and commented on the rest (also on bitbucket). I also found and fixed a bug introduced in the rewrite of the parsing loop. In Perl, assignment binds stronger than 'or', so "a = b or c" doesn't necessarily do what you want. You have to either write "a = (b or c)", or a = b || c.

  19. Frank Löffler reporter
    • changed status to resolved
    • removed comment

    Thanks for the extensive review Roland. It made the request a whole lot better! Merged.

  20. Log in to comment