[Tarantool-patches] [PATCH 0/4][v2] Fix typos and enable checkpatch

[Tarantool-patches] [PATCH 0/4][v2] Fix typos and enable checkpatch

[Sergey Bronnikov via Tarantool-patches]

From: Sergey Bronnikov <sergeyb@tarantool.org>

Sometimes we do mistakes and typos. Reviewers spend a lot of
time in proofreading of comments, commit messages and code itself and
reports typos to authors. The idea is to automate a part of work made by reviewers
and highlight about all typos and code styles problems in continuous integration,
before sending patches to review.

Patches fixes typos in our own source code and enables checkpatch in CI and CMake.

Changelog v2:

- Added CMake targets.
- Fixed typo in a step name.
- Fixed two typos.
- Updated Github Action for checkpatch, now checkpatch runs in workflow,
  not in action.
- Ignore check COMMIT_MESSAGE.

Branch: https://github.com/tarantool/luajit/tree/ligurio/gh-xxxx-spellchecking

Sergey Bronnikov (4):
  ci: fix a step name
  codehealth: fix typos
  cmake: introduce 'check' and 'LuaJIT-checkpatch' targets
  ci: enable checkpatch

 .github/actions/checkpatch/action.yml         | 11 +++++++
 .github/workflows/lint.yml                    | 22 ++++++++++++-
 src/lj_sysprof.c                              |  2 +-
 src/lj_wbuf.h                                 |  2 +-
 src/luajit-gdb.py                             |  6 ++--
 src/luajit_lldb.py                            |  6 ++--
 test/CMakeLists.txt                           | 33 +++++++++++++++++++
 test/LuaJIT-tests/src/ctest.c                 |  2 +-
 test/tarantool-tests/CMakeLists.txt           |  2 +-
 test/tarantool-tests/fix-emit-rma.test.lua    |  2 +-
 .../gh-4199-gc64-fuse.test.lua                |  2 +-
 ...-6096-external-unwinding-on-arm64.test.lua |  2 +-
 test/tarantool-tests/gh-6163-min-max.test.lua |  2 +-
 .../lj-416-xor-before-jcc.test.lua            |  2 +-
 ...6-arm64-incorrect-check-closed-uv.test.lua |  2 +-
 .../lj-512-profiler-hook-finalizers.test.lua  |  4 +--
 .../lj-603-err-snap-restore.test.lua          |  2 +-
 .../misclib-memprof-lapi.test.lua             |  6 ++--
 test/tarantool-tests/tap.lua                  |  4 +--
 test/tarantool-tests/unit-jit-parse.test.lua  |  2 +-
 tools/memprof/parse.lua                       |  4 +--
 tools/memprof/process.lua                     |  4 +--
 22 files changed, 94 insertions(+), 30 deletions(-)
 create mode 100644 .github/actions/checkpatch/action.yml

-- 
2.34.1

[Tarantool-patches] [PATCH 1/4][v2] ci: fix a step name

[Sergey Bronnikov via Tarantool-patches]

From: Sergey Bronnikov <sergeyb@tarantool.org>

Fixed an incorrect name in a github actions step.
---
 .github/workflows/lint.yml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml
index 71ceee9a..44338f6d 100644
--- a/.github/workflows/lint.yml
+++ b/.github/workflows/lint.yml
@@ -50,6 +50,6 @@ jobs:
           echo CMAKE_BUILD_PARALLEL_LEVEL=$(($(nproc) + 1)) | tee -a $GITHUB_ENV
       - name: configure
         run: cmake -S . -B ${{ env.BUILDDIR }} -G Ninja
-      - name: test
+      - name: luacheck
         run: cmake --build . --target LuaJIT-luacheck
         working-directory: ${{ env.BUILDDIR }}
-- 
2.34.1

[Tarantool-patches] [PATCH 2/4][v2] codehealth: fix typos

[Sergey Bronnikov via Tarantool-patches]

From: Sergey Bronnikov <sergeyb@tarantool.org>

Fix typos found with codespell in files with our own source code.
---
 src/lj_sysprof.c                                            | 2 +-
 src/lj_wbuf.h                                               | 2 +-
 src/luajit-gdb.py                                           | 6 +++---
 src/luajit_lldb.py                                          | 6 +++---
 test/LuaJIT-tests/src/ctest.c                               | 2 +-
 test/tarantool-tests/CMakeLists.txt                         | 2 +-
 test/tarantool-tests/fix-emit-rma.test.lua                  | 2 +-
 test/tarantool-tests/gh-4199-gc64-fuse.test.lua             | 2 +-
 .../gh-6096-external-unwinding-on-arm64.test.lua            | 2 +-
 test/tarantool-tests/gh-6163-min-max.test.lua               | 2 +-
 test/tarantool-tests/lj-416-xor-before-jcc.test.lua         | 2 +-
 .../lj-426-arm64-incorrect-check-closed-uv.test.lua         | 2 +-
 .../lj-512-profiler-hook-finalizers.test.lua                | 4 ++--
 test/tarantool-tests/lj-603-err-snap-restore.test.lua       | 2 +-
 test/tarantool-tests/misclib-memprof-lapi.test.lua          | 6 +++---
 test/tarantool-tests/tap.lua                                | 4 ++--
 test/tarantool-tests/unit-jit-parse.test.lua                | 2 +-
 tools/memprof/parse.lua                                     | 4 ++--
 tools/memprof/process.lua                                   | 4 ++--
 19 files changed, 29 insertions(+), 29 deletions(-)

diff --git a/src/lj_sysprof.c b/src/lj_sysprof.c
index 2e9ed9b3..745eb1a3 100644
--- a/src/lj_sysprof.c
+++ b/src/lj_sysprof.c
@@ -485,7 +485,7 @@ int lj_sysprof_stop(lua_State *L)
   if (SPS_HALT == sp->state) {
     errno = sp->saved_errno;
     sp->state = SPS_IDLE;
-    /* wbuf was terminated when error occured. */
+    /* wbuf was terminated when error occurred. */
     return PROFILE_ERRIO;
   }
 
diff --git a/src/lj_wbuf.h b/src/lj_wbuf.h
index 9eaa5e51..33ec8463 100644
--- a/src/lj_wbuf.h
+++ b/src/lj_wbuf.h
@@ -75,7 +75,7 @@ void lj_wbuf_addn(struct lj_wbuf *buf, const void *src, size_t n);
 /* Write string to the buffer. */
 void LJ_FASTCALL lj_wbuf_addstring(struct lj_wbuf *buf, const char *s);
 
-/* Immediatly flush the buffer. */
+/* Immediately flush the buffer. */
 void LJ_FASTCALL lj_wbuf_flush(struct lj_wbuf *buf);
 
 /* Check flags. */
diff --git a/src/luajit-gdb.py b/src/luajit-gdb.py
index 96ee2289..afdfecd9 100644
--- a/src/luajit-gdb.py
+++ b/src/luajit-gdb.py
@@ -268,7 +268,7 @@ def strdata(obj):
     try:
         return str(cast('char *', cast('GCstr *', obj) + 1))[len(PADDING):]
     except UnicodeEncodeError:
-        return "<luajit-gdb: error occured while rendering non-ascii slot>"
+        return "<luajit-gdb: error occurred while rendering non-ascii slot>"
 
 def itypemap(o):
     if LJ_64 and not LJ_GC64:
@@ -606,7 +606,7 @@ class LJDumpTable(LJBase):
     '''
 lj-tab <GCtab *>
 
-The command receives a GCtab adress and dumps the table contents:
+The command receives a GCtab address and dumps the table contents:
 * Metatable address whether the one is set
 * Array part <asize> slots:
   <aslot ptr>: [<index>]: <tv>
@@ -677,7 +677,7 @@ coroutine guest stack:
     + CP: Protected C frame
     + PP: VM performs a call as a result of executinig pcall or xpcall
 
-If L is ommited the main coroutine is used.
+If L is omitted the main coroutine is used.
     '''
 
     def invoke(self, arg, from_tty):
diff --git a/src/luajit_lldb.py b/src/luajit_lldb.py
index 9ee10269..4d7bef81 100644
--- a/src/luajit_lldb.py
+++ b/src/luajit_lldb.py
@@ -490,7 +490,7 @@ def strdata(obj):
         ptr = cast('char *', obj + 1)
         return ptr.summary
     except UnicodeEncodeError:
-        return "<luajit-lldb: error occured while rendering non-ascii slot>"
+        return "<luajit-lldb: error occurred while rendering non-ascii slot>"
 
 def itype(o):
     return tou32(o.it64 >> 47) if LJ_GC64 else o.it
@@ -909,7 +909,7 @@ class LJDumpTable(Command):
     '''
 lj-tab <GCtab *>
 
-The command receives a GCtab adress and dumps the table contents:
+The command receives a GCtab address and dumps the table contents:
 * Metatable address whether the one is set
 * Array part <asize> slots:
   <aslot ptr>: [<index>]: <tv>
@@ -979,7 +979,7 @@ coroutine guest stack:
     + CP: Protected C frame
     + PP: VM performs a call as a result of executinig pcall or xpcall
 
-If L is ommited the main coroutine is used.
+If L is omitted the main coroutine is used.
     '''
     def execute(self, debugger, args, result):
         l = self.parse(args)
