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

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

Open
MaxCWhitehead opened this issue Mar 29, 2024 · 5 comments

Comments

@MaxCWhitehead
Copy link
Collaborator

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

@nelson137
Copy link
Contributor

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.

@MaxCWhitehead
Copy link
Collaborator Author

MaxCWhitehead commented Apr 4, 2024

@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
Copy link
Collaborator Author

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.

@zicklag
Copy link
Member

zicklag commented Apr 5, 2024

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:

default-members = ["framework_crates/*"]


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

@MaxCWhitehead
Copy link
Collaborator Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants