feat(developer): cross-platform kmx compiler 🚑

[SabineSIL]

This is my first draft of kmcompx with lots of my comments ( _S2... ) still included. All the tests of kmcompxtest.exe run Ok.

There are some functions that are not replaced yet because I couldn`t find replacements for non-windows platforms + using char16_t:

Compiler.cpp: LoadString FindResource LoadResource GetAsyncKeyState GetModuleFileName GetFileVersionInfoSize GetFileVersionInfo VerQueryValue

CheckFilenameConsistency.cpp: _wfindfirst _wsplitpath_s _wmakepath_s

[keymanapp-test-bot[bot]]

User Test Results

Test specification and instructions

User tests are not required

Test Artifacts

• Developer
  • Keyman Developer
  • Compiler Regression Tests
  • kmcomp.zip
• Keyboards
  • Test Keyboards
• Linux
  • Keyman for Linux
• macOS
  • Keyman for macOS
• Web
  • KeymanWeb Test Home
• Windows
  • Keyman for Windows
  • FirstVoices Keyboards for Windows

[SabineSIL]

• All tests in kmcompxtest.exe run OK (except for Vietnam, eKwTamil99UniUpdt).
• removed all comments (_S2)
• Replacement found for all functions in CheckFilenameConsistency.cpp
• Still no replacement found for functions in Compiler.cpp:

LoadString FindResource LoadResource GetAsyncKeyState GetModuleFileName GetFileVersionInfoSize GetFileVersionInfo VerQueryValue

[mcdurdin]

LoadString FindResource LoadResource

I think these are all related to loading a string for reporting a compiler error. I think we should move these strings from Compiler.rc into a .cpp file, and then these should go away.

GetAsyncKeyState

This can be removed -- it's testing for the user pressing Escape -- we don't want to support that in future anyway (it's an undocumented feature). (future feature: If we want the user to be able to stop the compile, it should be done through a callback mechanism in the future.)

GetModuleFileName GetFileVersionInfoSize GetFileVersionInfo VerQueryValue

AddCompilerVersionStore calls GetFileVersion which calls these. This could be reduced down to:

#include "keymanversion.h"
...
  if ((msg = AddStore(fk, TSS_COMPILEDVERSION, KEYMAN_VersionWithTag_W)) != CERR_None) return msg;

GetModuleFileName

This is also used in the file search algorithm for NamedCodeConstants.cpp. For now, I would #ifdef this section for Windows-only and include a `//TODO: sort out how to find common includes in non-Windows platforms:

#ifdef _WINDOWS_
  // Finally look in kmcmpdll.dll directory
  GetModuleFileName(0, buf, 260);
  char *p = strrchr(buf, '\\'); if(p) p++; else p = buf;
  *p = 0;
  strncat_s(buf, _countof(buf), filename, 259-strlen(buf)); buf[259] = 0;  // I3481   // I3641
  if(FileExists(buf))
    return IntLoadFile(buf);
#endif

That also reminds me, we need to look for all path management functions where we use \\ and think about / as well.

[SabineSIL]

That also reminds me, we need to look for all path management functions where we use \\ and think about / as well.

use #if defined(_WIN32) || defined(_WIN64) for all (used) occurrences of '\' in Compiler.cpp, CheckFileNameConsistency.cpp

[SabineSIL]

This is also used in the file search algorithm for NamedCodeConstants.cpp. For now, I would #ifdef this section for Windows-only and include a `//TODO: sort out how to find common includes in non-Windows platforms:

Use inside a #ifdef WINDOWS now

[SabineSIL]

AddCompilerVersionStore calls GetFileVersion which calls these. This could be reduced down to:

• removed AddCompilerVersionStore() and GetFileVersion();

• introduced #define KEYMAN_VersionWin_W16 u"$VersionWin" in \windows\cpp\include\keymanversion_build.in since we need char16_t in AddStore()

• Use of KEYMAN_VersionWin_W16 in

if ((msg = AddStore(fk, TSS_COMPILEDVERSION, KEYMAN_VersionWithTag_W16)) != CERR_None) return msg;

[SabineSIL]

This can be removed -- it's testing for the user pressing Escape -- we don't want to support that in future anyway

....removed

[SabineSIL]

I think these are all related to loading a string for reporting a compiler error. I think we should move these strings from Compiler.rc into a .cpp file, and then these should go away.

Sorry, I do not understand what you mean.

