Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ci: merge staging to master #4

Merged
merged 57 commits into from
Oct 18, 2023
Merged

ci: merge staging to master #4

merged 57 commits into from
Oct 18, 2023

Conversation

MatrixAI-Bot
Copy link
Member

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

@MatrixAI-Bot MatrixAI-Bot self-assigned this Aug 7, 2023
@MatrixAI-Bot
Copy link
Member Author

@tegefaulkes
Copy link
Contributor

Looks like the mac and windows builds are failing in CI. It's been a long time since these jobs were ran in Polykey so I don't know if it's a new problem with the CLI migration, or a general problem from Polykey's network migration work.

I'll disable them for now since they're not on the critical path. The integration tests for these builds are already disabled.

Next step is to get the nix-build step working, right now it's failing to clone the uWebsockets repo. I think this sort of thing should be pre-fetched by nix? I'm not sure how to add that sort of thing in.

@MatrixAI-Bot
Copy link
Member Author

@tegefaulkes
Copy link
Contributor

Ok, i'm putting this on hold for now. The current problem with nix-dry and nix-build -attr application is blocked by the uWebsockets removal.

Priority now is to work on the agent migration and complete that, including removing uWebsockets in favour of ws.js.

@MatrixAI-Bot
Copy link
Member Author

@CMCDragonkai
Copy link
Member

@tegefaulkes is it possible to pull in the latest PK release right now?

@tegefaulkes
Copy link
Contributor

You should be able to use 1.1.5-alpha.0.

@CMCDragonkai
Copy link
Member

Looks like @matrixai/quic is currently a dependency of this. Seems wrong. It seems that it's relying on the QUICConfig type. That shouldn't be done. PK should abstract around that type, so we are only using configuration types from polykey.

»» ~/Projects/Polykey-CLI/src
 ♖ ag 'quic'                                   ⚡(staging) pts/5 16:39:08
types.ts
12:import type { QUICConfig } from '@matrixai/quic';
18:// TODO: fix this... We don't need a dependecy on `@matrixai/quic
19:type PolykeyQUICConfig = {
29:} & Partial<QUICConfig>;
64:      // Agent QUICSocket config
81:    quicServerConfig?: PolykeyQUICConfig;
82:    quicClientConfig?: PolykeyQUICConfig;

@MatrixAI-Bot
Copy link
Member Author

@MatrixAI-Bot
Copy link
Member Author

@MatrixAI-Bot
Copy link
Member Author

@MatrixAI-Bot
Copy link
Member Author

@CMCDragonkai
Copy link
Member

To address 3., it's important to reactivate some integration tests. But they need to be written minimally, so that they don't rely too much on implementation details. We're all the way up to the UX now. Not in the weeds. @tegefaulkes @amydevs

@CMCDragonkai
Copy link
Member

Docker image has been built and deployed, so that is all still working.

However there are integration tests failing https://gitlab.com/MatrixAI/open-source/Polykey-CLI/-/pipelines/1040057667.

This was expected, it's time to reactivate and refactor our integration tests, and maybe get PK re-enabled on windows and mac too.

@CMCDragonkai
Copy link
Member

The integration:linux which uses ubuntu still has a problem:

$ for f in ./builds/*-linux-*; do "$f"; done
/snapshot/Polykey-CLI/dist/polykey.js:62491
      throw new Error(`Failed requiring possible native bindings: ${prebuildTargets.concat(npmTargets)}`);
      ^
Error: Failed requiring possible native bindings: /snapshot/prebuild/quic-linux-x64.node,@matrixai/quic-linux-x64
    at requireBinding (/snapshot/Polykey-CLI/dist/polykey.js:62491:13)
    at node_modules/@matrixai/quic/dist/native/quiche.js (/snapshot/Polykey-CLI/dist/polykey.js:625[33](https://gitlab.com/MatrixAI/open-source/Polykey-CLI/-/jobs/5313359556#L33):29)
    at __require (/snapshot/Polykey-CLI/dist/polykey.js:13:50)
    at node_modules/@matrixai/quic/dist/native/index.js (/snapshot/Polykey-CLI/dist/polykey.js:62660:20)
    at __require (/snapshot/Polykey-CLI/dist/polykey.js:13:50)
    at node_modules/@matrixai/quic/dist/QUICSocket.js (/snapshot/Polykey-CLI/dist/polykey.js:6[34](https://gitlab.com/MatrixAI/open-source/Polykey-CLI/-/jobs/5313359556#L34)63:20)
    at __require (/snapshot/Polykey-CLI/dist/polykey.js:13:50)
    at node_modules/@matrixai/quic/dist/index.js (/snapshot/Polykey-CLI/dist/polykey.js:66268:24)
    at __require (/snapshot/Polykey-CLI/dist/polykey.js:13:50)
    at node_modules/polykey/dist/nodes/utils.js (/snapshot/Polykey-CLI/dist/polykey.js:70223:18)
Node.js v18.15.0

We saw this before. I think the script used to load the quic native binding doesn't seem to work after being esbuilt or something. Not sure need to check.

@MatrixAI-Bot
Copy link
Member Author

@CMCDragonkai
Copy link
Member

CMCDragonkai commented Oct 17, 2023

The error comes from:

  throw new Error(
    `Failed requiring possible native bindings: ${prebuildTargets.concat(
      npmTargets,
    )}`,
  );

Which is in src/native/quiche.ts in the requireBinding function.

/**
 * Try require on all prebuild targets first, then
 * try require on all npm targets second.
 */
