bcgit / pc-dart

Pointy Castle - Dart Derived Bouncy Castle APIs
MIT License
237 stars 123 forks source link

add dart2wasm support #226

Closed konsultaner closed 6 months ago

konsultaner commented 7 months ago

fixes #221

konsultaner commented 7 months ago

This is a blocker at the moment:

https://github.com/dart-lang/sdk/issues/55031

Which has been fixed. This should work with the next Version of dart. For non wasm compilition it should already work.

So it could be merged in my eyes?

konsultaner commented 6 months ago

@Ephenodrom Do you see any issues with this?

Ephenodrom commented 6 months ago

@konsultaner I am not very familiar with Dart / Flutter on web, due to i only use it on CLI or for apps running on different operating systems, therefore my knowledge is limited but I see no problem with the PR. I will check the unit tests and see if they are good.

Ephenodrom commented 6 months ago

@konsultaner There are several tests that fail with the following error : "Failed to load "test/stream/eax_test.dart": The "buf" argument must be an instance of ArrayBuffer or ArrayBufferView. Received an instance of Array".

Please take a look. The tests are run with "dart run test -p node" and use the latest dart version 3.3.1

konsultaner commented 6 months ago

The "buf" argument must be an instance of ArrayBuffer or ArrayBufferView. Received an instance of Array

@Ephenodrom I'm on it. I forgot to test it with node. seems a bit tricky, but I know why the tests fail.

konsultaner commented 6 months ago

@Ephenodrom I fixed it. Could you try again?

I just also want to notice that you might want to change dart pub run to just dart run in your CI script since it is depricated. if the next version of dart it released you might also want to add a test with --platform chrome -c dart2wasm to also validate the wasm support. For some reason --platform node -c dart2wasm is not supported yet. I guess this is due to a missing wasm-gc support in older node versions. I think the current latest node version should supprt it.

Ephenodrom commented 6 months ago

@konsultaner This looks good so far. I already merged the PR in the origin repo.

When I run the test for the node environment via "dart run test -p node ", there are some tests that fail. Is this intended at the moment?

konsultaner commented 6 months ago

@Ephenodrom no it is not intended. It should not fail at all. Thats strange. All tests were successful when I tested it locally. Ill have another look at it.

Could you run the github Actions again?

konsultaner commented 6 months ago

@Ephenodrom I checked everything again and ran dart test -p node on a node v16.20.1 with dart on 3.3.1. That resulted in:

image

What errors do you get?

Ephenodrom commented 6 months ago

@konsultaner Ok strange, I tried it with node version v20.6.1 and the latest version v21.7.1.

I receive for example the following error :

00:28 +119 -1: loading test/key_generators/rsa_key_generator_test.dart [E]
  Failed to load "test/key_generators/rsa_key_generator_test.dart": Value of "this" must be of type nullish or must be the global object
  org-dartlang-sdk:///lib/_internal/js_runtime/lib/js_string.dart 202:5   global.cryptoThisCheck
  org-dartlang-sdk:///lib/_internal/js_runtime/lib/math_patch.dart 253:9  _JSSecureRandom._JSSecureRandom
  org-dartlang-sdk:///lib/_internal/js_runtime/lib/math_patch.dart 251:3  <fn>
  org-dartlang-sdk:///lib/_internal/js_runtime/lib/math_patch.dart 74:30  PlatformWeb.PlatformWeb
  package:pointycastle/src/platform_check/web.dart 15:3                   <fn>
  package:pointycastle/src/platform_check/web.dart 65:39                  _generationTests.<fn>.<fn>.<fn>
konsultaner commented 6 months ago

@Ephenodrom I tried it again on node 21.7.1, dart 3.1.1 on linux with the command:

 dart run test -p node

And the same result, all tests run perfectly.

image

Could you please start the workflow to have the github ci test it too?

image

Ephenodrom commented 6 months ago

@konsultaner The checks look good so far. I have contacted someone from bouncycastle to sync the repository, if this is done I will create a new release on pub.dev. I keep you updated.

Ephenodrom commented 6 months ago

@konsultaner The changes are now live on pub.dev with version 3.8.0. Thank you very much for the PR.