[Merged by Bors] - add system information plugin and update relevant examples #5911

l1npengtul · 2022-09-08T02:28:03Z

Objective

Solve #5464

Solution

Adds a SystemInformationDiagnosticsPlugin to add diagnostics.

Adds Cargo.toml flags to fix building on different platforms.

Changelog

Adds sysinfo crate to bevy-diagnostics.

Changes in import order are due to clippy.

l1npengtul · 2022-09-08T02:28:18Z

NOTE: possibly blocked by #5454

IceSentry · 2022-09-08T02:31:50Z

It's not blocked on the log PR since this does something separate.

The general case config doesn't work on windows though. I simply used the default config for the general case. I think cargo is smart enough to pick the most applicable config.

IceSentry · 2022-09-08T02:33:38Z

There's also still the potential issue that the sysinfo crate doesn't work when used with bevy_dynamic. I can't reproduce it myself though.

robem · 2022-09-08T02:34:11Z

examples/diagnostics/log_diagnostics.rs

@@ -17,5 +17,7 @@ fn main() {
        // .add_plugin(bevy::diagnostic::EntityCountDiagnosticsPlugin::default())
        // Uncomment this to add an asset count diagnostics:
        // .add_plugin(bevy::asset::diagnostic::AssetCountDiagnosticsPlugin::<Texture>::default())
+        // Uncomment this to add an System Info diagnostics:


Nit: an System Info -> system info.

Suggested change

// Uncomment this to add an System Info diagnostics:

// Uncomment this to add system info diagnostics:

Ah, thanks for catching that.

l1npengtul · 2022-09-08T02:37:42Z

There's also still the potential issue that the sysinfo crate doesn't work when used with bevy_dynamic. I can't reproduce it myself though.

Ill try on my machine. Worst case sceneario, we throw away sys info and use the raw-platform APIs ourselves.

Or just disable the whole thing if using dynamic.

IceSentry · 2022-09-08T02:41:47Z

I'd much prefer just disabling it when using dynamic or just have a separate feature flag for the crate. Based on the blog post of sysinfo, getting all of this isn't trivial and I don't think it's worth it to go through the trouble of reimplementing it ourselves.

Nilirad · 2022-09-20T19:45:56Z

Adding S-Needs-Investigation since it has the same problem of #5454 regarding dynamic linking.

targrub · 2022-11-30T13:34:21Z

With the advancement in dynamic linking on Windows, is this ready for investigation?
#2921

alice-i-cecile · 2022-11-30T13:39:52Z

Yep, I think we can take a proper look at this now :)

l1npengtul · 2022-11-30T13:51:47Z

I'll take another look at this once I am free

l1npengtul · 2022-12-02T13:04:51Z

My last comment was 2 days ago... I swore it was today...

Anyway, I have confirmed it working on my computer and I do not know how to solve the failing CI check.

I believe this is ready for a merge, if someone were to test this on Mac and Windows just to make sure...

mockersf · 2022-12-03T00:45:24Z

and wasm, android and iOS?

crates/bevy_diagnostic/src/system_information_diagnostics_plugin.rs

…in.rs Co-authored-by: François <[email protected]>

l1npengtul · 2022-12-03T16:23:59Z

Yep... forgot about those :p

WASM is the exception, sysinfo doesn't support WASM.

IceSentry · 2022-12-23T07:43:43Z

@l1npengtul I'd suggest updating the cargo.toml based on the one in #5454 after some testing it looks like we can use a simpler cargo.toml, but also, we can disable default-features. I disabled it because the only thing it does is enable multithreading, which causes a dependency conflict and we don't really need mutlithreading for this anyway.

Also, did you test it with dynamic linking enabled? If so, please leave a comment on the other PR. It will help both of these to get merged faster if we can confirm that it works.

l1npengtul · 2022-12-23T07:47:14Z

@l1npengtul I'd suggest updating the cargo.toml based on the one in #5454 after some testing it looks like we can use a simpler cargo.toml, but also, we can disable default-features. I disabled it because the only thing it does is enable multithreading, which causes a dependency conflict and we don't really need mutlithreading for this anyway.

Also, did you test it with dynamic linking enabled? If so, please leave a comment on the other PR. It will help both of these to get merged faster if we can confirm that it works.

I think I did, I'll do it again to make sure.

IceSentry

I tested it on windows and wasm and it worked as expected.

On wasm, it gives a NaN unfortunately, but it doesn't break anything otherwise

crates/bevy_diagnostic/src/system_information_diagnostics_plugin.rs

…em error instead of white space

l1npengtul · 2022-12-28T07:30:23Z

I believe this is ready for merging.

crates/bevy_diagnostic/Cargo.toml