[mcdurdin]

I think these are all related to loading a string for reporting a compiler error. I think we should move these strings from Compiler.rc into a .cpp file, and then these should go away.

Sorry, I do not understand what you mean.

If you take a look at Compiler.rc in the repository, you'll see it is filled with stringtables which are all the strings for the errors generated by the compiler. We need to copy those strings to a C++ source file (e.g. #define or const char* type of thing) and then find a way to match those up to the identifiers.

Something like this (needs more work -- e.g. load into a std::map on first use, but conceptually):

struct CompilerError {
  DWORD ErrorCode;
  const char* Text;
};

const struct CompilerError CompilerErrors[] = {
  { CERR_InvalidLayoutLine, "Invalid 'layout' command" },
  { 0, nullptr }
};

char *GetCompilerErrorString(DWORD code) {
  for(int i = 0; CompilerErrors[i].ErrorCode; i++) {
    if(CompilerErrors[i].ErrorCode == code) {
      return CompilerErrors[i].Text;
    }
  }
  return nullptr;
}

[SabineSIL]

LoadString FindResource LoadResource

If you take a look at Compiler.rc in the repository, you'll see it is filled with stringtables which are all the strings for the errors generated by the compiler. We need to copy those strings to a C++ source file (e.g. #define or const char* type of thing) and then find a way to match those up to the identifiers.

I introduced CompMsg.cpp/.h for handling the ErrorMessage Strings. => LoadString() in AddCompileMessage() is replaced using GetCompilerErrorString()

For FindResource(), LoadResource() in GetVersionInfo() I used #ifdef WINDOWS .

[SabineSIL]

Hi Marc, I committed kmcompxtest and some Tests. So kmcompx should be ready for you to review.

