fix: disable system param tests in miri

[nelson137]

The new system param integration tests added in #482 fail in miri due to features unavailable in isolation mode. Disable those tests since there isn't a good way to fix this.

[nelson137]

I went down the rabbit hole of trying to fix this but ultimately couldn't get it working. Here's what I found on the off chance this is useful in the future:

The tests fail with

error: unsupported operation: `getcwd` not available when isolation is enabled

Which comes from FileAssetIo so I implemented a test asset IO with hard-coded asset files:

Code

Which you can use by doing asset_server.set_io(TestAssetIo)

static TEST_ASSET_STORE: std::sync::LazyLock<HashMap<(Option<String>, PathBuf), String>> =
    std::sync::LazyLock::new(|| {
        let mut map = HashMap::default();

        let mut insert = |pack: Option<&str>, path: &str, contents: &str| {
            map.insert(
                (pack.map(ToOwned::to_owned), path.into()),
                contents.to_owned(),
            )
        };

        // Core Pack /////////////////////////////////////////////////
        insert(
            None,
            "pack.yaml",
            "
root: game.yaml
",
        );
        insert(
            None,
            "/game.yaml",
            "
data: abc
important_numbers: [1, 2, 3]
",
        );

        // Pack 1 ////////////////////////////////////////////////////
        insert(
            Some("pack1"),
            "pack.yaml",
            "
name: pack1
id: pack1_01j9jyse2qf1sspywwfe0x1r5j
version: 0.0.0
game_version: 0.0.0
root: root.yaml
schemas: []
",
        );
        insert(
            Some("pack1"),
            "/root.yaml",
            "
label: First Pack
other_numbers: [10, 20, 30]
",
        );

        // Pack 2 ////////////////////////////////////////////////////
        insert(
            Some("pack2"),
            "pack.yaml",
            "
name: pack2
id: pack2_01j9msqxzjf38t26006w9xr0t5
version: 0.0.0
game_version: 0.0.0
root: root.yaml
schemas: []
",
        );
        insert(
            Some("pack2"),
            "/root.yaml",
            "
label: Second Pack
other_numbers: [100, 200, 300]
",
        );

        map
    });

struct TestAssetIo;

impl AssetIo for TestAssetIo {
    fn enumerate_packs(&self) -> BoxedFuture<anyhow::Result<Vec<String>>> {
        async move {
            let packs = TEST_ASSET_STORE
                .keys()
                .filter_map(|(pack, _)| pack.clone())
                .collect();
            anyhow::Ok(packs)
        }
        .boxed()
    }

    fn load_file(&self, loc: AssetLocRef) -> BoxedFuture<anyhow::Result<Vec<u8>>> {
        let loc = loc.to_owned();
        let contents = TEST_ASSET_STORE.get(&(loc.pack.clone(), loc.path.clone()));
        async move {
            contents.map(|c| c.as_bytes().to_vec()).ok_or_else(|| {
                anyhow::anyhow!(
                    "Unexpected asset location: pack={:?} path={}",
                    loc.pack,
                    loc.path.display()
                )
            })
        }
        .boxed()
    }
}

And now it fails with

error: unsupported operation: `clock_gettime` with `REALTIME` clocks not available when isolation is enabled

Which I fixed by making theUlid::create extension method use an AtomicU64 for the timestamp that increments every time it's called:

Code

impl UlidExt for Ulid {
    fn create() -> Self {
        #[cfg(not(feature = "miri"))]
        let timestamp = instant::now().floor() as u64;
        #[cfg(feature = "miri")]
        let timestamp = {
            use std::sync::atomic;
            static TEST_NOW: atomic::AtomicU64 = atomic::AtomicU64::new(0);
            TEST_NOW.fetch_add(1, atomic::Ordering::AcqRel)
        };

        Ulid::from_parts(timestamp, THREAD_RNG.with(|rng| rng.gen_u128()))
    }
}

which uses a new miri feature which had to be added to bones_utils:

[features]
...
miri = []

and enabled by bones_framework:

[features]
...
miri = ["bones_utils/miri"]

[dependencies]
...
bones_utils = { version = "0.4.0", path = "../bones_utils" }

And then this is where I got stuck. All the tests now pass, but it still fails with:

error: the main thread terminated without waiting for all remaining threads

which seems to be caused by the fact that the task pool never shutsdown, and there is no interface for requesting it do so (similar problem with rayon) (TaskPool docs).

[MaxCWhitehead]

It looks like the miri test is still being run / failing in CI - but locally it seems like it is ignored by miri. Not exactly sure what is going on here.

[MaxCWhitehead]

Seems to be checking out target branch main without the changes here:

Fetching the repository
  /usr/bin/git -c protocol.version=2 fetch --no-tags --prune --progress --no-recurse-submodules --depth=1 origin +560c25972ad335bfc26f03d088b714806caf11b8:refs/remotes/origin/main

I wonder if this is an issue with using pull_request_target instead of pull_request:

bones/.github/workflows/pull-requests.yml

Lines 4 to 10 in 560c259

	pull_request_target:
	types:
	- opened
	- edited
	- synchronize
	merge_group:

I might try changing this and pushing here to see if fixes. other jobs from ci.yml file seem to do the merged branch when checking out.

[MaxCWhitehead]

Seems like it is now running with both pull_request and pull_request_target (I assume the pull_request_target) version will go away once this is merged? Should be ok to merge if that passes I think.