IceSentry · 2022-12-30T04:28:12Z

@l1npengtul I pushed a PR to your branch to remove the code duplication. I believe it's gonna be in a good state after that.

I also tried adding the log_sysinfo to the module, but it doesn't really work since the intention was for that log to always be there so I kept them separate.

Remove duplication

l1npengtul · 2022-12-30T07:11:17Z

Thank you IceSentry!

IceSentry · 2023-01-02T19:23:18Z

My previous PR broke something, so I made a new PR to your repo. I went back a bit to having 2 mod internal but still extracted the common part and moved the log_system_info inside. This one should be good to go now.

fix cfg sys usage

IceSentry · 2023-01-02T19:39:23Z

Green CI 🎉

IceSentry · 2023-01-02T19:39:31Z

bors try

l1npengtul · 2023-01-02T19:40:19Z

Thank you IceSentry!!

bors · 2023-01-02T19:57:25Z

try

Build succeeded!

And happy new year! 🎉

alice-i-cecile · 2023-01-02T20:49:25Z

bors r+

# Objective Solve #5464 ## Solution Adds a `SystemInformationDiagnosticsPlugin` to add diagnostics. Adds `Cargo.toml` flags to fix building on different platforms. --- ## Changelog Adds `sysinfo` crate to `bevy-diagnostics`. Changes in import order are due to clippy. Co-authored-by: l1npengtul <[email protected]> Co-authored-by: IceSentry <[email protected]>

bors · 2023-01-02T21:07:26Z

Pull request successfully merged into main.

Build succeeded!

And happy new year! 🎉

…e#5911) # Objective Solve bevyengine#5464 ## Solution Adds a `SystemInformationDiagnosticsPlugin` to add diagnostics. Adds `Cargo.toml` flags to fix building on different platforms. --- ## Changelog Adds `sysinfo` crate to `bevy-diagnostics`. Changes in import order are due to clippy. Co-authored-by: l1npengtul <[email protected]> Co-authored-by: IceSentry <[email protected]>

# Problemo Some code in #5911 and #5454 does not compile with dynamic linking enabled. The code is behind a feature gate to prevent dynamically linked builds from breaking, but it's not quite set up correctly. ## Solution Forward the `dynamic` feature flag to the `bevy_diagnostic` crate and gate the code behind it. Co-authored-by: devil-ira <[email protected]>

…e#5911) # Objective Solve bevyengine#5464 ## Solution Adds a `SystemInformationDiagnosticsPlugin` to add diagnostics. Adds `Cargo.toml` flags to fix building on different platforms. --- ## Changelog Adds `sysinfo` crate to `bevy-diagnostics`. Changes in import order are due to clippy. Co-authored-by: l1npengtul <[email protected]> Co-authored-by: IceSentry <[email protected]>

# Problemo Some code in bevyengine#5911 and bevyengine#5454 does not compile with dynamic linking enabled. The code is behind a feature gate to prevent dynamically linked builds from breaking, but it's not quite set up correctly. ## Solution Forward the `dynamic` feature flag to the `bevy_diagnostic` crate and gate the code behind it. Co-authored-by: devil-ira <[email protected]>

add system information plugin and update relevant examples

59ec59c

robem reviewed Sep 8, 2022

View reviewed changes

IceSentry added A-Diagnostics Logging, crash handling, error reporting and performance analysis C-Dependencies A change to the crates that Bevy depends on labels Sep 8, 2022

fix docs and typo

b7c5680

Nilirad added the S-Needs-Investigation This issue requires detective work to figure out what's going wrong label Sep 20, 2022

zeroacez mentioned this pull request Sep 27, 2022

Add more system information as a diagnostics #5464

Closed

alice-i-cecile removed the S-Needs-Investigation This issue requires detective work to figure out what's going wrong label Nov 30, 2022

l1npengtul added 3 commits December 2, 2022 08:28

Merge branch 'main' into sys-usage-info

0059b34

Merge branch 'bevyengine:main' into sys-usage-info

ca5b05a

fix messed up cargo toml

19b6f31

mockersf reviewed Dec 3, 2022

View reviewed changes

crates/bevy_diagnostic/src/system_information_diagnostics_plugin.rs Outdated Show resolved Hide resolved

Update crates/bevy_diagnostic/src/system_information_diagnostics_plug…

28dee0e

…in.rs Co-authored-by: François <[email protected]>

l1npengtul added 2 commits December 23, 2022 16:45

Merge branch 'bevyengine:main' into sys-usage-info

a110617

simplify cargo toml according to 5454

c008554

IceSentry approved these changes Dec 23, 2022

View reviewed changes

crates/bevy_diagnostic/src/system_information_diagnostics_plugin.rs Outdated Show resolved Hide resolved

l1npengtul added 2 commits December 28, 2022 16:14

madotsuki you forgot to derive madotsuki no dont go to the balcony

08b5295

sunny removes the unused import warningand fixes the nonexistant syst…

7fc4b25

…em error instead of white space

mockersf reviewed Dec 28, 2022

View reviewed changes

crates/bevy_diagnostic/Cargo.toml Outdated Show resolved Hide resolved

l1npengtul and others added 3 commits December 28, 2022 17:23

add back #bevy

1ae255e

remove code duplication

59f3e4c

clean up

b37721a

Merge pull request #1 from IceSentry/log-sysinfo-usage

888c26c

Remove duplication

fix cfg sys usage

c51e9fc

Merge pull request #2 from IceSentry/fix-sysuage-info

4cc8ab7

fix cfg sys usage

bors bot added a commit that referenced this pull request Jan 2, 2023

Try #5911:

fa90c9b

alice-i-cecile approved these changes Jan 2, 2023

View reviewed changes

alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jan 2, 2023

bors bot changed the title ~~add system information plugin and update relevant examples~~ [Merged by Bors] - add system information plugin and update relevant examples Jan 2, 2023

bors bot closed this Jan 2, 2023

tim-blackbird mentioned this pull request Jan 22, 2023

[Merged by Bors] - Fix dynamic linking (on linux) #7333

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - add system information plugin and update relevant examples #5911

[Merged by Bors] - add system information plugin and update relevant examples #5911

l1npengtul commented Sep 8, 2022

l1npengtul commented Sep 8, 2022

IceSentry commented Sep 8, 2022

IceSentry commented Sep 8, 2022

robem Sep 8, 2022

alice-i-cecile Sep 8, 2022

l1npengtul Sep 8, 2022

l1npengtul commented Sep 8, 2022 •

edited

Loading

IceSentry commented Sep 8, 2022

Nilirad commented Sep 20, 2022

targrub commented Nov 30, 2022

alice-i-cecile commented Nov 30, 2022

l1npengtul commented Nov 30, 2022

l1npengtul commented Dec 2, 2022

mockersf commented Dec 3, 2022

l1npengtul commented Dec 3, 2022

IceSentry commented Dec 23, 2022 •

edited

Loading

l1npengtul commented Dec 23, 2022

IceSentry left a comment

l1npengtul commented Dec 28, 2022

IceSentry commented Dec 30, 2022

l1npengtul commented Dec 30, 2022

IceSentry commented Jan 2, 2023

IceSentry commented Jan 2, 2023

IceSentry commented Jan 2, 2023

l1npengtul commented Jan 2, 2023

bors bot commented Jan 2, 2023

alice-i-cecile commented Jan 2, 2023

bors bot commented Jan 2, 2023

	// Uncomment this to add an System Info diagnostics:
	// Uncomment this to add system info diagnostics:

[Merged by Bors] - add system information plugin and update relevant examples #5911

[Merged by Bors] - add system information plugin and update relevant examples #5911

Conversation

l1npengtul commented Sep 8, 2022

Objective

Solution

Changelog

l1npengtul commented Sep 8, 2022

IceSentry commented Sep 8, 2022

IceSentry commented Sep 8, 2022

robem Sep 8, 2022

Choose a reason for hiding this comment

alice-i-cecile Sep 8, 2022

Choose a reason for hiding this comment

l1npengtul Sep 8, 2022

Choose a reason for hiding this comment

l1npengtul commented Sep 8, 2022 • edited Loading

IceSentry commented Sep 8, 2022

Nilirad commented Sep 20, 2022

targrub commented Nov 30, 2022

alice-i-cecile commented Nov 30, 2022

l1npengtul commented Nov 30, 2022

l1npengtul commented Dec 2, 2022

mockersf commented Dec 3, 2022

l1npengtul commented Dec 3, 2022

IceSentry commented Dec 23, 2022 • edited Loading

l1npengtul commented Dec 23, 2022

IceSentry left a comment

Choose a reason for hiding this comment

l1npengtul commented Dec 28, 2022

IceSentry commented Dec 30, 2022

l1npengtul commented Dec 30, 2022

IceSentry commented Jan 2, 2023

IceSentry commented Jan 2, 2023

IceSentry commented Jan 2, 2023

l1npengtul commented Jan 2, 2023

bors bot commented Jan 2, 2023

try

alice-i-cecile commented Jan 2, 2023

bors bot commented Jan 2, 2023

l1npengtul commented Sep 8, 2022 •

edited

Loading

IceSentry commented Dec 23, 2022 •

edited

Loading