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.

Sign up for GitHub

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

Jump to bottom

Latest release (0.5.0) does not work on WASM & test.shis broken #419

Closed
thomaseizinger opened this issue Mar 15, 2022 · 10 comments · Fixed by #422
Closed

Latest release (0.5.0) does not work on WASM & test.shis broken #419

thomaseizinger opened this issue Mar 15, 2022 · 10 comments · Fixed by #422

Comments

@thomaseizinger
Copy link
Contributor

thomaseizinger commented Mar 15, 2022 •
edited
Loading

Investigations on why CI is failing downstream showed that the web-assembly build for that PR actually never worked: https://github.com/rust-bitcoin/rust-secp256k1/runs/5470259221?check_suite_focus=true#step:5:518

For reference, even after the latest changes to the CI config, it is still not working: https://github.com/rust-bitcoin/rust-secp256k1/runs/5517459899?check_suite_focus=true#step:5:2447
@thomaseizinger thomaseizinger changed the title Latest release does not work on WASM Latest release (0.5.0) does not work on WASM Mar 15, 2022
@sanket1729
Copy link
Member

I do not understand anything about wasm, but I can confirm that DO_WASM=true ./contrib/test also does not work locally.

@thomaseizinger thomaseizinger changed the title Latest release (0.5.0) does not work on WASM Latest release (0.5.0) does not work on WASM & test.shis broken Mar 15, 2022
@thomaseizinger
Copy link
Contributor Author

I do not understand anything about wasm, but I can confirm that DO_WASM=true ./contrib/test also does not work locally.

Yes, annoyingly though, it does not exit with a non-zero exit code which is why CI does not pick up the failure.

cc @tcharding

@tcharding
Copy link
Member

tcharding commented Mar 15, 2022 •
edited
Loading

Leave it with me, I'll (EDIT: try to) sort it out.

@sanket1729 sanket1729 mentioned this issue Mar 16, 2022
Closed
@tcharding tcharding mentioned this issue Mar 17, 2022
Merged
@tcharding
Copy link
Member

Its got me beat I'm afraid. Here is a draft (non-mergable) PR showing the little I found: #421

This was referenced Mar 19, 2022
Merged
Merged
@tcharding
Copy link
Member

tcharding commented Mar 19, 2022 •
edited
Loading

This one is proving to be a bit curly.

Any tips, suggestions, or ideas please?

@junderw
Copy link
Contributor

junderw commented Mar 19, 2022 •
edited
Loading

I get this error starting with 0.22.0 (maybe it gives us a hint?):

Expand 