diff --git a/test/LuaJIT-tests/src/ctest.c b/test/LuaJIT-tests/src/ctest.c
index d257567b..e99f2306 100644
--- a/test/LuaJIT-tests/src/ctest.c
+++ b/test/LuaJIT-tests/src/ctest.c
@@ -234,7 +234,7 @@ static int costatus(lua_State *L, lua_State *co) {
       else
 	return CO_SUS;  /* initial state */
     }
-    default:  /* some error occured */
+    default:  /* some error occurred */
       return CO_DEAD;
   }
 }
diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt
index b1211971..369728a3 100644
--- a/test/tarantool-tests/CMakeLists.txt
+++ b/test/tarantool-tests/CMakeLists.txt
@@ -131,7 +131,7 @@ else()
 endif()
 
 # LUA_CPATH and LD_LIBRARY_PATH variables and also TESTLIBS list
-# with dependecies are set in scope of BuildTestLib macro.
+# with dependencies are set in scope of BuildTestLib macro.
 add_custom_target(tarantool-tests
   DEPENDS ${LUAJIT_TEST_BINARY} ${TESTLIBS}
 )
diff --git a/test/tarantool-tests/fix-emit-rma.test.lua b/test/tarantool-tests/fix-emit-rma.test.lua
index 1f8e66f4..7d1e5295 100644
--- a/test/tarantool-tests/fix-emit-rma.test.lua
+++ b/test/tarantool-tests/fix-emit-rma.test.lua
@@ -4,7 +4,7 @@ local test = tap.test('fix-emit-rma'):skipcond({
   ['Test requires GC64 mode enabled'] = not require('ffi').abi('gc64'),
 })
 
--- Need to test 2 cases of `emit_rma()` particulary on x64:
+-- Need to test 2 cases of `emit_rma()` particularly on x64:
 --   * `IR_LDEXP` with `fld` instruction for loading constant
 --      number `TValue` by address.
 --   * `IR_OBAR` with the corresponding `test` instruction on
diff --git a/test/tarantool-tests/gh-4199-gc64-fuse.test.lua b/test/tarantool-tests/gh-4199-gc64-fuse.test.lua
index 4513d43b..1847e5e4 100644
--- a/test/tarantool-tests/gh-4199-gc64-fuse.test.lua
+++ b/test/tarantool-tests/gh-4199-gc64-fuse.test.lua
@@ -44,7 +44,7 @@ for n = 1, 100 do
       for i = 1, 5 do
         -- This constant fusion leads to the test failure.
         a[i] = 0
-        -- This summ is not necessarry but decreases the amount of
+        -- This summ is not necessary but decreases the amount of
         -- iterations.
         a[i] = a[i] + x + y
         -- Use all FPR registers and one value from the memory
diff --git a/test/tarantool-tests/gh-6096-external-unwinding-on-arm64.test.lua b/test/tarantool-tests/gh-6096-external-unwinding-on-arm64.test.lua
index cdeea441..e43750af 100644
--- a/test/tarantool-tests/gh-6096-external-unwinding-on-arm64.test.lua
+++ b/test/tarantool-tests/gh-6096-external-unwinding-on-arm64.test.lua
@@ -1,6 +1,6 @@
 local tap = require('tap')
 
--- Test file to check correctnes of external unwinding
+-- Test file to check correctness of external unwinding
 -- in LuaJIT.
 -- See also https://github.com/LuaJIT/LuaJIT/issues/698,
 -- https://github.com/LuaJIT/LuaJIT/pull/757.
diff --git a/test/tarantool-tests/gh-6163-min-max.test.lua b/test/tarantool-tests/gh-6163-min-max.test.lua
index 882cb1a0..219d1456 100644
--- a/test/tarantool-tests/gh-6163-min-max.test.lua
+++ b/test/tarantool-tests/gh-6163-min-max.test.lua
@@ -53,7 +53,7 @@ local x = 1
 
 jit.opt.start('hotloop=1')
 
--- XXX: Looping over the operations and their arguments breakes the
+-- XXX: Looping over the operations and their arguments breaks the
 -- semantics of some optimization tests below. The cases are
 -- copy-pasted to preserve optimization semantics.
 
