Tarantool development patches archive
 help / color / mirror / Atom feed
From: Oleg Babin via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>,
	tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH vshard 2/4] util: introduce Tarantool's semver parser
Date: Fri, 11 Feb 2022 19:38:46 +0300	[thread overview]
Message-ID: <5cf1afe8-97e2-07ae-8203-24a957fafa78@tarantool.org> (raw)
In-Reply-To: <ca4e57d5-d7d8-834a-e383-995f80ee27b5@tarantool.org>

Thanks for your answers and changes. LGTM.

On 11.02.2022 01:33, Vladislav Shpilevoy wrote:
> Thanks for the review!
>
> On 09.02.2022 18:53, Oleg Babin via Tarantool-patches wrote:
>> Thanks for your patch.
>>
>>
>> Looks good. Probably it's worth to move this code into separate module.
>>
>> There are several modules in Tarantool ecosystem that implement its own parsers.
> I don't mind if somebody would move it. And heap.lua too. But I don't really
> want to do that myself since it requires too much work not related to
> programming. Just the necessity to install CI in a new repository is enough
> for me to give that up.
>
>>> +
>>> +local function version_new(id_major, id_middle, id_minor, rel_type, rel_num,
>>> +                           id_commit)
>> I think we could validate that at least id_major is not nil.
> Yes, we can.
>
> ====================
> @@ -76,7 +76,8 @@ local version_mt = {
>   
>   local function version_new(id_major, id_middle, id_minor, rel_type, rel_num,
>                              id_commit)
> -    -- There is no any validation - the API is not public.
> +    -- There is no any proper validation - the API is not public.
> +    assert(id_major and id_middle and id_minor)
>       return setmetatable({
>           id_major = id_major,
>           id_middle = id_middle,
> ====================
>
>
> New patch:
>
> ====================
> util: introduce Tarantool's semver parser
>
> Tarantool's version since recently has semver-like format. It
> made impossible to check for features existence if they were
> introduced not in the first release of one x.x.x triplet.
>
> An alternative would be to test for feature existence by its
> usage, but it is not always possible with ease.
>
> This patch improves version parser in vshard to understand new
> version names.
>
> It will be used by a future commit about netbox's return_raw
> feature adoption to skip its tests for old versions
> (<= 2.10.0-beta2).
>
> Needed for #312
> ---
>   test/unit-luatest/suite.ini        |   5 +
>   test/unit-luatest/version_test.lua | 179 +++++++++++++++++++++++++++++
>   vshard/CMakeLists.txt              |   2 +-
>   vshard/router/init.lua             |   2 +-
>   vshard/storage/init.lua            |   2 +-
>   vshard/util.lua                    |  17 +--
>   vshard/version.lua                 | 149 ++++++++++++++++++++++++
>   7 files changed, 340 insertions(+), 16 deletions(-)
>   create mode 100644 test/unit-luatest/suite.ini
>   create mode 100644 test/unit-luatest/version_test.lua
>   create mode 100644 vshard/version.lua
>
> diff --git a/test/unit-luatest/suite.ini b/test/unit-luatest/suite.ini
> new file mode 100644
> index 0000000..303032c
> --- /dev/null
> +++ b/test/unit-luatest/suite.ini
> @@ -0,0 +1,5 @@
> +[default]
> +core = luatest
> +description = Unit tests
> +is_parallel = True
> +release_disabled =
> diff --git a/test/unit-luatest/version_test.lua b/test/unit-luatest/version_test.lua
> new file mode 100644
> index 0000000..905d36e
> --- /dev/null
> +++ b/test/unit-luatest/version_test.lua
> @@ -0,0 +1,179 @@
> +local t = require('luatest')
> +local lversion = require('vshard.version')
> +
> +local g = t.group('version')
> +
> +g.test_order = function(g)
> +    -- Example of a full version: 2.10.0-beta2-86-gc9981a567.
> +    local versions = {
> +        {
> +            str = '1.2.3-alpha',
> +            ver = lversion.new(1, 2, 3, 'alpha', 0, 0),
> +        },
> +        {
> +            str = '1.2.3-alpha-30',
> +            ver = lversion.new(1, 2, 3, 'alpha', 0, 30),
> +        },
> +        {
> +            str = '1.2.3-alpha-45',
> +            ver = lversion.new(1, 2, 3, 'alpha', 0, 45),
> +        },
> +        {
> +            str = '1.2.3-alpha1',
> +            ver = lversion.new(1, 2, 3, 'alpha', 1, 0),
> +        },
> +        {
> +            str = '1.2.3-alpha1-45',
> +            ver = lversion.new(1, 2, 3, 'alpha', 1, 45),
> +        },
> +        {
> +            str = '1.2.3-alpha2',
> +            ver = lversion.new(1, 2, 3, 'alpha', 2, 0),
> +        },
> +        {
> +            str = '1.2.3-alpha2-45',
> +            ver = lversion.new(1, 2, 3, 'alpha', 2, 45),
> +        },
> +        {
> +            str = '1.2.3-beta',
> +            ver = lversion.new(1, 2, 3, 'beta', 0, 0),
> +        },
> +        {
> +            str = '1.2.3-beta-45',
> +            ver = lversion.new(1, 2, 3, 'beta', 0, 45),
> +        },
> +        {
> +            str = '1.2.3-beta1',
> +            ver = lversion.new(1, 2, 3, 'beta', 1, 0),
> +        },
> +        {
> +            str = '1.2.3-beta1-45',
> +            ver = lversion.new(1, 2, 3, 'beta', 1, 45),
> +        },
> +        {
> +            str = '1.2.3-beta2',
> +            ver = lversion.new(1, 2, 3, 'beta', 2, 0),
> +        },
> +        {
> +            str = '1.2.3-beta2-45',
> +            ver = lversion.new(1, 2, 3, 'beta', 2, 45),
> +        },
> +        {
> +            str = '1.2.3-rc',
> +            ver = lversion.new(1, 2, 3, 'rc', 0, 0),
> +        },
> +        {
> +            str = '1.2.3-rc-45',
> +            ver = lversion.new(1, 2, 3, 'rc', 0, 45),
> +        },
> +        {
> +            str = '1.2.3-rc1',
> +            ver = lversion.new(1, 2, 3, 'rc', 1, 0),
> +        },
> +        {
> +            str = '1.2.3-rc1-45',
> +            ver = lversion.new(1, 2, 3, 'rc', 1, 45),
> +        },
> +        {
> +            str = '1.2.3-rc2',
> +            ver = lversion.new(1, 2, 3, 'rc', 2, 0),
> +        },
> +        {
> +            str = '1.2.3-rc2-45',
> +            ver = lversion.new(1, 2, 3, 'rc', 2, 45),
> +        },
> +        {
> +            str = '1.2.3-rc3',
> +            ver = lversion.new(1, 2, 3, 'rc', 3, 0),
> +        },
> +        {
> +            str = '1.2.3-rc4',
> +            ver = lversion.new(1, 2, 3, 'rc', 4, 0),
> +        },
> +        {
> +            str = '1.2.3',
> +            ver = lversion.new(1, 2, 3, nil, 0, 0),
> +        },
> +        {
> +            str = '1.2.4',
> +            ver = lversion.new(1, 2, 4, nil, 0, 0),
> +        },
> +        {
> +            str = '1.2.5-alpha',
> +            ver = lversion.new(1, 2, 5, 'alpha', 0, 0),
> +        },
> +        {
> +            str = '1.2.5-alpha1-45-gc9981a567',
> +            ver = lversion.new(1, 2, 5, 'alpha', 1, 45),
> +        },
> +        {
> +            str = '1.2.6-',
> +            ver = lversion.new(1, 2, 6, nil, 0, 0),
> +        },
> +        {
> +            str = '1.2.7-alpha-',
> +            ver = lversion.new(1, 2, 7, 'alpha', 0, 0),
> +        },
> +        {
> +            str = '1.2.7-alpha1-',
> +            ver = lversion.new(1, 2, 7, 'alpha', 1, 0),
> +        },
> +        {
> +            str = '1.2.7-alpha1-45',
> +            ver = lversion.new(1, 2, 7, 'alpha', 1, 45),
> +        },
> +        {
> +            str = '1.2.7-alpha1-46-',
> +            ver = lversion.new(1, 2, 7, 'alpha', 1, 46),
> +        },
> +        {
> +            str = '1.2.8-alpha',
> +            ver = lversion.new(1, 2, 8, 'alpha', 0, 0),
> +        },
> +        {
> +            str = '1.2.8-beta',
> +            ver = lversion.new(1, 2, 8, 'beta', 0, 0),
> +        },
> +        {
> +            str = '1.2.8-rc',
> +            ver = lversion.new(1, 2, 8, 'rc', 0, 0),
> +        },
> +        {
> +            str = '1.2.9',
> +            ver = lversion.new(1, 2, 9, nil, 0, 0),
> +        },
> +    }
> +    for i, v in pairs(versions) do
> +        local ver = lversion.parse(v.str)
> +        t.assert(ver == v.ver, ('versions ==, %d'):format(i))
> +        t.assert(not (ver ~= v.ver), ('versions not ~=, %d'):format(i))
> +        t.assert(not (ver < v.ver), ('versions not <, %d'):format(i))
> +        t.assert(not (ver > v.ver), ('versions not >, %d'):format(i))
> +        t.assert(ver <= v.ver, ('versions <=, %d'):format(i))
> +        t.assert(ver >= v.ver, ('versions <=, %d'):format(i))
> +        if i > 1 then
> +            local prev = versions[i - 1].ver
> +            t.assert(prev < ver, ('versions <, %d'):format(i))
> +            t.assert(prev <= ver, ('versions <=, %d'):format(i))
> +            t.assert(not (prev > ver), ('versions not >, %d'):format(i))
> +            t.assert(not (prev >= ver), ('versions not >=, %d'):format(i))
> +
> +            t.assert(not (ver < prev), ('versions not <, %d'):format(i))
> +            t.assert(not (ver <= prev), ('versions not <=, %d'):format(i))
> +            t.assert(ver > prev, ('versions >, %d'):format(i))
> +            t.assert(ver >= prev, ('versions >=, %d'):format(i))
> +
> +            t.assert(ver ~= prev, ('versions ~=, %d'):format(i))
> +            t.assert(not (ver == prev), ('versions not ==, %d'):format(i))
> +        end
> +    end
> +end
> +
> +g.test_error = function(g)
> +    t.assert_error_msg_contains('Could not parse version', lversion.parse,
> +                                'bad version')
> +    t.assert_error_msg_contains('Could not parse version', lversion.parse,
> +                                '1.x.x')
> +    t.assert_error_msg_contains('Could not parse version', lversion.parse,
> +                                '1.2.x')
> +end
> diff --git a/vshard/CMakeLists.txt b/vshard/CMakeLists.txt
> index 2d9be25..29500f6 100644
> --- a/vshard/CMakeLists.txt
> +++ b/vshard/CMakeLists.txt
> @@ -7,5 +7,5 @@ add_subdirectory(router)
>   
>   # Install module
>   install(FILES cfg.lua error.lua consts.lua hash.lua init.lua replicaset.lua
> -        util.lua rlist.lua heap.lua registry.lua
> +        util.lua rlist.lua heap.lua registry.lua version.lua
>           DESTINATION ${TARANTOOL_INSTALL_LUADIR}/vshard)
> diff --git a/vshard/router/init.lua b/vshard/router/init.lua
> index 064bd5a..44ed801 100644
> --- a/vshard/router/init.lua
> +++ b/vshard/router/init.lua
> @@ -7,7 +7,7 @@ local MODULE_INTERNALS = '__module_vshard_router'
>   -- Reload requirements, in case this module is reloaded manually.
>   if rawget(_G, MODULE_INTERNALS) then
>       local vshard_modules = {
> -        'vshard.consts', 'vshard.error', 'vshard.cfg',
> +        'vshard.consts', 'vshard.error', 'vshard.cfg', 'vshard.version',
>           'vshard.hash', 'vshard.replicaset', 'vshard.util'
>       }
>       for _, module in pairs(vshard_modules) do
> diff --git a/vshard/storage/init.lua b/vshard/storage/init.lua
> index 1642609..6820ad0 100644
> --- a/vshard/storage/init.lua
> +++ b/vshard/storage/init.lua
> @@ -13,7 +13,7 @@ local MODULE_INTERNALS = '__module_vshard_storage'
>   -- Reload requirements, in case this module is reloaded manually.
>   if rawget(_G, MODULE_INTERNALS) then
>       local vshard_modules = {
> -        'vshard.consts', 'vshard.error', 'vshard.cfg',
> +        'vshard.consts', 'vshard.error', 'vshard.cfg', 'vshard.version',
>           'vshard.replicaset', 'vshard.util',
>           'vshard.storage.reload_evolution', 'vshard.rlist', 'vshard.registry',
>           'vshard.heap', 'vshard.storage.ref', 'vshard.storage.sched',
> diff --git a/vshard/util.lua b/vshard/util.lua
> index 366fdea..9e0212a 100644
> --- a/vshard/util.lua
> +++ b/vshard/util.lua
> @@ -2,6 +2,7 @@
>   local log = require('log')
>   local fiber = require('fiber')
>   local lerror = require('vshard.error')
> +local lversion = require('vshard.version')
>   
>   local MODULE_INTERNALS = '__module_vshard_util'
>   local M = rawget(_G, MODULE_INTERNALS)
> @@ -16,13 +17,7 @@ if not M then
>       rawset(_G, MODULE_INTERNALS, M)
>   end
>   
> -local version_str = _TARANTOOL:sub(1, _TARANTOOL:find('-')-1)
> -local dot = version_str:find('%.')
> -local major = tonumber(version_str:sub(1, dot - 1))
> -version_str = version_str:sub(dot + 1)
> -dot = version_str:find('%.')
> -local middle = tonumber(version_str:sub(1, dot - 1))
> -local minor = tonumber(version_str:sub(dot + 1))
> +local tnt_version = lversion.parse(_TARANTOOL)
>   
>   --
>   -- Extract parts of a tuple.
> @@ -146,12 +141,8 @@ end
>   --
>   -- Check if Tarantool version is >= that a specified one.
>   --
> -local function version_is_at_least(major_need, middle_need, minor_need)
> -    if major > major_need then return true end
> -    if major < major_need then return false end
> -    if middle > middle_need then return true end
> -    if middle < middle_need then return false end
> -    return minor >= minor_need
> +local function version_is_at_least(...)
> +    return tnt_version >= lversion.new(...)
>   end
>   
>   if not version_is_at_least(1, 10, 1) then
> diff --git a/vshard/version.lua b/vshard/version.lua
> new file mode 100644
> index 0000000..a53eb60
> --- /dev/null
> +++ b/vshard/version.lua
> @@ -0,0 +1,149 @@
> +--
> +-- Semver parser adopted to Tarantool's versions.
> +-- Almost everything is the same as inhttps://semver.org.
> +--
> +-- Tarantool's version has format:
> +--
> +--     x.x.x-typen-commit-ghash
> +--
> +-- * x.x.x - major, middle, minor release numbers;
> +-- * typen - release type and its optional number: alpha1, beta5, rc10.
> +--   Optional;
> +-- * commit - commit count since the latest release. Optional;
> +-- * ghash - latest commit hash in format g<hash>. Optional.
> +--
> +-- Differences with the semver docs:
> +--
> +-- * No support for nested releases like x.x.x-alpha.beta. Only x.x.x-alpha.
> +-- * Release number is written right after its type. Not 'alpha.1' but 'alpha1'.
> +--
> +
> +local release_type_weight = {
> +    alpha = 0,
> +    beta = 1,
> +    rc = 2,
> +}
> +
> +local function release_type_cmp(t1, t2)
> +    t1 = release_type_weight[t1]
> +    t2 = release_type_weight[t2]
> +    -- 'No release type' means the greatest.
> +    if not t1 then
> +        if not t2 then
> +            return 0
> +        end
> +        return 1
> +    end
> +    if not t2 then
> +        return -1
> +    end
> +    return t1 - t2
> +end
> +
> +local function version_cmp(ver1, ver2)
> +    if ver1.id_major ~= ver2.id_major then
> +        return ver1.id_major - ver2.id_major
> +    end
> +    if ver1.id_middle ~= ver2.id_middle then
> +        return ver1.id_middle - ver2.id_middle
> +    end
> +    if ver1.id_minor ~= ver2.id_minor then
> +        return ver1.id_minor - ver2.id_minor
> +    end
> +    if ver1.rel_type ~= ver2.rel_type then
> +        return release_type_cmp(ver1.rel_type, ver2.rel_type)
> +    end
> +    if ver1.rel_num ~= ver2.rel_num then
> +        return ver1.rel_num - ver2.rel_num
> +    end
> +    if ver1.id_commit ~= ver2.id_commit then
> +        return ver1.id_commit - ver2.id_commit
> +    end
> +    return 0
> +end
> +
> +local version_mt = {
> +    __eq = function(l, r)
> +        return version_cmp(l, r) == 0
> +    end,
> +    __lt = function(l, r)
> +        return version_cmp(l, r) < 0
> +    end,
> +    __le = function(l, r)
> +        return version_cmp(l, r) <= 0
> +    end,
> +}
> +
> +local function version_new(id_major, id_middle, id_minor, rel_type, rel_num,
> +                           id_commit)
> +    -- There is no any proper validation - the API is not public.
> +    assert(id_major and id_middle and id_minor)
> +    return setmetatable({
> +        id_major = id_major,
> +        id_middle = id_middle,
> +        id_minor = id_minor,
> +        rel_type = rel_type,
> +        rel_num = rel_num,
> +        id_commit = id_commit,
> +    }, version_mt)
> +end
> +
> +local function version_parse(version_str)
> +    --  x.x.x-name<num>-<num>-g<commit>
> +    -- \____/\___/\___/\_____/
> +    --   P1   P2   P3    P4
> +    local id_major, id_middle, id_minor
> +    local rel_type
> +    local rel_num = 0
> +    local id_commit = 0
> +    local pos
> +
> +    -- Part 1 - version ID triplet.
> +    id_major, id_middle, id_minor = version_str:match('^(%d+)%.(%d+)%.(%d+)')
> +    if not id_major or not id_middle or not id_minor then
> +        error(('Could not parse version: %s'):format(version_str))
> +    end
> +    id_major = tonumber(id_major)
> +    id_middle = tonumber(id_middle)
> +    id_minor = tonumber(id_minor)
> +
> +    -- Cut to 'name<num>-<num>-g<commit>'.
> +    pos = version_str:find('-')
> +    if not pos then
> +        goto finish
> +    end
> +    version_str = version_str:sub(pos + 1)
> +
> +    -- Part 2 and 3 - release name, might be absent.
> +    rel_type, rel_num = version_str:match('^(%a+)(%d+)')
> +    if not rel_type then
> +        rel_type = version_str:match('^(%a+)')
> +        rel_num = 0
> +    else
> +        rel_num = tonumber(rel_num)
> +    end
> +
> +    -- Cut to '<num>-g<commit>'.
> +    pos = version_str:find('-')
> +    if not pos then
> +        goto finish
> +    end
> +    version_str = version_str:sub(pos + 1)
> +
> +    -- Part 4 - commit count since latest release, might be absent.
> +    id_commit = version_str:match('^(%d+)')
> +    if not id_commit then
> +        id_commit = 0
> +    else
> +        id_commit = tonumber(id_commit)
> +    end
> +
> +::finish::
> +    return version_new(id_major, id_middle, id_minor, rel_type, rel_num,
> +                       id_commit)
> +end
> +
> +return {
> +    parse = version_parse,
> +    new = version_new,
> +}

  reply	other threads:[~2022-02-11 16:39 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-02-09  0:32 [Tarantool-patches] [PATCH vshard 0/4] Router msgpack object and netbox return_raw Vladislav Shpilevoy via Tarantool-patches
2022-02-09  0:32 ` [Tarantool-patches] [PATCH vshard 1/4] test: support luatest Vladislav Shpilevoy via Tarantool-patches
2022-02-09 17:53   ` Oleg Babin via Tarantool-patches
2022-02-10 22:32     ` Vladislav Shpilevoy via Tarantool-patches
2022-02-11 16:38       ` Oleg Babin via Tarantool-patches
2022-02-09  0:32 ` [Tarantool-patches] [PATCH vshard 2/4] util: introduce Tarantool's semver parser Vladislav Shpilevoy via Tarantool-patches
2022-02-09 17:53   ` Oleg Babin via Tarantool-patches
2022-02-10 22:33     ` Vladislav Shpilevoy via Tarantool-patches
2022-02-11 16:38       ` Oleg Babin via Tarantool-patches [this message]
2022-02-09  0:32 ` [Tarantool-patches] [PATCH vshard 3/4] router: support msgpack object args Vladislav Shpilevoy via Tarantool-patches
2022-02-09 17:53   ` Oleg Babin via Tarantool-patches
2022-02-10 22:33     ` Vladislav Shpilevoy via Tarantool-patches
2022-02-11 16:38       ` Oleg Babin via Tarantool-patches
2022-02-09  0:32 ` [Tarantool-patches] [PATCH vshard 4/4] router: support netbox return_raw Vladislav Shpilevoy via Tarantool-patches
2022-02-09 17:53   ` Oleg Babin via Tarantool-patches
2022-02-10 22:34     ` Vladislav Shpilevoy via Tarantool-patches
2022-02-11 16:38       ` Oleg Babin via Tarantool-patches
2022-02-11 23:05 ` [Tarantool-patches] [PATCH vshard 0/4] Router msgpack object and " Vladislav Shpilevoy via Tarantool-patches
2022-02-15 16:55   ` Oleg Babin via Tarantool-patches
2022-02-15 21:16     ` Vladislav Shpilevoy via Tarantool-patches

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=5cf1afe8-97e2-07ae-8203-24a957fafa78@tarantool.org \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=olegrok@tarantool.org \
    --cc=v.shpilevoy@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH vshard 2/4] util: introduce Tarantool'\''s semver parser' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox