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

[Merged by Bors] - CI runs cargo miri test -p bevy_ecs #4310

Closed
wants to merge 11 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/bors.toml
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ status = [
"check-missing-examples-in-docs",
"check-unused-dependencies",
"ci",
"miri",
"check-benches",
]

Expand Down
32 changes: 32 additions & 0 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,38 @@ jobs:
# See tools/ci/src/main.rs for the commands this runs
run: cargo run -p ci -- nonlocal

miri:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v3
- uses: actions/cache@v2
with:
path: |
~/.cargo/bin/
~/.cargo/registry/index/
~/.cargo/registry/cache/
~/.cargo/git/db/
target/
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to not cache ~/.cargo/{.crates2.json,.crates.toml} so I assume it will still rebuild xargo each time? I am not sure how caching works for GHA though.

Copy link
Member

Choose a reason for hiding this comment

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

the list of paths to cache is from https://github.com/actions/cache/blob/main/examples.md#rust---cargo... maybe it's not complete?

key: ${{ runner.os }}-cargo-ci-${{ hashFiles('**/Cargo.toml') }}
- uses: actions-rs/toolchain@v1
with:
toolchain: nightly
components: miri
override: true
- name: Install alsa and udev
run: sudo apt-get update; sudo apt-get install --no-install-recommends libasound2-dev libudev-dev libwayland-dev libxkbcommon-dev
- name: CI job
run: cargo miri test -p bevy_ecs
env:
# -Zrandomize-layout makes sure we dont rely on the layout of anything that might change
RUSTFLAGS: -Zrandomize-layout
# -Zmiri-disable-isolation is needed because our executor uses `fastrand` which accesses system time.
# -Zmiri-ignore-leaks is needed because running bevy_ecs tests finds a memory leak but its impossible
# to track down because allocids are nondeterministic.
# -Zmiri-tag-raw-pointers is not strictly "necessary" but enables a lot of extra UB checks relating
# to raw pointer aliasing rules that we should be trying to uphold.
MIRIFLAGS: -Zmiri-disable-isolation -Zmiri-ignore-leaks -Zmiri-tag-raw-pointers
BoxyUwU marked this conversation as resolved.
Show resolved Hide resolved

check-benches:
runs-on: ubuntu-latest
needs: ci
Expand Down
14 changes: 12 additions & 2 deletions crates/bevy_ecs/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -637,8 +637,18 @@ mod tests {
#[test]
fn table_add_remove_many() {
let mut world = World::default();
let mut entities = Vec::with_capacity(10_000);
for _ in 0..1000 {
#[cfg(miri)]
let (mut entities, to) = {
let to = 10;
(Vec::with_capacity(to), to)
};
#[cfg(not(miri))]
let (mut entities, to) = {
let to = 10_000;
(Vec::with_capacity(to), to)
};

for _ in 0..to {
entities.push(world.spawn().insert(B(0)).id());
}

Expand Down
2 changes: 1 addition & 1 deletion crates/bevy_tasks/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ keywords = ["bevy"]

[dependencies]
futures-lite = "1.4.0"
event-listener = "2.4.0"
event-listener = "2.5.2"
Copy link
Member

@mockersf mockersf Mar 24, 2022

Choose a reason for hiding this comment

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

dependabot should have warned us of this one... 🤔

async-executor = "1.3.0"
async-channel = "1.4.2"
num_cpus = "1.0.1"
Expand Down
22 changes: 16 additions & 6 deletions crates/bevy_tasks/src/task_pool.rs
Original file line number Diff line number Diff line change
Expand Up @@ -121,13 +121,23 @@ impl TaskPool {
let ex = Arc::clone(&executor);
let shutdown_rx = shutdown_rx.clone();

let thread_name = if let Some(thread_name) = thread_name {
format!("{} ({})", thread_name, i)
} else {
format!("TaskPool ({})", i)
// miri does not support setting thread names
// TODO: change back when https://github.com/rust-lang/miri/issues/1717 is fixed
#[cfg(not(miri))]
let mut thread_builder = {
let thread_name = if let Some(thread_name) = thread_name {
format!("{} ({})", thread_name, i)
} else {
format!("TaskPool ({})", i)
};
thread::Builder::new().name(thread_name)
};
#[cfg(miri)]
let mut thread_builder = {
let _ = i;
let _ = thread_name;
thread::Builder::new()
};

let mut thread_builder = thread::Builder::new().name(thread_name);

if let Some(stack_size) = stack_size {
thread_builder = thread_builder.stack_size(stack_size);
Expand Down