diff --git a/test/tarantool-tests/lj-416-xor-before-jcc.test.lua b/test/tarantool-tests/lj-416-xor-before-jcc.test.lua
index 861114e8..7181fb4f 100644
--- a/test/tarantool-tests/lj-416-xor-before-jcc.test.lua
+++ b/test/tarantool-tests/lj-416-xor-before-jcc.test.lua
@@ -41,7 +41,7 @@ local testxor = ffi.load('libtestxor')
 -- pressure and specific registers allocations.
 local handler = setmetatable({}, {
   __newindex = function ()
-    -- 0 and nil are suggested as differnt constant-zero values
+    -- 0 and nil are suggested as different constant-zero values
     -- for the call and occupied different registers.
     testxor.test_xor_func(0, 0, 0, 0, 0, 0, nil, 0)
   end
diff --git a/test/tarantool-tests/lj-426-arm64-incorrect-check-closed-uv.test.lua b/test/tarantool-tests/lj-426-arm64-incorrect-check-closed-uv.test.lua
index 4cdf1211..6e58370d 100644
--- a/test/tarantool-tests/lj-426-arm64-incorrect-check-closed-uv.test.lua
+++ b/test/tarantool-tests/lj-426-arm64-incorrect-check-closed-uv.test.lua
@@ -14,7 +14,7 @@ do
   -- The function's prototype is created with the following
   -- constants at chunk parsing. After adding this constant to
   -- the function's prototype it will be marked as gray during
-  -- propogate phase.
+  -- propagate phase.
   local function usets() uv = '' end
   _G.usets = usets
 end
diff --git a/test/tarantool-tests/lj-512-profiler-hook-finalizers.test.lua b/test/tarantool-tests/lj-512-profiler-hook-finalizers.test.lua
index f7ee344f..9a3da644 100644
--- a/test/tarantool-tests/lj-512-profiler-hook-finalizers.test.lua
+++ b/test/tarantool-tests/lj-512-profiler-hook-finalizers.test.lua
@@ -20,9 +20,9 @@ local finish = os.clock()
 
 profile.stop()
 
--- XXX: The bug is occured as stopping of callbacks invocation,
+-- XXX: The bug is occurred as stopping of callbacks invocation,
 -- when a new tick strikes inside `gc_call_finalizer()`.
--- The amount of successfull callbacks isn't stable (2-15).
+-- The amount of successful callbacks isn't stable (2-15).
 -- So, assume that amount of profiling samples should be at least
 -- more than 0.5 intervals of time during sampling.
 test:ok(nsamples >= 0.5 * (finish - start) * 1e3 / INTERVAL,
diff --git a/test/tarantool-tests/lj-603-err-snap-restore.test.lua b/test/tarantool-tests/lj-603-err-snap-restore.test.lua
index a6c831ed..1c17f0d8 100644
--- a/test/tarantool-tests/lj-603-err-snap-restore.test.lua
+++ b/test/tarantool-tests/lj-603-err-snap-restore.test.lua
@@ -11,7 +11,7 @@ local function do_test()
   local recursive_f
   local function errfunc()
     xpcall(recursive_f, errfunc)
-    -- Since this error is occured on snapshot restoration and can
+    -- Since this error is occurred on snapshot restoration and can
     -- be handled by compiler itself, we shouldn't bother a user
     -- with it.
     handler_is_called = true
diff --git a/test/tarantool-tests/misclib-memprof-lapi.test.lua b/test/tarantool-tests/misclib-memprof-lapi.test.lua
index eae20893..c8d6b879 100644
--- a/test/tarantool-tests/misclib-memprof-lapi.test.lua
+++ b/test/tarantool-tests/misclib-memprof-lapi.test.lua
@@ -52,13 +52,13 @@ local function generate_output(filename, payload)
   collectgarbage()
 
   local res, err = misc.memprof.start(filename)
-  -- Should start succesfully.
+  -- Should start successfully.
   assert(res, err)
 
   payload()
 
   res, err = misc.memprof.stop()
-  -- Should stop succesfully.
+  -- Should stop successfully.
   assert(res, err)
 end
 
@@ -188,7 +188,7 @@ test:test("output", function(subtest)
   -- Check allocation reports. The second argument is a line
   -- number of the allocation event itself. The third is a line
   -- number of the corresponding function definition. The last
-  -- one is the number of allocations. 1 event - alocation of
+  -- one is the number of allocations. 1 event - allocation of
   -- table by itself + 1 allocation of array part as far it is
   -- bigger than LJ_MAX_COLOSIZE (16).
   subtest:ok(check_alloc_report(alloc, { line = 37, linedefined = 35 }, 2))
diff --git a/test/tarantool-tests/tap.lua b/test/tarantool-tests/tap.lua
index 47a8fe87..b91ca335 100644
--- a/test/tarantool-tests/tap.lua
+++ b/test/tarantool-tests/tap.lua
@@ -1,6 +1,6 @@
 --- tap.lua internal file.
 ---
---- The Test Anything Protocol vesion 13 producer.
+--- The Test Anything Protocol version 13 producer.
 ---
 
 -- Initializer FFI for <iscdata> check.
@@ -79,7 +79,7 @@ local function ok(test, cond, message, extra)
   io.write(tindent, ("line:\t%s\n"):format(trace[#trace].line))
   for frameno, frame in ipairs(trace) do
     io.write(tindent, ("frame #%d\n"):format(frameno))
-    -- XXX: Use "half indent" to dump <frame> fiels.
+    -- XXX: Use "half indent" to dump <frame> fields.
     local findent = indent(0.5) .. tindent
     for key, value in pairs(frame) do
       io.write(findent, ("%s:\t%s\n"):format(key, value))
diff --git a/test/tarantool-tests/unit-jit-parse.test.lua b/test/tarantool-tests/unit-jit-parse.test.lua
index e4445bf4..eccc56a0 100644
--- a/test/tarantool-tests/unit-jit-parse.test.lua
+++ b/test/tarantool-tests/unit-jit-parse.test.lua
@@ -33,7 +33,7 @@ local loop_trace = traces[1]
 for irnum = 1, N_TESTS do
   local ir_pattern = expected_irs[irnum]
   local irref = loop_trace:has_ir(ir_pattern)
-  test:ok(irref, 'find IR refernce by pattern: ' .. ir_pattern)
+  test:ok(irref, 'find IR reference by pattern: ' .. ir_pattern)
 end
 
 os.exit(test:check() and 0 or 1)
diff --git a/tools/memprof/parse.lua b/tools/memprof/parse.lua
index d865267b..2c09db90 100644
--- a/tools/memprof/parse.lua
+++ b/tools/memprof/parse.lua
@@ -52,14 +52,14 @@ local function link_to_previous(heap_chunk, e, nsize)
     if not e.primary[heap_chunk[2]] then
       e.primary[heap_chunk[2]] = {
         loc = heap_chunk[3],
-        alloced = 0,
+        allocated = 0,
         freed = 0,
         count = 0,
       }
     end
     -- Save information about delta for memory heap.
     local location_data = e.primary[heap_chunk[2]]
-    location_data.alloced = location_data.alloced + nsize
+    location_data.allocated = location_data.allocated + nsize
     location_data.freed = location_data.freed + heap_chunk[1]
     location_data.count = location_data.count + 1
   end
diff --git a/tools/memprof/process.lua b/tools/memprof/process.lua
index 0bcb965b..9dc202ae 100644
--- a/tools/memprof/process.lua
+++ b/tools/memprof/process.lua
@@ -39,8 +39,8 @@ function M.form_heap_delta(events, symbols)
       for _, heap_chunk in pairs(event.primary) do
         local ev_line = symtab.demangle(symbols, heap_chunk.loc)
 
-        if (heap_chunk.alloced > 0) then
-          dheap[ev_line].dbytes = dheap[ev_line].dbytes + heap_chunk.alloced
+        if (heap_chunk.allocated > 0) then
+          dheap[ev_line].dbytes = dheap[ev_line].dbytes + heap_chunk.allocated
           dheap[ev_line].nalloc = dheap[ev_line].nalloc + heap_chunk.count
         end
 
-- 
2.34.1

[Tarantool-patches] [PATCH 3/4][v2] cmake: introduce 'check' and 'LuaJIT-checkpatch' targets

[Sergey Bronnikov via Tarantool-patches]

From: Sergey Bronnikov <sergeyb@tarantool.org>

In Tarantool we use our own fork of checkpatch [2] with additional check
types. It's logical to use it in a LuaJIT development. We don't need
check tags in commit messages like NO_DOC, NO_CHANGELOG, NO_TEST and
others, so to be able to customize command-line options Github Action, provided
by checkpatch repository [3], was added to the repository.

See documentation for used checkpatch in [4].

Patch introduce new CMake targets: LuaJIT-checkpatch, that checks
patches on top of master branch using script checkpatch.pl [1], and
target check, that combines LuaJIT-luacheck and LuaJIT-checkpatch.

1. https://docs.kernel.org/dev-tools/checkpatch.html
2. https://github.com/tarantool/checkpatch
3. https://github.com/tarantool/checkpatch/blob/master/.github/actions/checkpatch/action.yml
4. https://github.com/tarantool/checkpatch/blob/master/doc/checkpatch.rst
---
 test/CMakeLists.txt | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
index 47296a22..ccbad035 100644
--- a/test/CMakeLists.txt
+++ b/test/CMakeLists.txt
@@ -42,6 +42,39 @@ else()
   )
 endif()
 
+find_program(CHECKPATCH checkpatch.pl
+             HINTS ${PROJECT_SOURCE_DIR}/checkpatch)
+if(CHECKPATCH)
+  set(MASTER_BRANCH "tarantool/master")
+  add_custom_target(${PROJECT_NAME}-checkpatch)
+  add_custom_command(TARGET ${PROJECT_NAME}-checkpatch
+    COMMENT "Running checkpatch"
+    COMMAND
+      ${CHECKPATCH}
+        --codespell
+        --color=always
+        --git ${MASTER_BRANCH}..HEAD
+        --ignore COMMIT_LOG_LONG_LINE
+        # Requires at least two lines in commit message and this
+        # is annoying.
+        --ignore COMMIT_MESSAGE
+        --ignore NO_CHANGELOG
+        --ignore NO_DOC
+        --ignore NO_TEST
+        --show-types
+    WORKING_DIRECTORY ${PROJECT_SOURCE_DIR}
+  )
+else()
+  add_custom_target(${PROJECT_NAME}-checkpatch)
+  add_custom_command(TARGET ${PROJECT_NAME}-checkpatch
+    COMMENT "`checkpatch.pl' is not found, so ${PROJECT_NAME}-checkpatch target is dummy"
+  )
+endif()
+
+add_custom_target(check
+  DEPENDS ${PROJECT_NAME}-checkpatch ${PROJECT_NAME}-luacheck
+)
+
 set(LUAJIT_TEST_COMMAND "${LUAJIT_TEST_BINARY} -e dofile[[${LUAJIT_TEST_INIT}]]")
 separate_arguments(LUAJIT_TEST_COMMAND)
 
-- 
2.34.1

[Tarantool-patches] [PATCH 4/4][v2] ci: enable checkpatch

[Sergey Bronnikov via Tarantool-patches]

From: Sergey Bronnikov <sergeyb@tarantool.org>

Patch enables running checkpatch [1] for checking patch on a pre-commit
stage.

1. https://github.com/tarantool/checkpatch
---
 .github/actions/checkpatch/action.yml | 11 +++++++++++
 .github/workflows/lint.yml            | 20 ++++++++++++++++++++
 2 files changed, 31 insertions(+)
 create mode 100644 .github/actions/checkpatch/action.yml

diff --git a/.github/actions/checkpatch/action.yml b/.github/actions/checkpatch/action.yml
new file mode 100644
index 00000000..2336fb15
--- /dev/null
+++ b/.github/actions/checkpatch/action.yml
@@ -0,0 +1,11 @@
+name: Checkpatch
+description: Check patches against LuaJIT development guidelines
+runs:
+  using: composite
+  steps:
+    - uses: actions/checkout@v3
+      with:
+        repository: tarantool/checkpatch
+        path: 'checkpatch'
+    - run: apt install -y codespell
+      shell: bash
diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml
index 44338f6d..d86c5d54 100644
--- a/.github/workflows/lint.yml
+++ b/.github/workflows/lint.yml
@@ -53,3 +53,23 @@ jobs:
       - name: luacheck
         run: cmake --build . --target LuaJIT-luacheck
         working-directory: ${{ env.BUILDDIR }}
+
+  checkpatch:
+    runs-on: [self-hosted, lightweight, Linux, x86_64]
+
+    steps:
+      - uses: actions/checkout@v3
+        with:
+          fetch-depth: 0
+          submodules: recursive
+      - name: checkpatch
+        uses: ./.github/actions/checkpatch
+      - name: environment
+        uses: ./.github/actions/setup
+      - name: make tarantool/master available
+        run: git checkout tarantool/master && git checkout -
+      - name: configure
+        run: cmake -S . -B ${{ env.BUILDDIR }}
+      - name: checkpatch
+        run: cmake --build . --target LuaJIT-checkpatch
+        working-directory: ${{ env.BUILDDIR }}
-- 
2.34.1

Re: [Tarantool-patches] [PATCH 1/4][v2] ci: fix a step name

[Maxim Kokryashkin via Tarantool-patches]

[-- Attachment #1: Type: text/plain, Size: 1056 bytes --]


Hi!
Thanks for the patch!
LGTM, however, I strongly discourage addition of
non-relevant changes to the series.
--
Best regards,
Maxim Kokryashkin
 
  
>Понедельник, 17 июля 2023, 16:37 +03:00 от Sergey Bronnikov via Tarantool-patches <tarantool-patches@dev.tarantool.org>:
> 
>From: Sergey Bronnikov < sergeyb@tarantool.org >
>
>Fixed an incorrect name in a github actions step.
>---
> .github/workflows/lint.yml | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml
>index 71ceee9a..44338f6d 100644
>--- a/.github/workflows/lint.yml
>+++ b/.github/workflows/lint.yml
>@@ -50,6 +50,6 @@ jobs:
>           echo CMAKE_BUILD_PARALLEL_LEVEL=$(($(nproc) + 1)) | tee -a $GITHUB_ENV
>       - name: configure
>         run: cmake -S . -B ${{ env.BUILDDIR }} -G Ninja
>- - name: test
>+ - name: luacheck
>         run: cmake --build . --target LuaJIT-luacheck
>         working-directory: ${{ env.BUILDDIR }}
>--
>2.34.1
 

[-- Attachment #2: Type: text/html, Size: 1781 bytes --]

Re: [Tarantool-patches] [PATCH 2/4][v2] codehealth: fix typos

[Maxim Kokryashkin via Tarantool-patches]

[-- Attachment #1: Type: text/plain, Size: 15462 bytes --]


Hi, Sergey!
Thanks for the patch!
LGTM
 
--
Best regards,
Maxim Kokryashkin
 
  
>Понедельник, 17 июля 2023, 16:38 +03:00 от Sergey Bronnikov via Tarantool-patches <tarantool-patches@dev.tarantool.org>:
> 
>From: Sergey Bronnikov < sergeyb@tarantool.org >
>
>Fix typos found with codespell in files with our own source code.
>---
> src/lj_sysprof.c | 2 +-
> src/lj_wbuf.h | 2 +-
> src/luajit-gdb.py | 6 +++---
> src/luajit_lldb.py | 6 +++---
> test/LuaJIT-tests/src/ctest.c | 2 +-
> test/tarantool-tests/CMakeLists.txt | 2 +-
> test/tarantool-tests/fix-emit-rma.test.lua | 2 +-
> test/tarantool-tests/gh-4199-gc64-fuse.test.lua | 2 +-
> .../gh-6096-external-unwinding-on-arm64.test.lua | 2 +-
> test/tarantool-tests/gh-6163-min-max.test.lua | 2 +-
> test/tarantool-tests/lj-416-xor-before-jcc.test.lua | 2 +-
> .../lj-426-arm64-incorrect-check-closed-uv.test.lua | 2 +-
> .../lj-512-profiler-hook-finalizers.test.lua | 4 ++--
> test/tarantool-tests/lj-603-err-snap-restore.test.lua | 2 +-
> test/tarantool-tests/misclib-memprof-lapi.test.lua | 6 +++---
> test/tarantool-tests/tap.lua | 4 ++--
> test/tarantool-tests/unit-jit-parse.test.lua | 2 +-
> tools/memprof/parse.lua | 4 ++--
> tools/memprof/process.lua | 4 ++--
> 19 files changed, 29 insertions(+), 29 deletions(-)
>
>diff --git a/src/lj_sysprof.c b/src/lj_sysprof.c
>index 2e9ed9b3..745eb1a3 100644
>--- a/src/lj_sysprof.c
>+++ b/src/lj_sysprof.c
>@@ -485,7 +485,7 @@ int lj_sysprof_stop(lua_State *L)
>   if (SPS_HALT == sp->state) {
>     errno = sp->saved_errno;
>     sp->state = SPS_IDLE;
>- /* wbuf was terminated when error occured. */
>+ /* wbuf was terminated when error occurred. */
>     return PROFILE_ERRIO;
>   }
> 
>diff --git a/src/lj_wbuf.h b/src/lj_wbuf.h
>index 9eaa5e51..33ec8463 100644
>--- a/src/lj_wbuf.h
>+++ b/src/lj_wbuf.h
>@@ -75,7 +75,7 @@ void lj_wbuf_addn(struct lj_wbuf *buf, const void *src, size_t n);
> /* Write string to the buffer. */
> void LJ_FASTCALL lj_wbuf_addstring(struct lj_wbuf *buf, const char *s);
> 
>-/* Immediatly flush the buffer. */
>+/* Immediately flush the buffer. */
> void LJ_FASTCALL lj_wbuf_flush(struct lj_wbuf *buf);
> 
> /* Check flags. */
>diff --git a/src/luajit-gdb.py b/src/luajit-gdb.py
>index 96ee2289..afdfecd9 100644
>--- a/src/luajit-gdb.py
>+++ b/src/luajit-gdb.py
>@@ -268,7 +268,7 @@ def strdata(obj):
>     try:
>         return str(cast('char *', cast('GCstr *', obj) + 1))[len(PADDING):]
>     except UnicodeEncodeError:
>- return "<luajit-gdb: error occured while rendering non-ascii slot>"
>+ return "<luajit-gdb: error occurred while rendering non-ascii slot>"
> 
> def itypemap(o):
>     if LJ_64 and not LJ_GC64:
>@@ -606,7 +606,7 @@ class LJDumpTable(LJBase):
>     '''
> lj-tab <GCtab *>
> 
>-The command receives a GCtab adress and dumps the table contents:
>+The command receives a GCtab address and dumps the table contents:
> * Metatable address whether the one is set
> * Array part <asize> slots:
>   <aslot ptr>: [<index>]: <tv>
>@@ -677,7 +677,7 @@ coroutine guest stack:
>     + CP: Protected C frame
>     + PP: VM performs a call as a result of executinig pcall or xpcall
> 
>-If L is ommited the main coroutine is used.
>+If L is omitted the main coroutine is used.
>     '''
> 
>     def invoke(self, arg, from_tty):
>diff --git a/src/luajit_lldb.py b/src/luajit_lldb.py
>index 9ee10269..4d7bef81 100644
>--- a/src/luajit_lldb.py
>+++ b/src/luajit_lldb.py
>@@ -490,7 +490,7 @@ def strdata(obj):
>         ptr = cast('char *', obj + 1)
>         return ptr.summary
>     except UnicodeEncodeError:
>- return "<luajit-lldb: error occured while rendering non-ascii slot>"
>+ return "<luajit-lldb: error occurred while rendering non-ascii slot>"
> 
> def itype(o):
>     return tou32(o.it64 >> 47) if LJ_GC64 else o.it
>@@ -909,7 +909,7 @@ class LJDumpTable(Command):
>     '''
> lj-tab <GCtab *>
> 
>-The command receives a GCtab adress and dumps the table contents:
>+The command receives a GCtab address and dumps the table contents:
> * Metatable address whether the one is set
> * Array part <asize> slots:
>   <aslot ptr>: [<index>]: <tv>
>@@ -979,7 +979,7 @@ coroutine guest stack:
>     + CP: Protected C frame
>     + PP: VM performs a call as a result of executinig pcall or xpcall
> 
>-If L is ommited the main coroutine is used.
>+If L is omitted the main coroutine is used.
>     '''
>     def execute(self, debugger, args, result):
>         l = self.parse(args)
>diff --git a/test/LuaJIT-tests/src/ctest.c b/test/LuaJIT-tests/src/ctest.c
>index d257567b..e99f2306 100644
>--- a/test/LuaJIT-tests/src/ctest.c
>+++ b/test/LuaJIT-tests/src/ctest.c
>@@ -234,7 +234,7 @@ static int costatus(lua_State *L, lua_State *co) {
>       else
>  return CO_SUS; /* initial state */
>     }
>- default: /* some error occured */
>+ default: /* some error occurred */
>       return CO_DEAD;
>   }
> }
>diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt
>index b1211971..369728a3 100644
>--- a/test/tarantool-tests/CMakeLists.txt
>+++ b/test/tarantool-tests/CMakeLists.txt
>@@ -131,7 +131,7 @@ else()
> endif()
> 
> # LUA_CPATH and LD_LIBRARY_PATH variables and also TESTLIBS list
>-# with dependecies are set in scope of BuildTestLib macro.
>+# with dependencies are set in scope of BuildTestLib macro.
> add_custom_target(tarantool-tests
>   DEPENDS ${LUAJIT_TEST_BINARY} ${TESTLIBS}
> )
>diff --git a/test/tarantool-tests/fix-emit-rma.test.lua b/test/tarantool-tests/fix-emit-rma.test.lua
>index 1f8e66f4..7d1e5295 100644
>--- a/test/tarantool-tests/fix-emit-rma.test.lua
>+++ b/test/tarantool-tests/fix-emit-rma.test.lua
>@@ -4,7 +4,7 @@ local test = tap.test('fix-emit-rma'):skipcond({
>   ['Test requires GC64 mode enabled'] = not require('ffi').abi('gc64'),
> })
> 
>--- Need to test 2 cases of `emit_rma()` particulary on x64:
>+-- Need to test 2 cases of `emit_rma()` particularly on x64:
> -- * `IR_LDEXP` with `fld` instruction for loading constant
> -- number `TValue` by address.
> -- * `IR_OBAR` with the corresponding `test` instruction on
>diff --git a/test/tarantool-tests/gh-4199-gc64-fuse.test.lua b/test/tarantool-tests/gh-4199-gc64-fuse.test.lua
>index 4513d43b..1847e5e4 100644
>--- a/test/tarantool-tests/gh-4199-gc64-fuse.test.lua
>+++ b/test/tarantool-tests/gh-4199-gc64-fuse.test.lua
>@@ -44,7 +44,7 @@ for n = 1, 100 do
>       for i = 1, 5 do
>         -- This constant fusion leads to the test failure.
>         a[i] = 0
>- -- This summ is not necessarry but decreases the amount of
>+ -- This summ is not necessary but decreases the amount of
>         -- iterations.
>         a[i] = a[i] + x + y
>         -- Use all FPR registers and one value from the memory
>diff --git a/test/tarantool-tests/gh-6096-external-unwinding-on-arm64.test.lua b/test/tarantool-tests/gh-6096-external-unwinding-on-arm64.test.lua
>index cdeea441..e43750af 100644
>--- a/test/tarantool-tests/gh-6096-external-unwinding-on-arm64.test.lua
>+++ b/test/tarantool-tests/gh-6096-external-unwinding-on-arm64.test.lua
>@@ -1,6 +1,6 @@
> local tap = require('tap')
> 
>--- Test file to check correctnes of external unwinding
>+-- Test file to check correctness of external unwinding
> -- in LuaJIT.
> -- See also  https://github.com/LuaJIT/LuaJIT/issues/698 ,
> --  https://github.com/LuaJIT/LuaJIT/pull/757 .
>diff --git a/test/tarantool-tests/gh-6163-min-max.test.lua b/test/tarantool-tests/gh-6163-min-max.test.lua
>index 882cb1a0..219d1456 100644
>--- a/test/tarantool-tests/gh-6163-min-max.test.lua
>+++ b/test/tarantool-tests/gh-6163-min-max.test.lua
>@@ -53,7 +53,7 @@ local x = 1
> 
> jit.opt.start('hotloop=1')
> 
>--- XXX: Looping over the operations and their arguments breakes the
>+-- XXX: Looping over the operations and their arguments breaks the
> -- semantics of some optimization tests below. The cases are
> -- copy-pasted to preserve optimization semantics.
> 
>diff --git a/test/tarantool-tests/lj-416-xor-before-jcc.test.lua b/test/tarantool-tests/lj-416-xor-before-jcc.test.lua
>index 861114e8..7181fb4f 100644
>--- a/test/tarantool-tests/lj-416-xor-before-jcc.test.lua
>+++ b/test/tarantool-tests/lj-416-xor-before-jcc.test.lua
>@@ -41,7 +41,7 @@ local testxor = ffi.load('libtestxor')
> -- pressure and specific registers allocations.
> local handler = setmetatable({}, {
>   __newindex = function ()
>- -- 0 and nil are suggested as differnt constant-zero values
>+ -- 0 and nil are suggested as different constant-zero values
>     -- for the call and occupied different registers.
>     testxor.test_xor_func(0, 0, 0, 0, 0, 0, nil, 0)
>   end
>diff --git a/test/tarantool-tests/lj-426-arm64-incorrect-check-closed-uv.test.lua b/test/tarantool-tests/lj-426-arm64-incorrect-check-closed-uv.test.lua
>index 4cdf1211..6e58370d 100644
>--- a/test/tarantool-tests/lj-426-arm64-incorrect-check-closed-uv.test.lua
>+++ b/test/tarantool-tests/lj-426-arm64-incorrect-check-closed-uv.test.lua
>@@ -14,7 +14,7 @@ do
>   -- The function's prototype is created with the following
>   -- constants at chunk parsing. After adding this constant to
>   -- the function's prototype it will be marked as gray during
>- -- propogate phase.
>+ -- propagate phase.
>   local function usets() uv = '' end
>   _G.usets = usets
> end
>diff --git a/test/tarantool-tests/lj-512-profiler-hook-finalizers.test.lua b/test/tarantool-tests/lj-512-profiler-hook-finalizers.test.lua
>index f7ee344f..9a3da644 100644
>--- a/test/tarantool-tests/lj-512-profiler-hook-finalizers.test.lua
>+++ b/test/tarantool-tests/lj-512-profiler-hook-finalizers.test.lua
>@@ -20,9 +20,9 @@ local finish = os.clock()
> 
> profile.stop()
> 
>--- XXX: The bug is occured as stopping of callbacks invocation,
>+-- XXX: The bug is occurred as stopping of callbacks invocation,
> -- when a new tick strikes inside `gc_call_finalizer()`.
>--- The amount of successfull callbacks isn't stable (2-15).
>+-- The amount of successful callbacks isn't stable (2-15).
> -- So, assume that amount of profiling samples should be at least
> -- more than 0.5 intervals of time during sampling.
> test:ok(nsamples >= 0.5 * (finish - start) * 1e3 / INTERVAL,
>diff --git a/test/tarantool-tests/lj-603-err-snap-restore.test.lua b/test/tarantool-tests/lj-603-err-snap-restore.test.lua
>index a6c831ed..1c17f0d8 100644
>--- a/test/tarantool-tests/lj-603-err-snap-restore.test.lua
>+++ b/test/tarantool-tests/lj-603-err-snap-restore.test.lua
>@@ -11,7 +11,7 @@ local function do_test()
>   local recursive_f
>   local function errfunc()
>     xpcall(recursive_f, errfunc)
>- -- Since this error is occured on snapshot restoration and can
>+ -- Since this error is occurred on snapshot restoration and can
>     -- be handled by compiler itself, we shouldn't bother a user
>     -- with it.
>     handler_is_called = true
>diff --git a/test/tarantool-tests/misclib-memprof-lapi.test.lua b/test/tarantool-tests/misclib-memprof-lapi.test.lua
>index eae20893..c8d6b879 100644
>--- a/test/tarantool-tests/misclib-memprof-lapi.test.lua
>+++ b/test/tarantool-tests/misclib-memprof-lapi.test.lua
>@@ -52,13 +52,13 @@ local function generate_output(filename, payload)
>   collectgarbage()
> 
>   local res, err = misc.memprof.start(filename)
>- -- Should start succesfully.
>+ -- Should start successfully.
>   assert(res, err)
> 
>   payload()
> 
>   res, err = misc.memprof.stop()
>- -- Should stop succesfully.
>+ -- Should stop successfully.
>   assert(res, err)
> end
> 
>@@ -188,7 +188,7 @@ test:test("output", function(subtest)
>   -- Check allocation reports. The second argument is a line
>   -- number of the allocation event itself. The third is a line
>   -- number of the corresponding function definition. The last
>- -- one is the number of allocations. 1 event - alocation of
>+ -- one is the number of allocations. 1 event - allocation of
>   -- table by itself + 1 allocation of array part as far it is
>   -- bigger than LJ_MAX_COLOSIZE (16).
>   subtest:ok(check_alloc_report(alloc, { line = 37, linedefined = 35 }, 2))
>diff --git a/test/tarantool-tests/tap.lua b/test/tarantool-tests/tap.lua
>index 47a8fe87..b91ca335 100644
>--- a/test/tarantool-tests/tap.lua
>+++ b/test/tarantool-tests/tap.lua
>@@ -1,6 +1,6 @@
> --- tap.lua internal file.
> ---
>---- The Test Anything Protocol vesion 13 producer.
>+--- The Test Anything Protocol version 13 producer.
> ---
> 
> -- Initializer FFI for <iscdata> check.
>@@ -79,7 +79,7 @@ local function ok(test, cond, message, extra)
>   io.write(tindent, ("line:\t%s\n"):format(trace[#trace].line))
>   for frameno, frame in ipairs(trace) do
>     io.write(tindent, ("frame #%d\n"):format(frameno))
>- -- XXX: Use "half indent" to dump <frame> fiels.
>+ -- XXX: Use "half indent" to dump <frame> fields.
>     local findent = indent(0.5) .. tindent
>     for key, value in pairs(frame) do
>       io.write(findent, ("%s:\t%s\n"):format(key, value))
>diff --git a/test/tarantool-tests/unit-jit-parse.test.lua b/test/tarantool-tests/unit-jit-parse.test.lua
>index e4445bf4..eccc56a0 100644
>--- a/test/tarantool-tests/unit-jit-parse.test.lua
>+++ b/test/tarantool-tests/unit-jit-parse.test.lua
>@@ -33,7 +33,7 @@ local loop_trace = traces[1]
> for irnum = 1, N_TESTS do
>   local ir_pattern = expected_irs[irnum]
>   local irref = loop_trace:has_ir(ir_pattern)
>- test:ok(irref, 'find IR refernce by pattern: ' .. ir_pattern)
>+ test:ok(irref, 'find IR reference by pattern: ' .. ir_pattern)
> end
> 
> os.exit(test:check() and 0 or 1)
>diff --git a/tools/memprof/parse.lua b/tools/memprof/parse.lua
>index d865267b..2c09db90 100644
>--- a/tools/memprof/parse.lua
>+++ b/tools/memprof/parse.lua
>@@ -52,14 +52,14 @@ local function link_to_previous(heap_chunk, e, nsize)
>     if not e.primary[heap_chunk[2]] then
>       e.primary[heap_chunk[2]] = {
>         loc = heap_chunk[3],
>- alloced = 0,
>+ allocated = 0,
>         freed = 0,
>         count = 0,
>       }
>     end
>     -- Save information about delta for memory heap.
>     local location_data = e.primary[heap_chunk[2]]
>- location_data.alloced = location_data.alloced + nsize
>+ location_data.allocated = location_data.allocated + nsize
>     location_data.freed = location_data.freed + heap_chunk[1]
>     location_data.count = location_data.count + 1
>   end
>diff --git a/tools/memprof/process.lua b/tools/memprof/process.lua
>index 0bcb965b..9dc202ae 100644
>--- a/tools/memprof/process.lua
>+++ b/tools/memprof/process.lua
>@@ -39,8 +39,8 @@ function M.form_heap_delta(events, symbols)
>       for _, heap_chunk in pairs(event.primary) do
>         local ev_line = symtab.demangle(symbols, heap_chunk.loc)
> 
>- if (heap_chunk.alloced > 0) then
>- dheap[ev_line].dbytes = dheap[ev_line].dbytes + heap_chunk.alloced
>+ if (heap_chunk.allocated > 0) then
>+ dheap[ev_line].dbytes = dheap[ev_line].dbytes + heap_chunk.allocated
>           dheap[ev_line].nalloc = dheap[ev_line].nalloc + heap_chunk.count
>         end
> 
>--
>2.34.1
 

[-- Attachment #2: Type: text/html, Size: 19105 bytes --]

Re: [Tarantool-patches] [PATCH 3/4][v2] cmake: introduce 'check' and 'LuaJIT-checkpatch' targets

[Maxim Kokryashkin via Tarantool-patches]

[-- Attachment #1: Type: text/plain, Size: 3590 bytes --]


Hi, Sergey!
Please consider my comments below.
 
> 
>>From: Sergey Bronnikov < sergeyb@tarantool.org >
>>
>>In Tarantool we use our own fork of checkpatch [2] with additional check
>>types. It's logical to use it in a LuaJIT development. We don't need
>Typo: s/in a/in/
>>check tags in commit messages like NO_DOC, NO_CHANGELOG, NO_TEST and
>>others, so to be able to customize command-line options Github Action, provided
>>by checkpatch repository [3], was added to the repository.
>Typo: s/by checkpatch/by the checkpatch/
>>
>>See documentation for used checkpatch in [4].
>Typo: s/documentation/the documentation/
>Typo: s/for used/for the/
>>
>>Patch introduce new CMake targets: LuaJIT-checkpatch, that checks
>Typo: s/introduce/introduces/
>>patches on top of master branch using script checkpatch.pl [1], and
>Typo: s/on top of/on top of the/
>>target check, that combines LuaJIT-luacheck and LuaJIT-checkpatch.
>>
>>1.  https://docs.kernel.org/dev-tools/checkpatch.html
>>2.  https://github.com/tarantool/checkpatch
>>3.  https://github.com/tarantool/checkpatch/blob/master/.github/actions/checkpatch/action.yml
>>4.  https://github.com/tarantool/checkpatch/blob/master/doc/checkpatch.rst
>Nit: It is kinda strange to see link [1] going after the link [4] in the commit message.
>I think, it generally looks clearer, when they are ordered, but that’s a matter of taste.
>Feel free to ignore.
>>
>>--- test/CMakeLists.txt | 33 +++++++++++++++++++++++++++++++++
>> 1 file changed, 33 insertions(+)
>>
>>diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
>>index 47296a22..ccbad035 100644
>>--- a/test/CMakeLists.txt
>>+++ b/test/CMakeLists.txt
>>@@ -42,6 +42,39 @@ else()
>>   )
>> endif()
>> 
>>+find_program(CHECKPATCH checkpatch.pl
>>+ HINTS ${PROJECT_SOURCE_DIR}/checkpatch)
>>+if(CHECKPATCH)
>I don’t really like that `MASTER_BRANCH` name is hardcoded. I think
>it’s possible to implement it similarly to how it’s done in the `tarantool/checkpatch`
>github action[1] with revision range. Or, at least, it is for sure possible to obtain
>the master branch name dynamically.
>>+ set(MASTER_BRANCH "tarantool/master")
>>+ add_custom_target(${PROJECT_NAME}-checkpatch)
>>+ add_custom_command(TARGET ${PROJECT_NAME}-checkpatch
>>+ COMMENT "Running checkpatch"
>>+ COMMAND
>>+ ${CHECKPATCH}
>>+ --codespell
>>+ --color=always
>>+ --git ${MASTER_BRANCH}..HEAD
>>+ --ignore COMMIT_LOG_LONG_LINE
>>+ # Requires at least two lines in commit message and this
>>+ # is annoying.
>>+ --ignore COMMIT_MESSAGE
>>+ --ignore NO_CHANGELOG
>>+ --ignore NO_DOC
>>+ --ignore NO_TEST
>>+ --show-types
>>+ WORKING_DIRECTORY ${PROJECT_SOURCE_DIR}
>>+ )
>>+else()
>>+ add_custom_target(${PROJECT_NAME}-checkpatch)
>It seems like the target definition can be moved out of the `if` statement
>just before it.
>>+ add_custom_command(TARGET ${PROJECT_NAME}-checkpatch
>>+ COMMENT "`checkpatch.pl' is not found, so ${PROJECT_NAME}-checkpatch target is dummy"
>>+ )
>>+endif()
>>+
>>+add_custom_target(check
>>+ DEPENDS ${PROJECT_NAME}-checkpatch ${PROJECT_NAME}-luacheck
>>+)
>>+
>As I have already said offline, I think we should include the `check` target as a dependency to the `test` target, just like it is currently done for the luacheck. It is much more convenient for local testing that way.
>> set(LUAJIT_TEST_COMMAND "${LUAJIT_TEST_BINARY} -e dofile[[${LUAJIT_TEST_INIT}]]")
>> separate_arguments(LUAJIT_TEST_COMMAND)
>> 
>>--
>>2.34.1
>[1]:  https://github.com/tarantool/checkpatch/blob/master/.github/actions/checkpatch/action.yml
>--
>Best regards,
>Maxim Kokryashkin

[-- Attachment #2: Type: text/html, Size: 6327 bytes --]

Re: [Tarantool-patches] [PATCH 4/4][v2] ci: enable checkpatch

[Maxim Kokryashkin via Tarantool-patches]

[-- Attachment #1: Type: text/plain, Size: 2635 bytes --]


Hi, Sergey!
Thanks for the patch!
Please consider my comments below.
 
> 
>>From: Sergey Bronnikov < sergeyb@tarantool.org >
>>
>>Patch enables running checkpatch [1] for checking patch on a pre-commit
>The pre-commit stage makes no sense when we are talking about CI — the
>commit has already occurred. That’s just a conventional workflow configuration.
> 
>Having that in mind, along with some grammar related issues. I suggest
>rephrasing it like:
>| Patch adds a CI workflow with the checkpatch[1] run.
>>stage.
>>
>>1.  https://github.com/tarantool/checkpatch
>>---
>> .github/actions/checkpatch/action.yml | 11 +++++++++++
>> .github/workflows/lint.yml | 20 ++++++++++++++++++++
>> 2 files changed, 31 insertions(+)
>> create mode 100644 .github/actions/checkpatch/action.yml
>>
>>diff --git a/.github/actions/checkpatch/action.yml b/.github/actions/checkpatch/action.yml
>>new file mode 100644
>>index 00000000..2336fb15
>>--- /dev/null
>>+++ b/.github/actions/checkpatch/action.yml
>>@@ -0,0 +1,11 @@
>>+name: Checkpatch
>>+description: Check patches against LuaJIT development guidelines
>I think it’s better to change the action name to `Setup checkpatch`,
>since the actual run is not performed here.
>The description should be updated correspondingly. Also, AFAIK, there
>is no such thing as `LuaJIT guidelines`, so maybe it’s better to change it
>to `Tarantool guidelines` if you want to keep that part.
>>+runs:
>>+ using: composite
>>+ steps:
>>+ - uses: actions/checkout@v3
>>+ with:
>>+ repository: tarantool/checkpatch
>>+ path: 'checkpatch'
>>+ - run: apt install -y codespell
>>+ shell: bash
>>diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml
>>index 44338f6d..d86c5d54 100644
>>--- a/.github/workflows/lint.yml
>>+++ b/.github/workflows/lint.yml
>>@@ -53,3 +53,23 @@ jobs:
>>       - name: luacheck
>>         run: cmake --build . --target LuaJIT-luacheck
>>         working-directory: ${{ env.BUILDDIR }}
>>+
>>+ checkpatch:
>>+ runs-on: [self-hosted, lightweight, Linux, x86_64]
>>+
>>+ steps:
>>+ - uses: actions/checkout@v3
>>+ with:
>>+ fetch-depth: 0
>>+ submodules: recursive
>>+ - name: checkpatch
>>+ uses: ./.github/actions/checkpatch
>>+ - name: environment
>>+ uses: ./.github/actions/setup
>>+ - name: make tarantool/master available
>>+ run: git checkout tarantool/master && git checkout -
>>+ - name: configure
>>+ run: cmake -S . -B ${{ env.BUILDDIR }}
>>+ - name: checkpatch
>>+ run: cmake --build . --target LuaJIT-checkpatch
>>+ working-directory: ${{ env.BUILDDIR }}
>>--
>>2.34.1
>--
>Best regards,
>Maxim Kokryashkin
> 

[-- Attachment #2: Type: text/html, Size: 3865 bytes --]

Re: [Tarantool-patches] [PATCH 3/4][v2] cmake: introduce 'check' and 'LuaJIT-checkpatch' targets

[Sergey Bronnikov via Tarantool-patches]

[-- Attachment #1: Type: text/plain, Size: 4831 bytes --]

Hi, Max!

Thanks for your comments!

See my answers below. Updated patch was force-pushed.


On 7/17/23 19:10, Maxim Kokryashkin via Tarantool-patches wrote:
> Hi, Sergey!
> Please consider my comments below.
>
>         From: Sergey Bronnikov <sergeyb@tarantool.org
>         </compose?To=sergeyb@tarantool.org>>
>
>         In Tarantool we use our own fork of checkpatch [2] with
>         additional check
>         types. It's logical to use it in a LuaJIT development. We
>         don't need
>
>     Typo: s/in a/in/
>
Fixed.
>
>         check tags in commit messages like NO_DOC, NO_CHANGELOG,
>         NO_TEST and
>         others, so to be able to customize command-line options Github
>         Action, provided
>         by checkpatch repository [3], was added to the repository.
>
>     Typo: s/by checkpatch/by the checkpatch/
>
Fixed.
>
>
>         See documentation for used checkpatch in [4].
>
>     Typo: s/documentation/the documentation/
>     Typo: s/for used/for the/
>
Fixed. Fixed.
>
>
>         Patch introduce new CMake targets: LuaJIT-checkpatch, that checks
>
>     Typo: s/introduce/introduces/
>
Fixed.
>
>         patches on top of master branch using script checkpatch.pl
>         [1], and
>
>     Typo: s/on top of/on top of the/
>
Fixed.
>
>         target check, that combines LuaJIT-luacheck and LuaJIT-checkpatch.
>
>         1. https://docs.kernel.org/dev-tools/checkpatch.html
>         2. https://github.com/tarantool/checkpatch
>         3.
>         https://github.com/tarantool/checkpatch/blob/master/.github/actions/checkpatch/action.yml
>         4.
>         https://github.com/tarantool/checkpatch/blob/master/doc/checkpatch.rst
>
>     Nit: It is kinda strange to see link [1] going after the link [4]
>     in the commit message.
>     I think, it generally looks clearer, when they are ordered, but
>     that’s a matter of taste.
>     Feel free to ignore.
>
Rewrote description and fixed order of references.
>
>
>         --- test/CMakeLists.txt | 33 +++++++++++++++++++++++++++++++++
>          1 file changed, 33 insertions(+)
>
>         diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
>         index 47296a22..ccbad035 100644
>         --- a/test/CMakeLists.txt
>         +++ b/test/CMakeLists.txt
>         @@ -42,6 +42,39 @@ else()
>            )
>          endif()
>
>         +find_program(CHECKPATCH checkpatch.pl
>         + HINTS ${PROJECT_SOURCE_DIR}/checkpatch)
>         +if(CHECKPATCH)
>
>     I don’t really like that `MASTER_BRANCH` name is hardcoded. I think
>     it’s possible to implement it similarly to how it’s done in the
>     `tarantool/checkpatch`
>     github action[1] with revision range. Or, at least, it is for sure
>     possible to obtain
>     the master branch name dynamically.
>
In LuaJIT we have a single branch for merging new patches - 
tarantool/master.

Why do you need to redefine master branch?

>         + set(MASTER_BRANCH "tarantool/master")
>         + add_custom_target(${PROJECT_NAME}-checkpatch)
>         + add_custom_command(TARGET ${PROJECT_NAME}-checkpatch
>         + COMMENT "Running checkpatch"
>         + COMMAND
>         + ${CHECKPATCH}
>         + --codespell
>         + --color=always
>         + --git ${MASTER_BRANCH}..HEAD
>         + --ignore COMMIT_LOG_LONG_LINE
>         + # Requires at least two lines in commit message and this
>         + # is annoying.
>         + --ignore COMMIT_MESSAGE
>         + --ignore NO_CHANGELOG
>         + --ignore NO_DOC
>         + --ignore NO_TEST
>         + --show-types
>         + WORKING_DIRECTORY ${PROJECT_SOURCE_DIR}
>         + )
>         +else()
>         + add_custom_target(${PROJECT_NAME}-checkpatch)
>
>     It seems like the target definition can be moved out of the `if`
>     statement
>     just before it.
>
Done. As well as definition of MASTER_BRANCH variable.
>
>         + add_custom_command(TARGET ${PROJECT_NAME}-checkpatch
>         + COMMENT "`checkpatch.pl' is not found, so
>         ${PROJECT_NAME}-checkpatch target is dummy"
>         + )
>         +endif()
>         +
>         +add_custom_target(check
>         + DEPENDS ${PROJECT_NAME}-checkpatch ${PROJECT_NAME}-luacheck
>         +)
>         +
>
>     As I have already said offline, I think we should include the
>     `check` target as a dependency to the `test` target, just like it
>     is currently done for the luacheck. It is much more convenient for
>     local testing that way.
>
Fixed.
>
>          set(LUAJIT_TEST_COMMAND "${LUAJIT_TEST_BINARY} -e
>         dofile[[${LUAJIT_TEST_INIT}]]")
>          separate_arguments(LUAJIT_TEST_COMMAND)
>
>         --
>         2.34.1
>
>     [1]:
>     https://github.com/tarantool/checkpatch/blob/master/.github/actions/checkpatch/action.yml
>     --
>     Best regards,
>     Maxim Kokryashkin
>

[-- Attachment #2: Type: text/html, Size: 13372 bytes --]

Re: [Tarantool-patches] [PATCH 4/4][v2] ci: enable checkpatch

[Sergey Bronnikov via Tarantool-patches]

[-- Attachment #1: Type: text/plain, Size: 1941 bytes --]

Hi, Max!

thanks for your review!


On 7/17/23 21:48, Maxim Kokryashkin via Tarantool-patches wrote:
> Hi, Sergey!
> Thanks for the patch!
> Please consider my comments below.
>
>         From: Sergey Bronnikov <sergeyb@tarantool.org
>         </compose?To=sergeyb@tarantool.org>>
>
>         Patch enables running checkpatch [1] for checking patch on a
>         pre-commit
>
>     The pre-commit stage makes no sense when we are talking about CI — the
>     commit has already occurred. That’s just a conventional workflow
>     configuration.
>
In my mind "pre-commit" means commit to a target branch, in our case it 
is a "tarantool/master".
>
>     Having that in mind, along with some grammar related issues. I suggest
>     rephrasing it like:
>     | Patch adds a CI workflow with the checkpatch[1] run.
>
Updated.
>
>         stage.
>
>         1. https://github.com/tarantool/checkpatch
>         ---
>          .github/actions/checkpatch/action.yml | 11 +++++++++++
>          .github/workflows/lint.yml | 20 ++++++++++++++++++++
>          2 files changed, 31 insertions(+)
>          create mode 100644 .github/actions/checkpatch/action.yml
>
>         diff --git a/.github/actions/checkpatch/action.yml
>         b/.github/actions/checkpatch/action.yml
>         new file mode 100644
>         index 00000000..2336fb15
>         --- /dev/null
>         +++ b/.github/actions/checkpatch/action.yml
>         @@ -0,0 +1,11 @@
>         +name: Checkpatch
>         +description: Check patches against LuaJIT development guidelines
>
>     I think it’s better to change the action name to `Setup checkpatch`,
>     since the actual run is not performed here.
>     The description should be updated correspondingly. Also, AFAIK, there
>     is no such thing as `LuaJIT guidelines`, so maybe it’s better to
>     change it
>     to `Tarantool guidelines` if you want to keep that part.
>
Updated.


<snipped>

[-- Attachment #2: Type: text/html, Size: 4925 bytes --]

Re: [Tarantool-patches] [PATCH 3/4][v2] cmake: introduce 'check' and 'LuaJIT-checkpatch' targets

[Maxim Kokryashkin via Tarantool-patches]

[-- Attachment #1: Type: text/plain, Size: 4547 bytes --]


Hi!
Thanks for the fixes!
LGTM now. As for the master branch name,
your comment is fair enough, I just wanted to ensure
that our solution is robust enough.
 
--
Best regards,
Maxim Kokryashkin
 
 
> 
>>Hi, Max!
>>Thanks for your comments!
>>See my answers below. Updated patch was force-pushed.
>> 
>>On 7/17/23 19:10, Maxim Kokryashkin via Tarantool-patches wrote:
>>>Hi, Sergey!
>>>Please consider my comments below.
>>> 
>>>> 
>>>>>From: Sergey Bronnikov < sergeyb@tarantool.org >
>>>>>
>>>>>In Tarantool we use our own fork of checkpatch [2] with additional check
>>>>>types. It's logical to use it in a LuaJIT development. We don't need
>>>>Typo: s/in a/in/ 
>>Fixed.
>>>>>check tags in commit messages like NO_DOC, NO_CHANGELOG, NO_TEST and
>>>>>others, so to be able to customize command-line options Github Action, provided
>>>>>by checkpatch repository [3], was added to the repository.
>>>>Typo: s/by checkpatch/by the checkpatch/ 
>>Fixed.
>>>>>
>>>>>See documentation for used checkpatch in [4].
>>>>Typo: s/documentation/the documentation/
>>>>Typo: s/for used/for the/ 
>>Fixed. Fixed.
>>>>>
>>>>>Patch introduce new CMake targets: LuaJIT-checkpatch, that checks
>>>>Typo: s/introduce/introduces/ 
>>Fixed.
>>>>>patches on top of master branch using script checkpatch.pl [1], and
>>>>Typo: s/on top of/on top of the/ 
>>Fixed.
>>>>>target check, that combines LuaJIT-luacheck and LuaJIT-checkpatch.
>>>>>
>>>>>1.  https://docs.kernel.org/dev-tools/checkpatch.html
>>>>>2.  https://github.com/tarantool/checkpatch
>>>>>3.  https://github.com/tarantool/checkpatch/blob/master/.github/actions/checkpatch/action.yml
>>>>>4.  https://github.com/tarantool/checkpatch/blob/master/doc/checkpatch.rst
>>>>Nit: It is kinda strange to see link [1] going after the link [4] in the commit message.
>>>>I think, it generally looks clearer, when they are ordered, but that’s a matter of taste.
>>>>Feel free to ignore. 
>>Rewrote description and fixed order of references.
>>>>>
>>>>>--- test/CMakeLists.txt | 33 +++++++++++++++++++++++++++++++++
>>>>> 1 file changed, 33 insertions(+)
>>>>>
>>>>>diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
>>>>>index 47296a22..ccbad035 100644
>>>>>--- a/test/CMakeLists.txt
>>>>>+++ b/test/CMakeLists.txt
>>>>>@@ -42,6 +42,39 @@ else()
>>>>>   )
>>>>> endif()
>>>>> 
>>>>>+find_program(CHECKPATCH checkpatch.pl
>>>>>+ HINTS ${PROJECT_SOURCE_DIR}/checkpatch)
>>>>>+if(CHECKPATCH)
>>>>I don’t really like that `MASTER_BRANCH` name is hardcoded. I think
>>>>it’s possible to implement it similarly to how it’s done in the `tarantool/checkpatch`
>>>>github action[1] with revision range. Or, at least, it is for sure possible to obtain
>>>>the master branch name dynamically.
>>In LuaJIT we have a single branch for merging new patches - tarantool/master.
>>Why do you need to redefine master branch?
>>>>>+ set(MASTER_BRANCH "tarantool/master")
>>>>>+ add_custom_target(${PROJECT_NAME}-checkpatch)
>>>>>+ add_custom_command(TARGET ${PROJECT_NAME}-checkpatch
>>>>>+ COMMENT "Running checkpatch"
>>>>>+ COMMAND
>>>>>+ ${CHECKPATCH}
>>>>>+ --codespell
>>>>>+ --color=always
>>>>>+ --git ${MASTER_BRANCH}..HEAD
>>>>>+ --ignore COMMIT_LOG_LONG_LINE
>>>>>+ # Requires at least two lines in commit message and this
>>>>>+ # is annoying.
>>>>>+ --ignore COMMIT_MESSAGE
>>>>>+ --ignore NO_CHANGELOG
>>>>>+ --ignore NO_DOC
>>>>>+ --ignore NO_TEST
>>>>>+ --show-types
>>>>>+ WORKING_DIRECTORY ${PROJECT_SOURCE_DIR}
>>>>>+ )
>>>>>+else()
>>>>>+ add_custom_target(${PROJECT_NAME}-checkpatch)
>>>>It seems like the target definition can be moved out of the `if` statement
>>>>just before it. 
>>Done. As well as definition of MASTER_BRANCH variable.
>>>>>+ add_custom_command(TARGET ${PROJECT_NAME}-checkpatch
>>>>>+ COMMENT "`checkpatch.pl' is not found, so ${PROJECT_NAME}-checkpatch target is dummy"
>>>>>+ )
>>>>>+endif()
>>>>>+
>>>>>+add_custom_target(check
>>>>>+ DEPENDS ${PROJECT_NAME}-checkpatch ${PROJECT_NAME}-luacheck
>>>>>+)
>>>>>+
>>>>As I have already said offline, I think we should include the `check` target as a dependency to the `test` target, just like it is currently done for the luacheck. It is much more convenient for local testing that way. 
>>Fixed.
>>>>> set(LUAJIT_TEST_COMMAND "${LUAJIT_TEST_BINARY} -e dofile[[${LUAJIT_TEST_INIT}]]")
>>>>> separate_arguments(LUAJIT_TEST_COMMAND)
>>>>> 
>>>>>--
>>>>>2.34.1
>>>>[1]:  https://github.com/tarantool/checkpatch/blob/master/.github/actions/checkpatch/action.yml
>>>>--
>>>>Best regards,
>>>>Maxim Kokryashkin
> 

[-- Attachment #2: Type: text/html, Size: 9374 bytes --]

Re: [Tarantool-patches] [PATCH 4/4][v2] ci: enable checkpatch

[Maxim Kokryashkin via Tarantool-patches]

[-- Attachment #1: Type: text/plain, Size: 1888 bytes --]


Hi!
Thanks for the fixes!
LGTM
--
Best regards,
Maxim Kokryashkin
 
 
> 
>>Hi, Max!
>>thanks for your review!
>> 
>>On 7/17/23 21:48, Maxim Kokryashkin via Tarantool-patches wrote:
>>>Hi, Sergey!
>>>Thanks for the patch!
>>>Please consider my comments below.
>>> 
>>>> 
>>>>>From: Sergey Bronnikov < sergeyb@tarantool.org >
>>>>>
>>>>>Patch enables running checkpatch [1] for checking patch on a pre-commit
>>>>The pre-commit stage makes no sense when we are talking about CI — the
>>>>commit has already occurred. That’s just a conventional workflow configuration.
>>>>  
>>In my mind "pre-commit" means commit to a target branch, in our case it is a "tarantool/master".
>>>>Having that in mind, along with some grammar related issues. I suggest
>>>>rephrasing it like:
>>>>| Patch adds a CI workflow with the checkpatch[1] run. 
>>Updated.
>>>>>stage.
>>>>>
>>>>>1.  https://github.com/tarantool/checkpatch
>>>>>---
>>>>> .github/actions/checkpatch/action.yml | 11 +++++++++++
>>>>> .github/workflows/lint.yml | 20 ++++++++++++++++++++
>>>>> 2 files changed, 31 insertions(+)
>>>>> create mode 100644 .github/actions/checkpatch/action.yml
>>>>>
>>>>>diff --git a/.github/actions/checkpatch/action.yml b/.github/actions/checkpatch/action.yml
>>>>>new file mode 100644
>>>>>index 00000000..2336fb15
>>>>>--- /dev/null
>>>>>+++ b/.github/actions/checkpatch/action.yml
>>>>>@@ -0,0 +1,11 @@
>>>>>+name: Checkpatch
>>>>>+description: Check patches against LuaJIT development guidelines
>>>>I think it’s better to change the action name to `Setup checkpatch`,
>>>>since the actual run is not performed here.
>>>>The description should be updated correspondingly. Also, AFAIK, there
>>>>is no such thing as `LuaJIT guidelines`, so maybe it’s better to change it
>>>>to `Tarantool guidelines` if you want to keep that part. 
>>Updated.
>> 
>><snipped>
>>>> 
> 

[-- Attachment #2: Type: text/html, Size: 3786 bytes --]