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

Fix JS Demo #267

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from
Draft

Conversation

Crypto2099
Copy link
Contributor

This is not ready for merging yet but seeks to address Issue #264.

Troubleshooting identified the problem as some duplicated functions in the cardano-addresses-jsapi.esm.js file due to legacy crypto-wrapper.js and crc32.js which are correctly being pulled in via emcc now.

There are still a few duplicated functions that need to be traced.

…ble-definition we're seeing after the build process.
…unctions and crc32 hashing functions which were included separately via dedicated JS files previously. Attempting to remove them from build.sh to see if the built files will successfully work afterwards.
@Crypto2099 Crypto2099 added the javascript Pull requests that update Javascript code label Jul 24, 2024
@Crypto2099 Crypto2099 self-assigned this Jul 24, 2024
@luite
Copy link
Collaborator

luite commented Jul 25, 2024

I'd like to build this PR locally to check the build result, but I'm getting the same npm ERESOLVE error as the CI: running ~/cardano-addresses/jsapi $ nix develop ..#cardano-addresses-js-shell --command npm install

https://github.com/IntersectMBO/cardano-addresses/actions/runs/10085769443/job/27887169614?pr=267

What should I do instead?

@Crypto2099
Copy link
Contributor Author

Crypto2099 commented Jul 25, 2024

I'd like to build this PR locally to check the build result, but I'm getting the same npm ERESOLVE error as the CI: running ~/cardano-addresses/jsapi $ nix develop ..#cardano-addresses-js-shell --command npm install

https://github.com/IntersectMBO/cardano-addresses/actions/runs/10085769443/job/27887169614?pr=267

What should I do instead?

This build error I believe was due to my local package and lock getting accidentally updated and included in the commit. I've rolled these changes back this morning. For local testing I am making changes and running the same steps as the typescript.yml workflow file in order to build a new copy of the demo.

The "problem" is that the generated file: cardano-addresses-jsapi.esm.js that is created by a combination of ./jsbits/emscripten/build.sh and ./nix/cardano-addresses-js.nix is creating duplicate function definitions. I went through and manually modified/commented out my local file and identified that all of the functions that were previously being included from ./jsbits/emscripten/crc32.js and ./jsbits/emscripten/crypto-wrapper.js were being duplicated (mostly crc32 and cryptonite hashing algorithms that seem to now be getting properly included in the emscripten output from the c headers).

However, there is still 4 lingering functions that I have not been able to isolate the source of the duplication:

  • function h$geteuid(): Lines 25147 & 28089 of output
  • function h$sysconf(): Lines 25152 & 28094 of output
  • function h$getpwuid_r: Lines 25157 & 28099 of output
  • function h$foundation_sysrandom_linux: Lines 24639 & 25170 of output

@Crypto2099
Copy link
Contributor Author

@luite I found at least the first three of the "duplicated" functions included as part of the GHCJS library from this shim file:
https://github.com/ghcjs/ghcjs/blob/b7711fbca7c3f43a61f1dba526e6f2a2656ef44c/lib/boot/shims/pkg/unix.js.pp#L13

Although, I am uncertain where the other (non-commented as stub) versions of these same functions are being included.

@Crypto2099
Copy link
Contributor Author

Ah, some of the offending code (previously imported from ./jsbits/emscripten/crypto-wrapper.js was added to the cryptonite-0.3.0 patch by @hamishmack here: https://github.com/input-output-hk/hackage-overlay-ghcjs/blob/master/patches/cryptonite-0.30.patch

@luite
Copy link
Collaborator

luite commented Jul 26, 2024

This build error I believe was due to my local package and lock getting accidentally updated and included in the commit. I've rolled these changes back this morning. For local testing I am making changes and running the same steps as the typescript.yml workflow file in order to build a new copy of the demo.

Thanks, I'll try again and have a look at the build artifacts!

Looks like you've found the issues, we've merged some of the JS files into the packages, so they can be removed from the nix build.

GHC 9.6 doesn't automatically build/include c-sources with emscripten yet, but a later GHC (I think 9.10) does do that, so a few more custom build steps can be removed in a future upgrade.

@luite
Copy link
Collaborator

luite commented Jul 26, 2024

Thanks it builds here now and can run the tests! Just some still failing because h$withCBufferOnHeap is still missing.

@luite
Copy link
Collaborator

luite commented Aug 5, 2024

I've pushed fixes for digest and foundation to fix the missing memory functions. It now fails with _crc32 missing, since that should still be included by hand (until upgrading to GHC 9.10)

@luite
Copy link
Collaborator

luite commented Aug 5, 2024

Use the fixes by updating cabal.project:

repository ghcjs-overlay
  url: https://raw.githubusercontent.com/input-output-hk/hackage-overlay-ghcjs/acbe0444181da7582127ca8b60eb122cdad1c458
  secure: True
  root-keys:
  key-threshold: 0
  --sha256: sha256-f8LXqyiSlCfi1gbHN3BEB489NCfcC1nut43lyQ0RG1s=

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
javascript Pull requests that update Javascript code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants