MatrixAI / js-db

Key-Value DB for TypeScript and JavaScript Applications
https://polykey.com
Apache License 2.0
5 stars 0 forks source link

ci: merge staging to master #38

Closed MatrixAI-Bot closed 2 years ago

MatrixAI-Bot commented 2 years ago

This is an automatic PR generated by the pipeline CI/CD. This will be automatically fast-forward merged if successful.

MatrixAI-Bot commented 2 years ago

Pipeline Attempt on 576896307 for 8fd0c9a539af1de0cfd3018ee947e0a08aa74c43

https://gitlab.com/MatrixAI/open-source/js-db/-/pipelines/576896307

ghost commented 2 years ago
👇 Click on the image for a new way to code review - Make big changes easier — review code in small groups of related files - Know where to start — see the whole change at a glance - Take a code tour — explore the change with an interactive tour - Make comments and review — all fully sync’ed with github [Try it now!](https://app.codesee.io/r/reviews?pr=38&src=https%3A%2F%2Fgithub.com%2FMatrixAI%2Fjs-db)

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map Legend

CMCDragonkai commented 2 years ago

Some things to verify:

  1. prerelease jobs
  2. release jobs
CMCDragonkai commented 2 years ago

Would be interesting to see the library prebuilds work, even for prereleases.

CMCDragonkai commented 2 years ago

Also the binding.gyp configuration can probably be tweaked. As well as the usage of submodules here for rocksdb and snappy.

CMCDragonkai commented 2 years ago

Consider bringing in the test load balancing here too from TypeScript-Demo-Lib. But that would need to first go into TypeScript-Demo-Lib-Native.

CMCDragonkai commented 2 years ago

One intermittent failure:

  ● rocksdbP › database › transactions › transactionMultiGetForUpdate addresses write skew by promoting gets into same-value puts
    expect(received).toBe(expected) // Object.is equality
    Expected: true
    Received: false
      366 |             );
      367 |           }),
    > 368 |         ).toBe(true);
          |           ^
      369 |       });
      370 |       test('transactionIteratorInit iterates over overlay defaults to underlay', async () => {
      371 |         await rocksdbP.dbPut(db, 'K1', '100', {});
      at Object.<anonymous> (tests/rocksdb/rocksdbP.test.ts:368:11

This occurred on the feature branch, but tests pass on staging. Need to investigate.

Also running the build takes a long time. We should ensure that the build is being cached even for check:test. One of the issues is that npm run build runs prebuild which uses prebuildify, it doesn't do an incremental build, but instead recompiles from scratch using node-gyp rebuild. This takes far longer compared to node-gyp build.

We should change check:test to:

  1. Cache the build directory
  2. Change to using node-gyp build instead, (although first time runs, require node-gyp configure)

This may additionally be used by the build:* jobs as well, since compilation takes time, and it's best to cache it.

CMCDragonkai commented 2 years ago

This would mean not using prebuildify until you are about to do releases. Normal build using node-gyp would suffice.

CMCDragonkai commented 2 years ago

Windows build failed:

C:\GitLab-Runner\builds\MatrixAI\open-source\js-db\src\rocksdb\napi\database.h(11,10): fatal error C1083: Cannot open include file: 'node/node_api.h': No such file or directory [C:\GitLab-Runner\builds\MatrixAI\open-source\js-db\build\rocksdb.vcxproj]
Done Building Project "C:\GitLab-Runner\builds\MatrixAI\open-source\js-db\build\rocksdb.vcxproj" (default targets) -- FAILED.
Done Building Project "C:\GitLab-Runner\builds\MatrixAI\open-source\js-db\build\rocksdb.vcxproj.metaproj" (default targets) -- FAILED.
Done Building Project "C:\GitLab-Runner\builds\MatrixAI\open-source\js-db\build\binding.sln" (default targets) -- FAILED.
Build FAILED.
"C:\GitLab-Runner\builds\MatrixAI\open-source\js-db\build\binding.sln" (default target) (1) ->
"C:\GitLab-Runner\builds\MatrixAI\open-source\js-db\build\rocksdb.vcxproj.metaproj" (default target) (2) ->
"C:\GitLab-Runner\builds\MatrixAI\open-source\js-db\build\rocksdb.vcxproj" (default target) (5) ->
(ClCompile target) -> 
  C:\GitLab-Runner\builds\MatrixAI\open-source\js-db\src\rocksdb\napi\database.h(11,10): fatal error C1083: Cannot open include file: 'node/node_api.h': No such file or directory [C:\GitLab-Runner\builds\MatrixAI\open-source\js-db\build\rocksdb.vcxproj]
    0 Warning(s)
    1 Error(s)

Seems just a matter of making sure we have the right node headers.

MatrixAI-Bot commented 2 years ago

Pipeline Attempt on 576925886 for 6d37873b3515a99a6fc5ba0aba1dc67cdf769fe2

https://gitlab.com/MatrixAI/open-source/js-db/-/pipelines/576925886

CMCDragonkai commented 2 years ago

The macos build failed:

  c++ -o Release/obj.target/rocksdb/src/rocksdb/napi/batch.o ../src/rocksdb/napi/batch.cpp '-DNODE_GYP_MODULE_NAME=rocksdb' '-DUSING_UV_SHARED=1' '-DUSING_V8_SHARED=1' '-DV8_DEPRECATION_WARNINGS=1' '-DV8_DEPRECATION_WARNINGS' '-DV8_IMMINENT_DEPRECATION_WARNINGS' '-D_GLIBCXX_USE_CXX11_ABI=1' '-D_DARWIN_USE_64_BIT_INODE=1' '-D_LARGEFILE_SOURCE' '-D_FILE_OFFSET_BITS=64' '-DBUILDING_NODE_EXTENSION' -I/var/folders/lr/5gw1xrv53h1b_9mjjvs2k1dr0000gn/T/prebuildify/node/16.14.2/include/node -I/var/folders/lr/5gw1xrv53h1b_9mjjvs2k1dr0000gn/T/prebuildify/node/16.14.2/src -I/var/folders/lr/5gw1xrv53h1b_9mjjvs2k1dr0000gn/T/prebuildify/node/16.14.2/deps/openssl/config -I/var/folders/lr/5gw1xrv53h1b_9mjjvs2k1dr0000gn/T/prebuildify/node/16.14.2/deps/openssl/openssl/include -I/var/folders/lr/5gw1xrv53h1b_9mjjvs2k1dr0000gn/T/prebuildify/node/16.14.2/deps/uv/include -I/var/folders/lr/5gw1xrv53h1b_9mjjvs2k1dr0000gn/T/prebuildify/node/16.14.2/deps/zlib -I/var/folders/lr/5gw1xrv53h1b_9mjjvs2k1dr0000gn/T/prebuildify/node/16.14.2/deps/v8/include -I../node_modules/napi-macros -I../deps/rocksdb/rocksdb/include  -O3 -gdwarf-2 -fvisibility=hidden -mmacosx-version-min=10.13 -arch x86_64 -Wall -Wendif-labels -W -Wno-unused-parameter -std=gnu++14 -stdlib=libc++ -fno-rtti -fno-exceptions -std=c++17-arch x86_64 -arch arm64 -MMD -MF ./Release/.deps/Release/obj.target/rocksdb/src/rocksdb/napi/batch.o.d.raw   -c
clang: error: no such file or directory: 'x86_64'
make: *** [Release/obj.target/rocksdb/src/rocksdb/napi/batch.o] Error 1
gyp ERR! build error 
gyp ERR! stack Error: `make` failed with exit code: 2
gyp ERR! stack     at ChildProcess.onExit (/Users/gitlab/builds/MatrixAI/open-source/js-db/node_modules/node-gyp/lib/build.js:194:23)
gyp ERR! stack     at ChildProcess.emit (node:events:527:28)
gyp ERR! stack     at Process.ChildProcess._handle.onexit (node:internal/child_process:291:12)
gyp ERR! System Darwin 20.6.0
gyp ERR! command "/usr/local/Cellar/node@16/16.15.1/bin/node" "/Users/gitlab/builds/MatrixAI/open-source/js-db/node_modules/.bin/node-gyp" "rebuild" "--target=16.14.2" "--devdir=/var/folders/lr/5gw1xrv53h1b_9mjjvs2k1dr0000gn/T/prebuildify/node" "--arch=x64" "--release"
gyp ERR! cwd /Users/gitlab/builds/MatrixAI/open-source/js-db
gyp ERR! node -v v16.15.1
gyp ERR! node-gyp -v v9.0.0
gyp ERR! not ok 
CMCDragonkai commented 2 years ago

This requires debugging on the shared machines in the office.

CMCDragonkai commented 2 years ago

I think we will need something like https://docs.gitlab.com/ee/ci/yaml/#needspipelinejob in order to pull artifacts from parent pipeline to child pipeline. This can be done for the binary compiled native addons to be shared with all the sharded test jobs.

CMCDragonkai commented 2 years ago

I need to separate out the npm run build to do incremental node-gyp configure and node-gyp build, and then shared the native builds among the test jobs.

Right now npm run build runs prebuildify and that ends up doing node-gyp rebuild which rebuilds all the time. This is not good, as it takes 30 to 40 minutes to do compilation.

The dist is not needed for running tests, it's only needed for release and prerelease jobs. Will need to update this on TS demo lib native first, and then try it back on here.

We also need to verify that gitlab cache archives and persists the modification time of the cached data. Otherwise our incremental rebuild using make may not work.

According to https://stackoverflow.com/a/66146658/582917

The shared runners of Gitlab.com do seem to leave the file modification time intact when using a cache, and even on the main checkout.

So this idea should be feasible and then node-gyp build should just work.

Debug builds would be nice when debugging the tests on cicd, but then the issue is that the debug builds can't be used in the release/prerelease jobs, because they need the production build which would end up having more computation time needed.

CMCDragonkai commented 2 years ago

Ok pending tasks:

  1. Fix builds on Windows using matrix-win-1
  2. Fix builds on MacOS using matrix-win-1
  3. Optimise the native build process so it's not always running node-gyp rebuild, but instead node-gyp build while caching the build/ directory, expecting the gitlab CI/CD to maintain mtime metadata.
CMCDragonkai commented 2 years ago

When running on the shared machines, we need to make sure to clone recursively or initiate the submodule dependencies.

git submodule update --init --recursive

Note that on Windows, running node-gyp rebuild, it appears to generate additional files in the deps so git status appears dirty. But these additional files should not be committed. Just ignore them.

Untracked files:
  (use "git add <file>..." to include in what will be committed)
        deps/rocksdb/Release/
        deps/rocksdb/rocksdb.sln
        deps/rocksdb/rocksdb.vcxproj
        deps/rocksdb/rocksdb.vcxproj.filters
        deps/snappy/Release/
        deps/snappy/snappy.sln
        deps/snappy/snappy.vcxproj
        deps/snappy/snappy.vcxproj.filters

Setup for these dev machines should follow the same process as in the build:windows job.

CMCDragonkai commented 2 years ago

Replicated both errors locally now.

For windows, the problem is that it finishes building deps/rocksdb and deps/snappy, but then when it starts attempting to build src/rocksdb/napi, that's when it fails.

The fatal error is:

C:\Users\Roger Qiu\Projects\js-db\src\rocksdb\napi\database.h(11,10): fatal error C1083: Cannot open include file: 'node/node_api.h': No s

The node/node_api.h is the Node API header. So it seems the problem is that we cannot find the node API headers.

The node headers should have been downloaded by node-gyp at the beginning.

Now I noticed that in TS demo lib native, I used to do:

#include <node_api.h>

But in js-db I switched to #include <node/node_api.h>.

It is possible that in the windows headers downloaded, it doesn't put it into a subdirectory.

On Nix, I'm just using npm_config_nodedir, but on windows, this env variable is not set, instead I'm relying on node-gyp autodownloading the headers, because the node headers don't come with the default installation of nodejs on Windows (via the chocolatey package). Although I'm going to check again.

I also noticed that prebuildify is also installing their own headers. See https://github.com/prebuild/prebuildify/issues/64


I realised one reason why compilation is so slow on Windows/Mac is that they are not using all cores. Adding in a local .npmrc with:

# Enables npm link
prefix=~/.npm
# Use all cores during node-gyp compilation
jobs=max

Should make this the case across the board. This should be added into the demo libs.

CMCDragonkai commented 2 years ago

Yea so basically the issue this.

Firstly the windows official installation of NodeJS which is an MSI installer does not install the NodeJS headers. So the headers do not exist in C:\Program Files\nodejs\.

Secondly, by default node-gyp configure will download headers to:

C:\Users\Roger Qiu\AppData\Local\node-gyp\Cache\

See: https://stackoverflow.com/questions/50745670/nodejs-headers-on-windows-are-not-installed-automatically

By default it will download headers according to the currently executed node version that executes the node-gyp command. This is 16.14.2 which is why the actual headers are in:

C:\Users\Roger Qiu\AppData\Local\node-gyp\Cache\16.14.2\include

Now the headers are actually downloaded from: https://nodejs.org/dist/v16.14.2/ on the fly.

Note that there's a node-v16.14.2-headers.tar.gz.

That's only the headers though. It is also necessary to have the node.lib file, these are downloaded separately as well. This file comes from https://nodejs.org/dist/v16.14.2/win-x64/, and you need one for each type of architecture that you need to compile.

On windows, shared objects are .dll (as opposed to .so on unix), and static libraries are .lib. So node.lib is a static library, that is statically linked to any native addon (which on windows should end up producing a .dll, but nodejs renames this to a common extension being .node). This appears unique to windows, requiring a static nodejs library to link to.

In Windows, dll files (dynamically linked libraries) need to be in the same directory as the application or on the search path. lib files (static libraries) need to be statically linked during linking (the last step of building the application). It's common in Windows so have a library come with both a dll and lib file. In this case, the lib file is an import library containing the information needed to easily link to the dll.

After going through some issues on nodejs (https://github.com/nodejs/node/pull/41850), I found that the nodejs source is capable of producing (on windows):

On linux, both shared libraries and dynamic libraries have the same format, often called .so. Shared libraries are specified at compile time, but dynamic libraries are loaded at runtime.

Thirdly when we run npm run build, this actually runs prebuildify, which ends up getting its own cache due to some "conflict" between electron and nodejs https://github.com/prebuild/prebuildify/issues/64.

When we do npm install, this actually calls node-gyp-build which runs node-gyp rebuild, not prebuildify. The prebuildify is only used for npm run build. And this is because prebuildify toolchain is used to prebuild precompiled binaries and ship them in the npm package that can be installed. On the downstream machines, they are meant to use the precompiled binaries or compile from source on their system. Which is why we have challenges with using pkg to bundle them properly.

Now prebuildify's cache is in C:\Users\Roger Qiu\AppData\Local\Temp\prebuildify\.

Compare:

PS C:\Users\Roger Qiu\Projects\js-db> ls 'C:\Users\Roger Qiu\AppData\Local\Temp\prebuildify\node\16.14.2'

    Directory: C:\Users\Roger Qiu\AppData\Local\Temp\prebuildify\node\16.14.2

Mode                 LastWriteTime         Length Name
----                 -------------         ------ ----
d-----        12/05/2022   9:24 PM                arm64
d-----        12/05/2022   9:24 PM                ia32
d-----        12/05/2022   9:24 PM                include
d-----        12/05/2022   9:24 PM                x64
------        12/05/2022   9:24 PM              2 installVersion

PS C:\Users\Roger Qiu\Projects\js-db> ls 'C:\Users\Roger Qiu\AppData\Local\node-gyp\Cache\16.14.2'

    Directory: C:\Users\Roger Qiu\AppData\Local\node-gyp\Cache\16.14.2

Mode                 LastWriteTime         Length Name
----                 -------------         ------ ----
d-----        12/05/2022   8:32 PM                arm64
d-----        12/05/2022   8:32 PM                ia32
d-----        12/05/2022   8:32 PM                include
d-----        12/05/2022   8:32 PM                x64
-a----        12/05/2022   8:32 PM              2 installVersion

They are the same. And also prebuildify defaults on getting the latest version, rather than the current version like node-gyp. We get around this by explicitly setting the --target option in our prebuild script in package.json.

This is not ideal, we should have these dependencies specified explicitly during our build process to avoid surprises like the above...

It seems that if we were to specify the npm_config_nodedir environment variable, this should be sufficient to prevent node-gyp or prebuildify from attempting to download their own set of headers and the node.lib.

Then if we were able to download it our selves, we can then properly set the npm_config_nodedir path such that #include <node/node_api.h> will be includable and not require us to use #include <node_api.h> specifically for Windows.

In particular the proper path for inclusion should have been:

C:\Users\Roger Qiu\AppData\Local\Temp\prebuildify\node\16.14.2/include
MatrixAI-Bot commented 2 years ago

Pipeline Attempt on 585753964 for fd749a153d3c47777d1fc569d186923b017a2220

https://gitlab.com/MatrixAI/open-source/js-db/-/pipelines/585753964

CMCDragonkai commented 2 years ago

In powershell variables are automatically exported when set to $env.

But that means one has to remove them from $env like:

Remove-Item Env:\npm_config_nodedir

Afterwards, there's no temporary setting of env variables.

So:

$env:X = "hello world"; do-something
Remove-Item Env:\X
CMCDragonkai commented 2 years ago

As for doing the download ourselves manually it appears quite easy to do this. Rather than writing our own script, we can just use what node-gyp already does. This means something like:

npm_config_devdir=./tmp/node-gyp node-gyp install --target=v16.14.2

This ends up downloading the headers and also node.lib if necessary to the designated devdir location.

By default it will use whatever is the currently executed version. But the --target overrides it.

Now in order to maintain that, we must then use that location as the npm_config_nodedir.

CMCDragonkai commented 2 years ago

This would be the full command:

node-gyp install --target=v16.14.2 --devdir=./tmp/node-gyp --ensure
CMCDragonkai commented 2 years ago

So this demonstrates that we can can control the nodedir now.

rmdir '~/AppData/Local/Temp/prebuildify/node' -Recurse
rmdir '~/AppData/Local/node-gyp/Cache' -Recurse
node-gyp install --target=v16.14.2 --devdir=./tmp/node-gyp --ensure
$env:npm_config_nodedir='./tmp/node-gyp/16.14.2'; npm run build
Remove-Item Env:\npm_config_nodedir --verbose

So now we bypass both prebuildify and node-gyp caching, and we can even cache these headers ourselves.

As for nix builds, it still has to use the pkgs.nodejs to avoid downloading in the middle of nix-build.

CMCDragonkai commented 2 years ago

However unfortunately even after doing this, the #include <node/node_api.h> is still not legitmate.

I reckon we may need to swap back to #include <node_api.h>, as this is how it is documented. Maybe the windows toolchain is more strict on its include directories. I do notice that perhaps a reason why it works fine on NixOS, is that it adds -isystem /nix/store/zl4bvsqfxyx5vn9bbhnrmbmpfvzqj4gd-nodejs-16.14.2/include into the flags, but this does not happen with the windows builds.

CMCDragonkai commented 2 years ago

Yea it appears that -isystem is done because Nix is supplying gcc.

Then I wonder why in mac it works...

CMCDragonkai commented 2 years ago

Note that all npm_config_* variables must be lowercase. So I'm also changing:

  # Cache .npm
  npm_config_cache: "${CI_PROJECT_DIR}/tmp/npm"
  # Prefer offline node module installation
  npm_config_prefer_offline: "true"

Will need to be done across the board @emmacasolin @tegefaulkes

CMCDragonkai commented 2 years ago

I think by setting npm_config_nodedir it overrides any usage of --target, and therefore it's not necessary to specify it.

Ok so the remaining issue is that the jobs for windows and macos have to specify the relevant versions for nodejs and python. This is because these are outside of the nix system, and therefore they have to be specified out-of-band.

In this case, we are specifying it in CI/CD jobs for now.

CMCDragonkai commented 2 years ago

Ok I've managed to fix the not being able to find the node_api.h... but now a new problem.

On the windows system, upon setting npm_config_nodedir, it appears to to try to bind to:

$npm_config_nodedir\Release\node.lib

This doesn't quite make sense..., the downloads are supposed to put the node.lib in the relevant architecture... but this is no longer the case.

I wonder if the problem is that by specifying nodedir, it now thinks that I'm pointing it to the actual source of nodejs, and therefore thinks the node.lib is in $npm_config_nodedir/Release/node.lib (which would be the case if it was in fact compiled location).

But instead I'm pointing this to an installed location that already exists so I can control the installation location.

CMCDragonkai commented 2 years ago

However... if I don't specify nodedir, but only do something like this:

$Env:npm_config_target='16.14.2'
$Env:npm_config_devdir='./tmp/node-gyp'
node-gyp install --ensure
node-gyp configure --verbose
node-gyp build --verbose

Then it appears to work, it actually properly loads the right architecture.

But what about prebuildify...? Can we retain incremental computation, and make it not get its own headers?

If prebuildify can't do incremental computation, then it's sort of useless here. And we should replace prebuildify with our own prebuildify that just uses node-gyp directly and then puts stuff in the prebuilds directory in the right format that node-gyp-build expects.

CMCDragonkai commented 2 years ago

So to first figure out the node-gyp build to see if it works, then to figure out whether we still use prebuildify.

CMCDragonkai commented 2 years ago

New bug with node-gyp build.

LINK : fatal error LNK1149: output filename matches input filename 'C:\Users\Roger Qiu\Projects\js-db\build
\Release\rocksdb.lib' [C:\Users\Roger Qiu\Projects\js-db\build\rocksdb.vcxproj]
Done Building Project "C:\Users\Roger Qiu\Projects\js-db\build\rocksdb.vcxproj" (default targets) -- FAILED
.

Done Building Project "C:\Users\Roger Qiu\Projects\js-db\build\rocksdb.vcxproj.metaproj" (default targets)
-- FAILED.

Done Building Project "C:\Users\Roger Qiu\Projects\js-db\build\binding.sln" (default targets) -- FAILED.

Build FAILED.

"C:\Users\Roger Qiu\Projects\js-db\build\binding.sln" (default target) (1) ->
"C:\Users\Roger Qiu\Projects\js-db\build\rocksdb.vcxproj.metaproj" (default target) (2) ->
"C:\Users\Roger Qiu\Projects\js-db\build\rocksdb.vcxproj" (default target) (5) ->
(Link target) ->
  LINK : fatal error LNK1149: output filename matches input filename 'C:\Users\Roger Qiu\Projects\js-db\bui
ld\Release\rocksdb.lib' [C:\Users\Roger Qiu\Projects\js-db\build\rocksdb.vcxproj]

    0 Warning(s)
    1 Error(s)

Time Elapsed 00:00:23.53
MatrixAI-Bot commented 2 years ago

Pipeline Attempt on 585906987 for 5b43d9646a2e048ed30a87b691561c257783f51f

https://gitlab.com/MatrixAI/open-source/js-db/-/pipelines/585906987

CMCDragonkai commented 2 years ago

The new 22.05 revision means that we are now using 16.15.0 as the node version.

We need to update all of the pkg-fetch releases. @emmacasolin Can you do an update for js-polykey and ts-demo-lib?

CMCDragonkai commented 2 years ago

Some bug discoveries:

So basically we need to use npm_config_jobs=max as env variable, and --msvs_version=max as a parameter.

The only thing guaranteed to work is the CLI parameters. The .npmrc is not reliable at all here.

CMCDragonkai commented 2 years ago

Ok I've gone through the prebuildify code. It does in fact hard code the arguments of: node-gyp rebuild. In fact it forces a --target and --devdir (which is ignored when npm_config_nodedir is set) as well as --arch=x64 and --release.

This means if we want to do incremental compilation, we cannot be using prebuildify.

Since it is quite complicated magic, I'm going to be removing prebuildify, and replacing it with just a simpler output by copying what's in the ./build and pushing it to the ./prebuilds directory based on the structure expected of node-gyp-build.

It seems we just need:

prebuilds/
  darwin-x64+arm64/node.napi.node
  linux-x64/node.napi.node
  win32-x64/node.napi.node

Will need to verify what exactly is the structure, but it appears to be the process.platform, then dash, then process.arch but with x64+arm64 for universal binary, and then finally node.napi.node.

CMCDragonkai commented 2 years ago

The failure in the windows build might be do with the fact that the name of the native is called rocksdb, while there is a dependency with the same name. That is both binding.gyp and deps/rocksdb/rocksdb.gyp have the name rocksdb.

This isn't a problem on Linux because it uses rocksdb.a to mean the static library from deps/rocksdb, and rocksdb.node for the binding.gyp.

But it appears that on Windows, it is using rocksdb.lib instead of .a, and maybe there's a conflict here...

So I'm going to change the name of binding.gyp target_name to be just db.

CMCDragonkai commented 2 years ago

I'm attempting some changes on feature-no-prebuildify. By removing the prebuildify, our build will need a special scripts/prebuild.js., that replaces prebuildify. It should do its build incrementally with:

node-gyp configure --msvs_version=2019
node-gyp build --jobs=max",

Compensate for the option bugs that node-gyp has.

Document all the weird behaviour.

And create the relevant prebuilds directory after compilation.

The main reason tsc isn't doing incremental compilation is because it doesn't track removed files. So that's why we use rimraf ./dist. Otherwise the dist may have previously compiled files that is now removed. Track this https://github.com/microsoft/TypeScript/issues/30602.

However as long as the CI/CD tracks the mtimes, while caching ./build, it should be sufficient to proceed.

CMCDragonkai commented 2 years ago

After renaming to db, it works. No more name conflict, windows compilation succeeds.

CMCDragonkai commented 2 years ago

Ok examining the nodeGypBuild.path function I have found the expected structure:

prebuilds/
  <os.platform()>-<os.arch()>(?:\+<os.arch()>)*/
    <tag>(\.<tag>)*\.node

Where the <tag> can be: <runtime>|napi|abi\d+|uv\d+|armv\d+|glibc|musl.

And <runtime> can be node | electron | node-webkit.

Note that it is actually mandatory for there to be an napi|abi\d+. The napi is for ABI backwards/forwards compatibility, while the abi\d+, is for specific process.versions.module.

The tags can be in any order. The \.node is the extension, it must .node.

CMCDragonkai commented 2 years ago

In our case, it's only relevant to tag the binary if we want to extend our specificity. The default specificity is just to say node and napi which results in node.napi.node.

The arch is interesting because we will need to pass the --arch into the node-gyp which should be x64+arm64.

If the arch is not specified, it defaults to the host platform.

So our ./scripts/prebuild.js can just do something like this:

  1. For platform, always use os.platform(), there's no cross compilation for node-gyp anyway.
  2. For architecture, this can take the default os.arch(), or if specified with --arch, which is also passed to node-gyp configure and node-gyp build.
  3. As for the rest of the tags, we will just fix to node.napi for now. We won't have different runtimes atm between node and electron. The NAPI is compatible between the 2. And none of the other flags are relevant yet.

Future builds which precompile for android or ios may require further tags. In which case the os.platform() may change, and the runtime may also be different, like jsc because react native uses javascript core.

CMCDragonkai commented 2 years ago

Note that now that we have a scripts/prebuild.js, the --nodedir has to be set when building within nix. I did this previously in the shell.nix by setting npm_config_nodedir, but this isn't recognised by scripts/prebuild.js.

At the same time in the TS demo lib native, we have --nodedir being set in postInstall. So that would be propagated down to the relevant commands too.

We may need to recognise the npm_config_nodedir option.

Also because the prebuild script runs as prebuild to npm run build, it does not appear to receive options from just npm run build --nodedir=....

Which may mean env variable is the only way to do it.

https://stackoverflow.com/questions/61510927/passing-cli-args-to-pre-command-in-package-json

Which would mean that TS demo lib native has an incorrect line in the utils.nix.

CMCDragonkai commented 2 years ago

Ok now the script takes into account env variables. We can make use of this now.

CMCDragonkai commented 2 years ago

Ok there's still a problem with running npm run build on Windows. The reason is this:

The problem is mainly that the PATHEXT is not consulted when execFileSync (and therefore spawnSync) is used. It's only consulted when we use execSync or enable the shell option which causes the command to be executed while in a CMD shell. https://github.com/nodejs/node/issues/10302

This means when we try to execute node-gyp as we normally do in our execFileSync, it ends up with an ENOENT error because it cannot find it.

However if I add the .cmd extension to form node-gyp.cmd, then it does find the file and executes it properly.

CMCDragonkai commented 2 years ago

Rather than bringing in cross-spawn which I think we actually use in PK (@emmacasolin please confirm), a quick solution is to enable the usage of shell: true option which will just run the program with an intermediate shell.

Alternatively we can add the .cmd extension too when we detect we're on the windows platform, but I think it's easier to just enable the shell rather than hardcoding an extension of the command being executed.

CMCDragonkai commented 2 years ago

Ok so the windows build should work now on CI/CD.

Incremental compilation should still be ensured by integrating the caching of the ./build directory in https://docs.gitlab.com/ee/ci/caching/. We may wish to express this globally. This directory is reserved for native building. So no need to do any cache override in the specific build job.

On matrix-win-1, full compilation with all cores took about 7 minutes.

Mac failures still to be looked at. This may have to do with compilation flags.

MatrixAI-Bot commented 2 years ago

Pipeline Attempt on 587061594 for 1f6299d1c5c78c379a2587bdf113051eabd2eeaa

https://gitlab.com/MatrixAI/open-source/js-db/-/pipelines/587061594

CMCDragonkai commented 2 years ago

Also important to introduce the chocolatey caching that we have in js-polykey too.

emmacasolin commented 2 years ago

Ok there's still a problem with running npm run build on Windows. The reason is this:

The problem is mainly that the PATHEXT is not consulted when execFileSync (and therefore spawnSync) is used. It's only consulted when we use execSync or enable the shell option which causes the command to be executed while in a CMD shell. nodejs/node#10302

This means when we try to execute node-gyp as we normally do in our execFileSync, it ends up with an ENOENT error because it cannot find it.

However if I add the .cmd extension to form node-gyp.cmd, then it does find the file and executes it properly.

The ENOENT issue had already been brought up here https://github.com/MatrixAI/js-polykey/issues/401#issuecomment-1181364907, but I can add this additional context because I hadn't previously looked into the cause of it

CMCDragonkai commented 2 years ago

Yea this would only affect commands that are scripts and are not exes. So for PK that might affect our own scripts. But using an intermediate shell is not the solution for PK... I only enabled it here because it's just a script executing more scripts.

CMCDragonkai commented 2 years ago

Windows CI/CD builds succeeded: https://gitlab.com/MatrixAI/open-source/js-db/-/jobs/2716815439

However mac CI/CD builds still failing: https://gitlab.com/MatrixAI/open-source/js-db/-/jobs/2716815441