error: linking with `rust-lld` failed: exit status: 1
  |
  = note: rust-lld: error: duplicate symbol: WASM32_INT_SIZE
          >>> defined in /tmp/rustciBlixx/libsecp256k1_sys-b664e02e63969bc9.rlib(precomputed_ecmult_gen.o)
          >>> defined in /tmp/rustciBlixx/libsecp256k1_sys-b664e02e63969bc9.rlib(precomputed_ecmult.o)
          
          rust-lld: error: duplicate symbol: WASM32_INT_ALIGN
          >>> defined in /tmp/rustciBlixx/libsecp256k1_sys-b664e02e63969bc9.rlib(precomputed_ecmult_gen.o)
          >>> defined in /tmp/rustciBlixx/libsecp256k1_sys-b664e02e63969bc9.rlib(precomputed_ecmult.o)
          
          rust-lld: error: duplicate symbol: WASM32_UNSIGNED_INT_SIZE
          >>> defined in /tmp/rustciBlixx/libsecp256k1_sys-b664e02e63969bc9.rlib(precomputed_ecmult_gen.o)
          >>> defined in /tmp/rustciBlixx/libsecp256k1_sys-b664e02e63969bc9.rlib(precomputed_ecmult.o)
          
          rust-lld: error: duplicate symbol: WASM32_UNSIGNED_INT_ALIGN
          >>> defined in /tmp/rustciBlixx/libsecp256k1_sys-b664e02e63969bc9.rlib(precomputed_ecmult_gen.o)
          >>> defined in /tmp/rustciBlixx/libsecp256k1_sys-b664e02e63969bc9.rlib(precomputed_ecmult.o)
          
          rust-lld: error: duplicate symbol: WASM32_SIZE_T_SIZE
          >>> defined in /tmp/rustciBlixx/libsecp256k1_sys-b664e02e63969bc9.rlib(precomputed_ecmult_gen.o)
          >>> defined in /tmp/rustciBlixx/libsecp256k1_sys-b664e02e63969bc9.rlib(precomputed_ecmult.o)
          
          rust-lld: error: duplicate symbol: WASM32_SIZE_T_ALIGN
          >>> defined in /tmp/rustciBlixx/libsecp256k1_sys-b664e02e63969bc9.rlib(precomputed_ecmult_gen.o)
          >>> defined in /tmp/rustciBlixx/libsecp256k1_sys-b664e02e63969bc9.rlib(precomputed_ecmult.o)
          
          rust-lld: error: duplicate symbol: WASM32_UNSIGNED_CHAR_SIZE
          >>> defined in /tmp/rustciBlixx/libsecp256k1_sys-b664e02e63969bc9.rlib(precomputed_ecmult_gen.o)
          >>> defined in /tmp/rustciBlixx/libsecp256k1_sys-b664e02e63969bc9.rlib(precomputed_ecmult.o)
          
          rust-lld: error: duplicate symbol: WASM32_UNSIGNED_CHAR_ALIGN
          >>> defined in /tmp/rustciBlixx/libsecp256k1_sys-b664e02e63969bc9.rlib(precomputed_ecmult_gen.o)
          >>> defined in /tmp/rustciBlixx/libsecp256k1_sys-b664e02e63969bc9.rlib(precomputed_ecmult.o)
          
          rust-lld: error: duplicate symbol: WASM32_PTR_SIZE
          >>> defined in /tmp/rustciBlixx/libsecp256k1_sys-b664e02e63969bc9.rlib(precomputed_ecmult_gen.o)
          >>> defined in /tmp/rustciBlixx/libsecp256k1_sys-b664e02e63969bc9.rlib(precomputed_ecmult.o)
          
          rust-lld: error: duplicate symbol: WASM32_PTR_ALIGN
          >>> defined in /tmp/rustciBlixx/libsecp256k1_sys-b664e02e63969bc9.rlib(precomputed_ecmult_gen.o)
          >>> defined in /tmp/rustciBlixx/libsecp256k1_sys-b664e02e63969bc9.rlib(precomputed_ecmult.o)
          
          rust-lld: error: duplicate symbol: WASM32_INT_SIZE
          >>> defined in /tmp/rustciBlixx/libsecp256k1_sys-b664e02e63969bc9.rlib(precomputed_ecmult.o)
          >>> defined in /tmp/rustciBlixx/libsecp256k1_sys-b664e02e63969bc9.rlib(secp256k1.o)
          
          rust-lld: error: duplicate symbol: WASM32_INT_ALIGN
          >>> defined in /tmp/rustciBlixx/libsecp256k1_sys-b664e02e63969bc9.rlib(precomputed_ecmult.o)
          >>> defined in /tmp/rustciBlixx/libsecp256k1_sys-b664e02e63969bc9.rlib(secp256k1.o)
          
          rust-lld: error: duplicate symbol: WASM32_UNSIGNED_INT_SIZE
          >>> defined in /tmp/rustciBlixx/libsecp256k1_sys-b664e02e63969bc9.rlib(precomputed_ecmult.o)
          >>> defined in /tmp/rustciBlixx/libsecp256k1_sys-b664e02e63969bc9.rlib(secp256k1.o)
          
          rust-lld: error: duplicate symbol: WASM32_UNSIGNED_INT_ALIGN
          >>> defined in /tmp/rustciBlixx/libsecp256k1_sys-b664e02e63969bc9.rlib(precomputed_ecmult.o)
          >>> defined in /tmp/rustciBlixx/libsecp256k1_sys-b664e02e63969bc9.rlib(secp256k1.o)
          
          rust-lld: error: duplicate symbol: WASM32_SIZE_T_SIZE
          >>> defined in /tmp/rustciBlixx/libsecp256k1_sys-b664e02e63969bc9.rlib(precomputed_ecmult.o)
          >>> defined in /tmp/rustciBlixx/libsecp256k1_sys-b664e02e63969bc9.rlib(secp256k1.o)
          
          rust-lld: error: duplicate symbol: WASM32_SIZE_T_ALIGN
          >>> defined in /tmp/rustciBlixx/libsecp256k1_sys-b664e02e63969bc9.rlib(precomputed_ecmult.o)
          >>> defined in /tmp/rustciBlixx/libsecp256k1_sys-b664e02e63969bc9.rlib(secp256k1.o)
          
          rust-lld: error: duplicate symbol: WASM32_UNSIGNED_CHAR_SIZE
          >>> defined in /tmp/rustciBlixx/libsecp256k1_sys-b664e02e63969bc9.rlib(precomputed_ecmult.o)
          >>> defined in /tmp/rustciBlixx/libsecp256k1_sys-b664e02e63969bc9.rlib(secp256k1.o)
          
          rust-lld: error: duplicate symbol: WASM32_UNSIGNED_CHAR_ALIGN
          >>> defined in /tmp/rustciBlixx/libsecp256k1_sys-b664e02e63969bc9.rlib(precomputed_ecmult.o)
          >>> defined in /tmp/rustciBlixx/libsecp256k1_sys-b664e02e63969bc9.rlib(secp256k1.o)
          
          rust-lld: error: duplicate symbol: WASM32_PTR_SIZE
          >>> defined in /tmp/rustciBlixx/libsecp256k1_sys-b664e02e63969bc9.rlib(precomputed_ecmult.o)
          >>> defined in /tmp/rustciBlixx/libsecp256k1_sys-b664e02e63969bc9.rlib(secp256k1.o)
          
          rust-lld: error: duplicate symbol: WASM32_PTR_ALIGN
          >>> defined in /tmp/rustciBlixx/libsecp256k1_sys-b664e02e63969bc9.rlib(precomputed_ecmult.o)
          >>> defined in /tmp/rustciBlixx/libsecp256k1_sys-b664e02e63969bc9.rlib(secp256k1.o)

