ci: merge staging to master #38

MatrixAI-Bot · 2022-06-30T10:54:08Z

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

…napi/workers.cpp`

…for JS

* Applied snapshots to dbGet and dbMultiGet, changed dbGetMany to dbMultiGet * Snapshots work with transactionGet and transactionGetForUpdate * Testing repeatable read using snapshots * Turns out snapshot lifecycle is controlled by the transaction, so no manual release * Transactions now have their own priority work count * Testing dbClose will automatically close of snapshots, iterators and transactions

* Enabled `NODE_DEBUG_NATIVE` runtime control of rocksdb logging * Testing transaction iterator

be called when the detached target is already closed. So I've removed the precondition of `Detach+` methods and `DecrementPendingWork` from both `Database` and `Transaction` classes.

… be checking for `pendingWork_ == 1`

* Fixed missing promisify for `transactionClear` * Using `__func__` for log messages * Namespacing C++ iterator workers * Testing `transactionClear` with non-repeatable read

…transactions

…t demonstrating race condition thrashing

Introduce Snapshot Isolation OCC to DBTransaction

MatrixAI-Bot · 2022-06-30T10:54:10Z

Pipeline Attempt on 576896307 for 8fd0c9a

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

ghost · 2022-06-30T10:54:47Z

👇 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!

Legend

CMCDragonkai · 2022-07-12T15:44:22Z

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 · 2022-07-12T15:48:10Z

Some bug discoveries:

The msvs_version option doesn't work from .npmrc nor from environment variable nodejs/node-gyp#2698
The --jobs option doesn't work from .npmrc nodejs/node-gyp#2699

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 · 2022-07-12T16:04:59Z

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 · 2022-07-12T16:27:33Z

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 · 2022-07-12T16:37:16Z

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 microsoft/TypeScript#30602.

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

CMCDragonkai · 2022-07-12T16:41:53Z

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

CMCDragonkai · 2022-07-13T08:11:46Z

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

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

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 · 2022-07-13T08:23:17Z

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:

For platform, always use os.platform(), there's no cross compilation for node-gyp anyway.
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.
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 · 2022-07-13T09:59:56Z

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 · 2022-07-13T10:07:56Z

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

CMCDragonkai · 2022-07-13T13:32:22Z

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.

CMCDragonkai · 2022-07-13T13:35:19Z

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.

* Added `scripts/prebuild.js` which is set as the `prebuild` script in `package.json` * The `scripts/prebuild.js` supports `npm_config_devdir`, `npm_config_nodedir` and `npm_config_arch` as environment variables * Environment variables are necessary to pass configuration when using `npm run build` because the prebuild script doesn't receive commmand line parameters * Added `engines` which specifies the node runtime and msvs version, used by `scripts/prebuild.js` * Renamed `src/rocksdb` to `src/native` to avoid name conflict with `deps/rocksdb`, this affects windows builds * All `package.json` scripts must be prefixed with the actual program to be executable on Windows * The `execFile` functions do not read `PATHEXT` requiring the usage of `shell: true` option on windows * Node headers and static libraries are installed into `npm_config_devdir` but only for window and macos, nix still uses `npm_config_nodedir` * The `scripts/prebuild.js` supports incremental compilation, just need to cache the `./build` directory * No more `prebuildify`, completely replaced by `scripts/prebuild.js`, replicates expected `./prebuilds` file and directory structure

CMCDragonkai · 2022-07-13T14:07:49Z

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 · 2022-07-13T14:13:23Z

Pipeline Attempt on 587061594 for 1f6299d

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

CMCDragonkai · 2022-07-13T14:48:54Z

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

emmacasolin · 2022-07-14T01:30:55Z

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

https://nodejs.org/api/child_process.html#spawning-bat-and-cmd-files-on-windows

https://github.com/moxystudio/node-cross-spawn

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 · 2022-07-14T02:02:29Z

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 · 2022-07-14T03:08:12Z

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

CMCDragonkai · 2022-07-14T03:52:24Z

Fixed the mac builds, it was a missing comma in the settings. Surprising that the gyp file allows missing commas.

So the mac builds and tests work now on matrix-mac-1.

But I did notice that the compilation settings are a bit messy, worth looking into cleaning it up so it's clearer. Messy settings include multiple overrides of -std and -stdlib, and -mmacosx-version-min1 and -arch. It's also not entirely consistent between the deps and this js-db's native binding.

MatrixAI-Bot · 2022-07-14T03:55:23Z

Pipeline Attempt on 587539054 for 886f7cb

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

CMCDragonkai · 2022-07-14T04:18:31Z

While the mac builds is running on the CI/CD I experimented with how nix could work.

Firstly the latest update to matrix-mac-1 replaced /etc/zshrc and I had to add it back in with sudo vim /etc/zshrc with:

# Nix
if [ -e '/nix/var/nix/profiles/default/etc/profile.d/nix-daemon.sh' ]; then
  . '/nix/var/nix/profiles/default/etc/profile.d/nix-daemon.sh'
fi
# End Nix

Afterwards, I enabled some settings in /etc/nix/nix.conf... and we can enter nix-shell fine.

Trying to compile results in an error:

error: unknown target CPU 'armv8.3-a+crypto+sha2+aes+crc+fp16+lse+simd+ras+rdm+rcpc'

This is apparently due to the fact that I have x86_64 and arm64 to build a universal binary. However this is not supported yet: https://discourse.nixos.org/t/error-unknown-target-cpu-armv8-3-a/16833.

CMCDragonkai · 2022-07-14T04:25:13Z

@emmacasolin can you incorporate chocolatey caching and homebrew caching into the gitlab CI/CD jobs here as well.

MatrixAI-Bot · 2022-07-14T04:29:28Z

Pipeline Succeeded on 587539054 for 886f7cb

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

emmacasolin · 2022-07-14T04:51:32Z

@emmacasolin can you incorporate chocolatey caching and homebrew caching into the gitlab CI/CD jobs here as well.

Branch created here https://github.com/MatrixAI/js-db/tree/feature-cicd-platforms-caching

CMCDragonkai added 26 commits June 1, 2022 11:01

Preparing for native leveldb build

946a01e

Added leveldb native dependencies

a972390

Direct to C++ leveldb works

e7229bf

Switching over to rocksdb

da767cc

Removed StringOrBufferLength check from rocksdb.cpp

e42f01e

Modularized rocksdb.cpp into a rocksdb/napi directory

dc4db39

Added infoLogLevel to RocksDBDatabaseOptions

ba968b0

Added in basic transaction_* methods, and modularised `src/rocksdb/…

