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

Re-enable web unit test #4444

Merged
merged 1 commit into from
Apr 27, 2023
Merged

Re-enable web unit test #4444

merged 1 commit into from
Apr 27, 2023

Conversation

FirelightFlagboy
Copy link
Contributor

@FirelightFlagboy FirelightFlagboy commented Apr 21, 2023

Remove wasm-pack from devDependencies since it don't work.
Instead install it from the wasm-pack repo.

Other Changes

  • Correct unused warning when compiling platform_http_proxy on wasm32.
  • In bindings/web/scripts/build.js:
    • Provide process.env to sub command.
    • Now the script will directly invoque wasm-pack.
    • Ensure rust target wasm32-unknown-unknown is installed.
  • Remove ci build type for rust web since wasm-pack don't support custom profile.
  • Cache rust artifact when build the web bindings.

Closes #4447

Copy link
Contributor

@mmmarcos mmmarcos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (although the CI is complaining about wasm-pack not being found)

.github/workflows/ci-web.yml Outdated Show resolved Hide resolved
@FirelightFlagboy FirelightFlagboy self-assigned this Apr 21, 2023
Base automatically changed from reenable-web-ci to master April 21, 2023 13:02
@FirelightFlagboy FirelightFlagboy force-pushed the re-enable-web-unit-test branch 6 times, most recently from c75a97d to 0ee8013 Compare April 24, 2023 08:24
@@ -109,6 +111,9 @@ mod not_wasm32 {
}
}

#[cfg(not(target_arch = "wasm32"))]
pub use not_wasm32::{HTTPS_PROXY, HTTP_PROXY};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the names to use in platform are native and web

(don't use wasm32 given we specifically target web i.e. we consider we have access to the browser API)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Member

@touilleMan touilleMan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@@ -1,80 +1,160 @@
#! /usr/bin/env node

const { argv, exit, platform } = require('node:process');
const { spawnSync } = require('node:child_process');
const { spawnSync, spawn } = require('node:child_process');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

spawn doesn't seems used

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No it isn't, all good now :)

Remove `wasm-pack` from `devDependencies` since it don't work.
Instead install it from the wasm-pack repo.

Other Changes
-------------

- Correct unused warning when compiling `platform_http_proxy` on `wasm32`.
- In `bindings/web/scripts/build.js`:
  - Provide `process.env` to sub command.
  - Now the script will directly invoque `wasm-pack`.
  - Ensure rust target `wasm32-unknown-unknown` is installed.
  - Ensure a link exist between `ci-rust` & `debug`.
- Cache rust artifact when build the web bindings.

Closes #4447
@FirelightFlagboy FirelightFlagboy added this pull request to the merge queue Apr 27, 2023
Merged via the queue into master with commit 86cc3d2 Apr 27, 2023
@FirelightFlagboy FirelightFlagboy deleted the re-enable-web-unit-test branch April 27, 2023 09:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wasm-pack don't support custom profile
4 participants