bones icon indicating copy to clipboard operation
bones copied to clipboard

Add CI to validate bones_matchmaker and any other crates in other_crates during pull requests

Open MaxCWhitehead opened this issue 10 months ago • 5 comments

It seems our pull requests CI doesn't validate bones_matchmaker build.

Saw PR CI pass while the bones_matchmaker build/release job fails, should validate this in PR CI.

Issue tracking build failure: #361

MaxCWhitehead avatar Mar 29 '24 15:03 MaxCWhitehead

I'm stumped on this one. For revisions that have this error (cca4233 and before) cargo b --workspace succeeds, you only see the error with cargo b -p bones_matchmaker.

I think the features that the dependencies are compiled with (or maybe even the minor version of a crate if it's duplicated in the tree) may be different depending on what is being compiled. For example, cargo b -p bones_matchmaker fails, but cargo b -p bones_matchmaker -p bones_framework.

I attempted to add a job to compile just bones_matchmaker (here), but for some reason the error disappears! If you want to merge these changes we can, but I don't know how useful it will be since it doesn't seem to catch the errors we want.

I think a better solution would to be to move all dependencies to the root of the workspace and then use the anyhow = { workspace = true } syntax in the crates.

nelson137 avatar Apr 04 '24 02:04 nelson137

@nelson137 definitely a tricky one... We use cargo dep resolver 2 (https://doc.rust-lang.org/cargo/reference/resolver.html#feature-resolver-version-2)

This means that cargo will add feature flags if another thing being built has them.

Problem seemed to be that bones_matchmaker does not have multi-threaded enabled for bevy_tasks, which was added in 0.11 (we recently upgraded from 0.10). I just did a bunch of cargo build -p bones_matchmaker -p bones_other_crate until found pairs that were suspect, it built with bones_scripting which has multi-threaded on too - which helped track it down.

Definitely a huge footgun with resolver 2, no idea what the reasonable way to troubleshoot this is normally. I think the idea behind this resolver is reducing dependencies by unifying some features, but yeah causes headaches like this it seems.

MaxCWhitehead avatar Apr 04 '24 03:04 MaxCWhitehead

I think a better solution would to be to move all dependencies to the root of the workspace and then use the anyhow = { workspace = true } syntax in the crates.

I probably wouldn't do this for everything universally - but I can definitely see value in moving some of the stuff that is not so sensitive around features / version compat with other crates. Idk maybe we can do it for almost everything - but yeah this is indeed a powerful tool and I've used it on other projects and enjoyed it.

MaxCWhitehead avatar Apr 04 '24 04:04 MaxCWhitehead

I think some of the confusion regarding cargo b --workspace might be caused by the fact that the matchmaker isn't in the framework_crates folder and we set the default workspace crates to only include the framework_crates:

https://github.com/fishfolk/bones/blob/6b8d743ae4cc2216a013f6bc39899a60ec9570b1/Cargo.toml#L4


Haven't digested the rest of the details in this issue, just wanted to point that out in case it was causing confusion.

zicklag avatar Apr 05 '24 14:04 zicklag

I think some of the confusion regarding cargo b --workspace might be caused by the fact that the matchmaker isn't in the framework_crates folder and we set the default workspace crates to only include the framework_crates:

On that note --all can be passed to most (all?) workspace commands to target all members instead of just default_members if that is helpful vs creating a new job for matchmaker specifically.

MaxCWhitehead avatar Apr 05 '24 16:04 MaxCWhitehead