fix(common): enable build for win x64, use global VERSION.md and fix decxstr() bug#3076
fix(common): enable build for win x64, use global VERSION.md and fix decxstr() bug#3076
Conversation
common/core/desktop/meson.build
Outdated
| # Fallback to "more" for Windows compatbility | ||
| version: run_command(find_program('cat', 'more'), | ||
| files('VERSION.md')).stdout().strip(), | ||
| files('../../../VERSION.md')).stdout().strip(), |
There was a problem hiding this comment.
I think @ermshiperete intentionally used 'VERSION.md' in #2843 to fix the failing Linux packaging builds
There was a problem hiding this comment.
Yes, I did. This fixes the Linux package build, but unfortunately breaks things everywhere else unless you (manually) copy VERSION.md... I'm working on a better solution that should work everywhere...
There was a problem hiding this comment.
I take it meson will choose the correct script in find_program based on the OS? Because the documentation is a little vague on that point.
There was a problem hiding this comment.
It takes the first it finds and can execute. On Windows (at least on my machine) it finds both .bat and .sh, but has problems executing .sh (runs without obvious error, but doesn't seem to DO anything) which is why I put .bat first. On Linux it ignores the .bat file.
`VERSION.md` is in the root of the source directory. However, when building a Linux package from a source package we can't access files from the root directory. Prior to creating the source package `VERSION.md` gets copied to `common/core/desktop` so that it is available in the source package. However, this hack broke the meson build everywhere else unless `VERSION.md` got copied manually. This change uses `VERSION.md` from the root directory if it is available and otherwise assumes that it exists in the same directory as the `meson.build` script. This change also switches to using scripts to get the version number rather than doing it in the meson build itself because that gives more flexibility. It is important to note that the Windows batch file has to be listed first in `find_program` because Windows might detect `getversion.sh` and tries to run it with bash but this fails because of the different directory separators.
|
Should https://github.com/keymanapp/keyman/blob/master/common/core/desktop/doc/BUILDING.md be updated to mention build.bat or x64? |
| { | ||
| for(PKMX_WCHAR q = incxstr(buf); *buf; buf = q, q = incxstr(buf)) | ||
| if(!u16ncmp(buf, chr, (int)(q-buf))) | ||
| if(!u16ncmp(buf, chr, (intptr_t)(q-buf))) |
There was a problem hiding this comment.
Does the Windows version (line 167) remain the same?
if(!wcsncmp(buf, chr, (int)(q-buf)))
There was a problem hiding this comment.
Good catch. I knew there was something else I needed to do. We should audit the rest of keyman32/keyman64 for use of improper casts.
It is unlikely to cause a problem presently because we currently have a base address fixed in the keyman64.dll below 2GB (which itself is not recommended...) so we should probably put this in as a maintenance issue. It could arise if Windows needs to relocate the dll I guess.
Keyman Core build on Windows x64
Keyman core was only being built for x86 on Windows. This builds also for x64.
Build Agents updated: I have updated the build scripts on TeamCity to reference
build.batinstead of callingmeson/ninjadirectly.build.bathas various parameters:x86- build x86 version of core (Windows) intobuild-x86/and testx64- build x64 version of core (Windows) intobuild-x64/and testall- build both versions and test-c- use afterx86orx64to leave your shell configured for VC++ for that target platformReference root
VERSION.mdmeson.buildnow references the rootVERSION.mdinstead of a copy in the local folder. This means we no longer need to copy it in during build. Build Agents updated accordingly.decxstr() bug
Importantly, this fixes a long-standing latent bug in
decxstr()where it could test before the start of the string. It always happened to work okay in test cases in the past, but failed in x64 tests because the filler bytes were0xFF...Obviously testing outside array bounds is a serious no-no. Hence fixed in both windows and core code.
Might be worth checking macos code at some point, although may be not necessary as intent is to replace it this release with Keyman Core.