@junderw
Copy link
Contributor

junderw commented Mar 20, 2022

Perhaps including precomputed_ecmult.c AND precomputed_ecmult_gen.c in the build is causing the problem.

@thomaseizinger
Copy link
Contributor Author

thomaseizinger commented Mar 20, 2022 •
edited
Loading

Perhaps including precomputed_ecmult.c AND precomputed_ecmult_gen.c in the build is causing the problem.

That is what upstream does too though and it is fine, at least from what I can see from the Makefile.

The error originates from symbols that we add in our buildscript to make it wasm compatible.

Perhaps we no longer need the symbols altogether?

@junderw
Copy link
Contributor

junderw commented Mar 20, 2022 •
edited
Loading

I made #426 which seems to help.

@TheBlueMatt
Copy link
Member

The error originates from symbols that we add in our buildscript to make it wasm compatible.

Yep, see #421 (comment)

This was referenced Mar 20, 2022
Closed
Closed
@TheBlueMatt TheBlueMatt mentioned this issue Mar 27, 2022
Merged
3 tasks
apoelstra added a commit that referenced this issue Mar 30, 2022
@apoelstra
Merge #421: Fix wasm build
f7cae46 
bfd88db Move WASM const definitions to a source file (Tobin Harding)

Pull request description:

  Total re-write ... again :)

  Currently we are defining the WASM integer size and alignments in the `stdio.h` header file, this is wrong because this file is included in the build by way of `build.rs` as well as by upstream `libsecp256k1`.

  Move WASM integer definitions to a `C` source file and build the file  into the binary if target is WASM.

  Fixes the first part of #419 (#422 does the second part).

  ### Note to reviewers

  I'm not exactly sure why the directory `was-sysroot` is named as it is or if the name is significant to `cargo` , please review carefully the directory tree changes.

  ```
  cd secp256k1-sys
  tree wasm
  wasm
  ├── wasm.c
  └── wasm-sysroot
             ├── stdio.h
             ├── stdlib.h
             └── string.h
  ```

ACKs for top commit:
  thomaseizinger:
    ACK bfd88db
  apoelstra:
    ACK bfd88db

Tree-SHA512: ba822b764fb5f74dfd22cc797f7e3f70440dbaabfe34e0475c796e0e5d88f2086bedb00a1ec765cce91bde6bb45130b9abe5d9289317d6c20f692c6ed711969e
apoelstra added a commit that referenced this issue Apr 1, 2022
@apoelstra
Merge #420: Improve CI script
8caf41d 
58db1b6 Run WASM for multiple toolchains (Tobin Harding)
946ac3b Do docs build in Nightly job (Tobin Harding)
f7bc7d3 Install clang to run adress sanitizer (Tobin Harding)
96685c5 Remove unnecessary matrix (Tobin Harding)
a8a679e Re-name nightly CI job to Nightly (Tobin Harding)
9c9d622 Remove trailing whitespace (Tobin Harding)

Pull request description:

  Improve the CI pipeline. Done while investigating #419. This PR now only includes the GitHub actions changes, I'm moving the `test.sh` changes to a #422 because they cannot be merged yet.

  (Please note, this PR has been heavily re-worked so discussion below may be confusing to reviewers new to the PR.)

ACKs for top commit:
  apoelstra:
    ACK 58db1b6
  thomaseizinger:
    ACK 58db1b6

Tree-SHA512: 5520cf7a7ea0ba701aeaf6b97150416192c0629df8b65545a20d8937a4d76bd323a0d7a875deccb7ce9adc4f3a423e6cd27b300682f206f79407f5ab4eaa5ddb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
5 participants
@TheBlueMatt @thomaseizinger @sanket1729 @junderw @tcharding