function requireBinding(targets: Array<string>): Quiche {
  const prebuildTargets = targets.map((target) =>
    path.join(prebuildPath, `quic-${target}.node`),
  );
  for (const prebuildTarget of prebuildTargets) {
    try {
      return require(prebuildTarget);
    } catch (e) {
      if (e.code !== 'MODULE_NOT_FOUND') throw e;
    }
  }
  const npmTargets = targets.map((target) => `@matrixai/quic-${target}`);
  for (const npmTarget of npmTargets) {
    try {
      return require(npmTarget);
    } catch (e) {
      if (e.code !== 'MODULE_NOT_FOUND') throw e;
    }
  }
  throw new Error(
    `Failed requiring possible native bindings: ${prebuildTargets.concat(
      npmTargets,
    )}`,
  );
}

The error message tells us that it is trying load native binding in:

/snapshot/prebuild/quic-linux-x64.node,@matrixai/quic-linux-x64

The first path is /snapshot/prebuild/quic-linux-x64.node which is a development-only path.

The second path is @matrixai/quic-linux-x64 which should be set up properly.

The reason is that:

  1. The @matrixai/quic-linux-x64 is set as an external package for esbuild.
  2. And also it's set as an asset for pkg in package.json, since the pkg.js script cannot auto-find it using node-gyp-build.

@CMCDragonkai
Copy link
Member

I wonder if pkg when finally putting together the entire executable, does it support the ability to require @matrixai/quic-linux-x64?

It could be due to pkg not being able to maintain a VFS that enables the ability to require it.

It's possible that originally node-gyp-build does it differently, and so that's why it doesn't have this problem.

@CMCDragonkai
Copy link
Member

I think I have an idea, it's possible the entire package.json is missing and that's why require('@matrixai/quic-linux/x64') does not work. Because I'm using the entire package name as the require path, and perhaps that requires the package.json too.

…ns the `package.json` is required to ensure that `main` can be resolved
@MatrixAI-Bot
Copy link
Member Author

@CMCDragonkai
Copy link
Member

Yep that was the problem and was solved by adding package.json into the assets list too.

As a general matter it is somewhat more correct to depend on the special packages since the package.json uses main to dictate where the .node is.

However it is little inefficient to bundle the package.json.

One way is to change our custom requireBinding so it just tries to load the @matrixai/quic-linux-x64/node.napi.node instead. That would mean we wouldn't need the package.json. BTW this works because the special native packages don't use dist, otherwise @matrixai/quic-linux-x64/node.napi.node would be needed.

However with the ESM migration MatrixAI/TypeScript-Demo-Lib#32, node's import does not support native addons: https://nodejs.org/api/esm.html#no-addon-loading

The docs suggest using https://nodejs.org/api/process.html#processdlopenmodule-filename-flags instead.

So I think that would be more accurate, that means dropping the require and swapping to use dlopen instead, and therefore needing a deterministic path to look that up.

@MatrixAI-Bot
Copy link
Member Author

@CMCDragonkai
Copy link
Member

Ok remaining thing is:

image

@CMCDragonkai
Copy link
Member

If that passes then:

  1. Docker is working
  2. Pkg - ubuntu is working
  3. And it's all deployed to ECR

Then this should merge to master.

And then we can focus on the remaining tasks in testnet 6.

1. Making sure the docker integration tests pass in the relevant env parameters
2. extended timeouts for problimatic tests that do a password hash. Even though i'm sure we're passing in these options, the hashing still seems to be very slow. On the order of 10+ seconds.
3. Some tests were being skipped with `.skip` if this test was optionally disable based on the integration test it became `.skip.skip`. I've fixed this.
@MatrixAI-Bot
Copy link
Member Author

@MatrixAI-Bot
Copy link
Member Author

@MatrixAI-Bot
Copy link
Member Author

Pipeline Succeeded on 1040506440 for e029c8d

https://gitlab.com/MatrixAI/open-source/Polykey-CLI/-/pipelines/1040506440

@MatrixAI-Bot MatrixAI-Bot merged commit e029c8d into master Oct 18, 2023
1 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants