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

Deny warnings and add miri to CI #2448

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Deny warnings and add miri to CI #2448

wants to merge 10 commits into from

Conversation

blobcode
Copy link

This Pull Request fixes/closes #2366

It changes the following:

  • Denies Warnings in cargo build
  • Enables miri as a seperate step running on ubuntu (miri has limited support on windows)

Note that this will cause previously passing builds to fail.

@jedel1043
Copy link
Member

Opened #2449 to fix the deprecation warnings.

@jedel1043
Copy link
Member

Also, I'm not sure if we want to run all tests through MIRI. On my machine (ryzen 5 5600) it takes up to 1hr to run the full test suite. Maybe we should isolate just a couple important tests (JsString, boa_gc, modules using unsafe) and ignore all others on miri runs.

@blobcode
Copy link
Author

It looks like according to
rust-lang/miri#1758
we likely will need to add miri to the names of tests we want to run.
If someone could let me know more specifically what tests we want to run through miri to start that would be appreciated.

@jedel1043
Copy link
Member

jedel1043 commented Nov 29, 2023

Really sorry for the wait 😅 We'll probably want to just run the tests for JsString and the boa_gc tests, which are the most "unsafe" parts of our engine. Further tests can be added to the list as we add them to the repo.

We think it should be better to exclude whole modules from miri runs using #[cfg_attr(miri, ignore)], since it saves us the pain of categorizing tests individually.

@jedel1043 jedel1043 added the waiting-on-author Waiting on PR changes from the author label Nov 29, 2023
@BlueSialia
Copy link

Hello, I was looking at the good first issue tasks to be able to star collaborating. I found this, almost finished PR? Looks to me like it's only necessary to undo 323eca2 and add #[cfg_attr(miri, ignore)] to a bunch of files. Although I can't discern which are the files related to JsString and boa_gc tests yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting-on-author Waiting on PR changes from the author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deny warnings and run miri in the CI
3 participants