Skip to content

Add basic rust integration test. NFC #22964

New issue

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

#

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

Already on GitHub? # to your account

Jump to bottom
Merged
merged 1 commit into from
Nov 20, 2024
Merged

Add basic rust integration test. NFC #22964

merged 1 commit into from
Nov 20, 2024

Conversation

sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Nov 19, 2024

No description provided.

@sbc100 sbc100 requested review from dschuff and tlively November 19, 2024 21:48
@sbc100 sbc100 force-pushed the rust_basics branch 6 times, most recently from f6c6ad3 to 17e69be Compare November 19, 2024 22:09
tlively
tlively reviewed Nov 19, 2024
View reviewed changes
@sbc100
Copy link
Collaborator Author

sbc100 commented Nov 19, 2024

Sorry I'm still trying to get the CI working for this.

@sbc100
Copy link
Collaborator Author

sbc100 commented Nov 19, 2024

OK, all looks good now, PTAL

dschuff
dschuff reviewed Nov 19, 2024
View reviewed changes
.circleci/config.yml
- run:
name: install rust
command: |
curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y
Copy link
Member

Choose a reason for hiding this comment

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

#YOLO

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is the official way to install rust apparently.. so its actually good that we use this method here I think.

.circleci/config.yml Outdated
@@ -721,6 +730,18 @@ jobs:
core0.test_hello_argc
core2.test_demangle_stacks_symbol_map"
- upload-test-results
test-rust:
executor: linux-python
Copy link
Member

Choose a reason for hiding this comment

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

Another option here might be the CircleCI rust image? https://circleci.com/developer/images/image/cimg/rust

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tried that first but decided against it for 2 reasons:

  1. We need python to run emscripten and the rust image does not have python installed.
  2. We want to run rust nightly I think, which the rust image doesn't seem to have

Copy link
Member

Choose a reason for hiding this comment

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

Ah, interesting. Are there changes going into core rustc that we will need?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

One of my ideas behind this integration test is to make sure emscripten output is compatible with rust as it evolves, so like we do for firefox and chromium, we always test against latest/nightly, even though it means our CI can sometimes break due to external changes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As an aside I find it odd that circle CI doesn't have base images with more than one tool. Surely there are a lot of polyglot projects out there that needs python + node or python + rust. It seems that such projects currently have to build their own base image or manually install one or more runtimes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't look like you're installing nightly yet?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What's the recommended way to do that?

Copy link
Contributor

Choose a reason for hiding this comment

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

rustup toolchain install nightly

Looks like you might also be able to specify it when you install rustup https://rust-lang.github.io/rustup/installation/index.html#installing-nightly

@sbc100 sbc100 requested review from tlively and dschuff November 19, 2024 22:56
@sbc100
Copy link
Collaborator Author

sbc100 commented Nov 19, 2024

CC @hoodmane in case you have ideas for some more rust smoke tests we can/should add here?

@sbc100 sbc100 enabled auto-merge (squash) November 20, 2024 00:00
dschuff
dschuff approved these changes Nov 20, 2024
View reviewed changes
Copy link
Member

@dschuff dschuff left a comment

Choose a reason for hiding this comment

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

LGTM. Don't forget to put EMTEST_SKIP_RUST into the environment on the Chromium CI bots so this can roll.

@sbc100 sbc100 disabled auto-merge November 20, 2024 19:28
@sbc100 sbc100 merged commit f56ee36 into emscripten-core:main Nov 20, 2024
26 of 28 checks passed
@sbc100 sbc100 deleted the rust_basics branch November 20, 2024 19:28
@kripken
Copy link
Member

kripken commented Nov 20, 2024

@sbc100 Rust tests are failing on the roller:

https://logs.chromium.org/logs/emscripten-releases/buildbucket/cr-buildbucket/8730720020817447969/+/u/Emscripten_testsuite__other_/stdout

https://logs.chromium.org/logs/emscripten-releases/buildbucket/cr-buildbucket/8730716182753081457/+/u/Emscripten_testsuite__other_/stdout

@dschuff
Copy link
Member

dschuff commented Nov 21, 2024

That should be fixed by https://chromium-review.googlesource.com/c/emscripten-releases/+/6038095

@kripken
Copy link
Member

kripken commented Nov 21, 2024

Hmm, that was merged 2PM yesterday, but the roller is still failing as of 4 hours ago. I see that the error message

https://logs.chromium.org/logs/emscripten-releases/buildbucket/cr-buildbucket/8730651596731910801/+/u/Emscripten_testsuite__other_/stdout

mentions EMTEST_SKIP_CARGO while the code here mentions _RUST instead. Are those perhaps not in sync?

@dschuff dschuff mentioned this pull request Nov 21, 2024
Closed
sbc100 added a commit to sbc100/emscripten that referenced this pull request Nov 21, 2024
@sbc100
[test] Rename EMTEST_SKIP_CARGO -> EMTEST_SKIP_RUST. NFC
16d8337 
See emscripten-core#22964
@sbc100 sbc100 mentioned this pull request Nov 21, 2024
Merged
@sbc100
Copy link
Collaborator Author

sbc100 commented Nov 21, 2024

Ah yes! I decided to fix this on the emscripten side: #22982

sbc100 added a commit to sbc100/emscripten that referenced this pull request Nov 21, 2024
@sbc100
[test] Rename EMTEST_SKIP_CARGO -> EMTEST_SKIP_RUST. NFC
ff9cddb 
See emscripten-core#22964
sbc100 added a commit to sbc100/emscripten that referenced this pull request Nov 21, 2024
@sbc100
[test] Rename EMTEST_SKIP_CARGO -> EMTEST_SKIP_RUST. NFC
83e85e9 
See emscripten-core#22964
sbc100 added a commit that referenced this pull request Nov 21, 2024
@sbc100
[test] Rename EMTEST_SKIP_CARGO -> EMTEST_SKIP_RUST. NFC (#22982)
bd2de9c 
See #22964
@sbc100 sbc100 mentioned this pull request Nov 21, 2024
Open
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@curiousdannii curiousdannii curiousdannii left review comments

@dschuff dschuff dschuff approved these changes

@tlively tlively tlively approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@sbc100 @kripken @dschuff @curiousdannii @tlively