f521ab0

…napi/workers.cpp`

Native addon exported functions now match naming convention standard …

92edd7f

…for JS

Fixed Transaction* workers in C++ to instantiate the Transaction

7b55563

Lint fixing native C/C++ code

6862983

Tested write-skew anomaly and solved with transactionGetForUpdate

dd1cec1

Transactions support iterators and refactoring C++ object lifecycle

0dc0b96

* Enabled `NODE_DEBUG_NATIVE` runtime control of rocksdb logging * Testing transaction iterator

GC finalizers may run after env_cleanup_hook, so Detach methods can

c909c02

be called when the detached target is already closed. So I've removed the precondition of `Detach+` methods and `DecrementPendingWork` from both `Database` and `Transaction` classes.

Fix database->isClosing_ and Transaction::DecrementPendingWork should…

04c5448

… be checking for `pendingWork_ == 1`

Testing iterator consistency

434ef4a

Adding transactionClear

23d3405

Adding transactionMultiGet and transactionMultiGetForUpdate

f9cecbc

* Fixed missing promisify for `transactionClear` * Using `__func__` for log messages * Namespacing C++ iterator workers * Testing `transactionClear` with non-repeatable read

Re-enabled DBTransaction and integrated it with rocksdb's optimistic …

b8a6083

…transactions

Added DBTransaction.getForUpdate to address write skew, and added tes…

58a62f2

…t demonstrating race condition thrashing

Introducing PCC locking for DBTransaction

a06ccdf

Export RocksDB and RocksDBP interfaces

cb25e1f

Updated docs

567526b

Updated benchmarks

879c5e2

Merge pull request #19 from MatrixAI/feature-control

8fd0c9a

Introduce Snapshot Isolation OCC to DBTransaction

MatrixAI-Bot self-assigned this Jun 30, 2022

CMCDragonkai mentioned this pull request Jun 30, 2022

Introduce Snapshot Isolation OCC to DBTransaction #19

Merged

38 tasks

chore: lintfixed scripts/prebuild.js

1f6299d

emmacasolin mentioned this pull request Jul 14, 2022

Fix Failing Build Tests on Windows & MacOS MatrixAI/Polykey-CLI#14

Open

binding.gyp: missing comma typo for macos builds

886f7cb

MatrixAI-Bot merged commit 886f7cb into master Jul 14, 2022

github-pages bot temporarily deployed to github-pages July 14, 2022 04:29 Inactive

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ci: merge staging to master #38

ci: merge staging to master #38

MatrixAI-Bot commented Jun 30, 2022

MatrixAI-Bot commented Jun 30, 2022

ghost commented Jun 30, 2022 •

edited by ghost

Loading

CMCDragonkai commented Jul 12, 2022 •

edited

Loading

CMCDragonkai commented Jul 12, 2022

CMCDragonkai commented Jul 12, 2022

CMCDragonkai commented Jul 12, 2022

CMCDragonkai commented Jul 12, 2022

CMCDragonkai commented Jul 12, 2022

CMCDragonkai commented Jul 13, 2022 •

edited

Loading

CMCDragonkai commented Jul 13, 2022

CMCDragonkai commented Jul 13, 2022 •

edited

Loading

CMCDragonkai commented Jul 13, 2022

CMCDragonkai commented Jul 13, 2022

CMCDragonkai commented Jul 13, 2022

CMCDragonkai commented Jul 13, 2022

MatrixAI-Bot commented Jul 13, 2022

CMCDragonkai commented Jul 13, 2022

emmacasolin commented Jul 14, 2022

CMCDragonkai commented Jul 14, 2022

CMCDragonkai commented Jul 14, 2022

CMCDragonkai commented Jul 14, 2022

MatrixAI-Bot commented Jul 14, 2022

CMCDragonkai commented Jul 14, 2022

CMCDragonkai commented Jul 14, 2022

MatrixAI-Bot commented Jul 14, 2022

emmacasolin commented Jul 14, 2022

ci: merge staging to master #38

ci: merge staging to master #38

Conversation

MatrixAI-Bot commented Jun 30, 2022

MatrixAI-Bot commented Jun 30, 2022

ghost commented Jun 30, 2022 • edited by ghost Loading

Legend

CMCDragonkai commented Jul 12, 2022 • edited Loading

CMCDragonkai commented Jul 12, 2022

CMCDragonkai commented Jul 12, 2022

CMCDragonkai commented Jul 12, 2022

CMCDragonkai commented Jul 12, 2022

CMCDragonkai commented Jul 12, 2022

CMCDragonkai commented Jul 13, 2022 • edited Loading

CMCDragonkai commented Jul 13, 2022

CMCDragonkai commented Jul 13, 2022 • edited Loading

CMCDragonkai commented Jul 13, 2022

CMCDragonkai commented Jul 13, 2022

CMCDragonkai commented Jul 13, 2022

CMCDragonkai commented Jul 13, 2022

MatrixAI-Bot commented Jul 13, 2022

CMCDragonkai commented Jul 13, 2022

emmacasolin commented Jul 14, 2022

CMCDragonkai commented Jul 14, 2022

CMCDragonkai commented Jul 14, 2022

CMCDragonkai commented Jul 14, 2022

MatrixAI-Bot commented Jul 14, 2022

CMCDragonkai commented Jul 14, 2022

CMCDragonkai commented Jul 14, 2022

MatrixAI-Bot commented Jul 14, 2022

emmacasolin commented Jul 14, 2022

ghost commented Jun 30, 2022 •

edited by ghost

Loading

CMCDragonkai commented Jul 12, 2022 •

edited

Loading

CMCDragonkai commented Jul 13, 2022 •

edited

Loading

CMCDragonkai commented Jul 13, 2022 •

edited

Loading