kmcompxtest I added code to include Tests for checking if the correct ErrorNumber is produced by kmcompxtest. Now kmcompxtest: (- checks kmn-Files that compile for accessibility, size, content ( if that's the case -> tests OK) )

• checks kmn-Files that compile for having a name that indicates they have Errors ( if that's the case -> tests FAIL)
• checks kmn-files that don`t compile whether Filename/ErrorCode correspond ( if that's the case -> tests OK)

Tests for checking the ErrorNumber are located in C:\Projects\keyman\keyman\common\test\keyboards\kmcompx_tests\source. C:\Projects\keyman\keyman\common\test\keyboards\kmcompx_tests\README.md explains the naming of Test-files.

Version Number: I had to change the Version numbers in 2 places. It is set to 15.0.270.0 in C:\Projects\keyman\keyman\VERSION.md. Since it was still 15.0.265.0 in C:\Projects\keyman\keyman\common\windows\cpp\include\keymanversion_build.h I changed it in that file also.

[SabineSIL]

I finished the comments on PR 6980 except for

• those we want to move to a new PR,
• the one on setup of a C++ templated set of filesystem functions
• the one on the change of \r\n -> \r

[SabineSIL]

A comment on namespaces: To make review easier I left the namespace at the position where the original kmcmp_... names were used even if that meant reopening a namespace several times. This will be changed after review.

[SabineSIL]

https://github.com/keymanapp/keyman/pull/6980#pullrequestreview-1234250948

Which might be real problems?

[ermshiperete]

The build failures:

Test-14.0 (Keyman - Linux)

[16:17:08 ]  52/57 045 - deadkey and context               OK       0.02 s
[16:17:08 ]  53/57 046 - deadkey and contextex             OK       0.01 s
[16:17:08 ]  54/57 047 - caps always off initially on      OK       0.02 s
[16:17:08 ]  55/57 048 - modifier keys keep context        OK       0.01 s
[16:17:08 ]  56/57 key_list                                FAIL     0.01 s (exit status 175 or signal 47 SIGinvalid)
[16:17:08 ]  57/57 imx_list                                FAIL     0.01 s (exit status 23)
[16:17:08 ]  
[16:17:08 ]  Ok:                   55
[16:17:08 ]  Expected Fail:         0
[16:17:08 ]  Fail:                  2
[16:17:08 ]  Unexpected Pass:       0
[16:17:08 ]  Skipped:               0
[16:17:08 ]  Timeout:               0
[16:17:08 ]  
[16:17:08 ]  
[16:17:08 ]  The output from the failed tests:
[16:17:08 ]  
[16:17:08 ]  56/57 key_list                                FAIL     0.01 s (exit status 175 or signal 47 SIGinvalid)
[16:17:08 ]  
[16:17:08 ]  --- command ---
[16:17:08 ]  15:17:08 /var/lib/TeamCity/agent/work/99b311828f4ee7c/keyman/linux/keyboardprocessor/arch/release/tests/unit/kmx/key_list /var/lib/TeamCity/agent/work/99b311828f4ee7c/keyman/core/tests/unit/kmx/kmx_key_list.kmx
[16:17:08 ]  --- stderr ---
[16:17:08 ]  Test failed with 7 at ../../../../core/tests/unit/kmx/kmx_key_list.cpp:58:
[16:17:08 ]    km_kbp_keyboard_load(full_path.native().c_str(), &test_kb)
[16:17:08 ]  -------
[16:17:08 ]  
[16:17:08 ]  57/57 imx_list                                FAIL     0.01 s (exit status 23)
[16:17:08 ]  
[16:17:08 ]  --- command ---
[16:17:08 ]  15:17:08 /var/lib/TeamCity/agent/work/99b311828f4ee7c/keyman/linux/keyboardprocessor/arch/release/tests/unit/kmx/imx_list /var/lib/TeamCity/agent/work/99b311828f4ee7c/keyman/core/tests/unit/kmx/kmx_imsample.kmx
[16:17:08 ]  --- stdout ---
[16:17:08 ]  3581736460  ../../../../core/src/kmx/kmx_context.cpp:77  Reset  KMX_Context::Reset
[16:17:08 ]  
[16:17:08 ]  3581736460  ../../../../core/src/kmx/kmx_file.cpp:116  LoadKeyboard  Loading file '/var/lib/TeamCity/agent/work/99b311828f4ee7c/keyman/core/tests/unit/kmx/kmx_imsample.kmx'
[16:17:08 ]  
[16:17:08 ]  3581736460  ../../../../core/src/kmx/kmx_file.cpp:130  LoadKeyboard  Could not open file
[16:17:08 ]  
[16:17:08 ]  --- stderr ---
[16:17:08 ]  Test failed with 7 at ../../../../core/tests/unit/kmx/kmx_imx.cpp:164:
[16:17:08 ]    km_kbp_keyboard_load(full_path.native().c_str(), &test_kb)
[16:17:08 ]  -------
[16:17:08 ]  
[16:17:08 ]  Full log written to /var/lib/TeamCity/agent/work/99b311828f4ee7c/keyman/linux/keyboardprocessor/arch/release/meson-logs/testlog.txt
[16:17:08 ]  make: *** [Makefile:19: tmpinstall] Error 2
[16:17:08 ]  Process exited with code 2
[16:17:08 ]  Process exited with code 2 (Step: Build (Command Line))
[16:17:08 ]  Step Build (Command Line) failed

Test-14.0 (Keyman - Windows)

[16:20:10 ]  
[16:20:10 ]  ------------------------------------------------------------------------------
[16:20:10 ]  
[16:20:10 ]  57/57 imx_list                              FAIL            0.03s   (exit status 16407 or signal 16279 SIGinvalid)
[16:20:10 ]  >>> MALLOC_PERTURB_=134 C:\BuildAgent\work\7ac43416c45637e9\keyman\core\build\x86\release\tests\unit\kmx\imx_list.exe C:/BuildAgent/work/7ac43416c45637e9/keyman/core/tests/unit/kmx/kmx_imsample.kmx
[16:20:10 ]  ------------------------------------- 8< -------------------------------------
[16:20:10 ]  stdout:
[16:20:10 ]  1096505311  ../../../src/kmx/kmx_context.cpp:77  km::kbp::kmx::KMX_Context::Reset  KMX_Context::Reset
[16:20:10 ]  
[16:20:10 ]  
[16:20:10 ]  
[16:20:10 ]  1096505311  ../../../src/kmx/kmx_file.cpp:116  km::kbp::kmx::KMX_ProcessEvent::LoadKeyboard  Loading file 'C'
[16:20:10 ]  
[16:20:10 ]  
[16:20:10 ]  
[16:20:10 ]  1096505312  ../../../src/kmx/kmx_file.cpp:130  km::kbp::kmx::KMX_ProcessEvent::LoadKeyboard  Could not open file
[16:20:10 ]  
[16:20:10 ]  
[16:20:10 ]  
[16:20:10 ]  stderr:
[16:20:10 ]  Test failed with 7 at ../../../tests/unit/kmx/kmx_imx.cpp:164:
[16:20:10 ]  
[16:20:10 ]    km_kbp_keyboard_load(full_path.native().c_str(), &test_kb)
[16:20:10 ]  
[16:20:10 ]  ------------------------------------------------------------------------------
[16:20:10 ]  
[16:20:10 ]  
[16:20:10 ]  Summary of Failures:
[16:20:10 ]  
[16:20:10 ]  56/57 key_list                              FAIL            0.03s   (exit status 5807 or signal 5679 SIGinvalid)
[16:20:10 ]  57/57 imx_list                              FAIL            0.03s   (exit status 16407 or signal 16279 SIGinvalid)
[16:20:10 ]  
[16:20:10 ]  
[16:20:10 ]  Ok:                 55
[16:20:10 ]  Expected Fail:      0
[16:20:10 ]  Fail:               2
[16:20:10 ]  Unexpected Pass:    0
[16:20:10 ]  Skipped:            0
[16:20:10 ]  Timeout:            0

The other build failures fail for the same reason.

[SabineSIL]

Aha, diese beiden Files sind bei mir plötzlich in Git aufgetaucht (wo die herkommen: ??) . Weil`s kmx-Files sind, und weil sie ja immer neu gebildet werden dachte ich mir, die sind nicht nötig, und hab sie gelöscht.

Soll ich sie committen/pushen? VG Sabine

From: Eberhard Beilharz @.*** Sent: Tuesday, 24 January 2023 15:55 To: keymanapp/keyman Cc: SabineSIL; Mention Subject: Re: [keymanapp/keyman] feat(developer): cross-platform kmx compiler 🚑 (PR #6980)

The build failures:

Test-14.0 (Keyman - Linux)

[16:17:08 ] 52/57 045 - deadkey and context OK 0.02 s [16:17:08 ] 53/57 046 - deadkey and contextex OK 0.01 s [16:17:08 ] 54/57 047 - caps always off initially on OK 0.02 s [16:17:08 ] 55/57 048 - modifier keys keep context OK 0.01 s [16:17:08 ] 56/57 key_list FAIL 0.01 s (exit status 175 or signal 47 SIGinvalid) [16:17:08 ] 57/57 imx_list FAIL 0.01 s (exit status 23) [16:17:08 ]
[16:17:08 ] Ok: 55 [16:17:08 ] Expected Fail: 0 [16:17:08 ] Fail: 2 [16:17:08 ] Unexpected Pass: 0 [16:17:08 ] Skipped: 0 [16:17:08 ] Timeout: 0 [16:17:08 ]
[16:17:08 ]
[16:17:08 ] The output from the failed tests: [16:17:08 ]
[16:17:08 ] 56/57 key_list FAIL 0.01 s (exit status 175 or signal 47 SIGinvalid) [16:17:08 ]
[16:17:08 ] --- command --- [16:17:08 ] 15:17:08 /var/lib/TeamCity/agent/work/99b311828f4ee7c/keyman/linux/keyboardprocessor/arch/release/tests/unit/kmx/key_list /var/lib/TeamCity/agent/work/99b311828f4ee7c/keyman/core/tests/unit/kmx/kmx_key_list.kmx [16:17:08 ] --- stderr --- [16:17:08 ] Test failed with 7 at ../../../../core/tests/unit/kmx/kmx_key_list.cpp:58: [16:17:08 ] km_kbp_keyboard_load(full_path.native().c_str(), &test_kb) [16:17:08 ] ------- [16:17:08 ]
[16:17:08 ] 57/57 imx_list FAIL 0.01 s (exit status 23) [16:17:08 ]
[16:17:08 ] --- command --- [16:17:08 ] 15:17:08 /var/lib/TeamCity/agent/work/99b311828f4ee7c/keyman/linux/keyboardprocessor/arch/release/tests/unit/kmx/imx_list /var/lib/TeamCity/agent/work/99b311828f4ee7c/keyman/core/tests/unit/kmx/kmx_imsample.kmx [16:17:08 ] --- stdout --- [16:17:08 ] 3581736460 ../../../../core/src/kmx/kmx_context.cpp:77 Reset KMX_Context::Reset [16:17:08 ]
[16:17:08 ] 3581736460 ../../../../core/src/kmx/kmx_file.cpp:116 LoadKeyboard Loading file '/var/lib/TeamCity/agent/work/99b311828f4ee7c/keyman/core/tests/unit/kmx/kmx_imsample.kmx' [16:17:08 ]
[16:17:08 ] 3581736460 ../../../../core/src/kmx/kmx_file.cpp:130 LoadKeyboard Could not open file [16:17:08 ]
[16:17:08 ] --- stderr --- [16:17:08 ] Test failed with 7 at ../../../../core/tests/unit/kmx/kmx_imx.cpp:164: [16:17:08 ] km_kbp_keyboard_load(full_path.native().c_str(), &test_kb) [16:17:08 ] ------- [16:17:08 ]
[16:17:08 ] Full log written to /var/lib/TeamCity/agent/work/99b311828f4ee7c/keyman/linux/keyboardprocessor/arch/release/meson-logs/testlog.txt [16:17:08 ] make: *** [Makefile:19: tmpinstall] Error 2 [16:17:08 ] Process exited with code 2 [16:17:08 ] Process exited with code 2 (Step: Build (Command Line)) [16:17:08 ] Step Build (Command Line) failed

Test-14.0 (Keyman - Windows)

[16:20:10 ]
[16:20:10 ] ------------------------------------------------------------------------------ [16:20:10 ]
[16:20:10 ] 57/57 imx_list FAIL 0.03s (exit status 16407 or signal 16279 SIGinvalid) [16:20:10 ] >>> MALLOC_PERTURB_=134 C:\BuildAgent\work\7ac43416c45637e9\keyman\core\build\x86\release\tests\unit\kmx\imx_list.exe C:/BuildAgent/work/7ac43416c45637e9/keyman/core/tests/unit/kmx/kmx_imsample.kmx [16:20:10 ] ------------------------------------- 8< ------------------------------------- [16:20:10 ] stdout: [16:20:10 ] 1096505311 ../../../src/kmx/kmx_context.cpp:77 km::kbp::kmx::KMX_Context::Reset KMX_Context::Reset [16:20:10 ]
[16:20:10 ]
[16:20:10 ]
[16:20:10 ] 1096505311 ../../../src/kmx/kmx_file.cpp:116 km::kbp::kmx::KMX_ProcessEvent::LoadKeyboard Loading file 'C' [16:20:10 ]
[16:20:10 ]
[16:20:10 ]
[16:20:10 ] 1096505312 ../../../src/kmx/kmx_file.cpp:130 km::kbp::kmx::KMX_ProcessEvent::LoadKeyboard Could not open file [16:20:10 ]
[16:20:10 ]
[16:20:10 ]
[16:20:10 ] stderr: [16:20:10 ] Test failed with 7 at ../../../tests/unit/kmx/kmx_imx.cpp:164: [16:20:10 ]
[16:20:10 ] km_kbp_keyboard_load(full_path.native().c_str(), &test_kb) [16:20:10 ]
[16:20:10 ] ------------------------------------------------------------------------------ [16:20:10 ]
[16:20:10 ]
[16:20:10 ] Summary of Failures: [16:20:10 ]
[16:20:10 ] 56/57 key_list FAIL 0.03s (exit status 5807 or signal 5679 SIGinvalid) [16:20:10 ] 57/57 imx_list FAIL 0.03s (exit status 16407 or signal 16279 SIGinvalid) [16:20:10 ]
[16:20:10 ]
[16:20:10 ] Ok: 55 [16:20:10 ] Expected Fail: 0 [16:20:10 ] Fail: 2 [16:20:10 ] Unexpected Pass: 0 [16:20:10 ] Skipped: 0 [16:20:10 ] Timeout: 0

The other build failures fail for the same reason.

— Reply to this email directly, view it on GitHub https://github.com/keymanapp/keyman/pull/6980#issuecomment-1402079523 , or unsubscribe https://github.com/notifications/unsubscribe-auth/AUVSGYZUFLEYUFOTX6EUPSTWT7UM7ANCNFSM54HZSDZQ . You are receiving this because you were mentioned.Image removed by sender.Message ID: @.***>

[ermshiperete]

yes, we have the .kmx files in the repo because not all environments can build them, so they should be committed.

[rc-swag]

have the .kmx files in the repo because not all environments can build them, so they should be committed.

@SabineSIL This got me originally also, I was like surely they don't belong in the repo.

[mcdurdin]

have the .kmx files in the repo because not all environments can build them, so they should be committed.

Good news is that this PR is a step towards fixing that!