-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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] - log system info on startup #5454
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good, and this is a nice feature.
I could be convinced that this should either a) only be enabled in debug mode or b) be part of a custom panic handler but I'm also fine with this design (and it's much simpler).
The panic handler part should be in another PR in my opinion. I don't like debug only because a lot of people just run in release mode for performance reason even if it's not recommended. I should probably figure out a way to disable it with a feature or something. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tested and confirmed that this works as expected with the stats
SystemInfo { os: "Windows 10 Pro", kernel: "19044", cpu: "Intel(R) Core(TM) i7-8700K CPU @ 3.70GHz", core_count: "6", memory: "34294992 KB" }
Diagnostic information is working properly for me on Windows 10 too :) |
ok on MacOS 👍 |
I am OK with leaving the logging code even on release mode. It is useful for user bug reporting after the software has been shipped. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Strangely enough, this seems to break dynamically linked builds on my end (it doesn't happen on main
).
My system info:
SystemInfo { os: "Linux rolling Arch Linux", kernel: "5.18.12-arch1-1", cpu: "Intel(R) Core(TM) i7-3770K CPU @ 3.50GHz", core_count: "4", memory: "15.6 GiB" }
I think it should be printed in a prettier manner.
@Nilirad I tried to just copy what the AdapterInfo did. How would you want it printed instead? I like the idea that it was a single easy to copy line. |
For now, having both reports (CPU/system & GPU) being debug info is good enough. Some time in the future, we'll need a proper error reporting method, but that's not it yet |
I was thinking about a pretty-printed struct, with each field in its own line. But I guess all the other occurrences of logged values are also one-liners just like you did. At this point I think it is OK as it is. |
Does anybody else experience linking errors when building with the dynamic feature? EDIT: I'm on nightly. |
I haven't tried it since dynamic loading doesn't work great on windows. At least it didn't work last time I tried. |
Would this be better as it's own plugin that queries and writes Resources? I could imagine other parts of bevy and the community wanting to get at this information in a structured way as well. |
I tried to get this out as fast as possible, because it's main purpose right now is to have more info when users reports an issue, but yes it would be easy to make it as a plugin that also inserts the result in a Res. I would do that in a future PR though |
We could even do things like record the amount of used and free memory, cpu usage and things like that as diagnostics. It was just out of scope for this PR. |
🎫? |
Done #5464 @alice-i-cecile why did you remove the ready for final review tag? Is it because of the potential issue with dynamic loading? |
Yes; I want verification on that. |
I'll try to give it a deeper look tomorrow or the day after, depending on how much free time I'll have. |
I tried to build with different configurations, none of which was successful. cargo 1.64.0-nightly (85b500cca 2022-07-24), Fast builds config ON (LLD linker + share generics)
cargo 1.64.0-nightly (85b500cca 2022-07-24), Fast builds config OFF
cargo 1.62.1 (a748cf5a3 2022-06-08)
|
It looks like the crate is doing some native calls which aren't linked. I honestly do not know how to resolve that. I'm not really familiar with all the dylib stuff 😦 |
@bjorn3 I think you're the most suited person to figure out what's going on here. |
Nope, only in the final attempt or via bors try |
tryBuild failed: |
I tried building locally for iOS, main works but this branch fails with (mostly) the same error as in CI (differences can be explained by the fact that CI is on an intel mac and I have an arm mac). Building directly for an iOS target works, but building through Xcode doesn't. When coming from Xcode, we update the library path: bevy/examples/ios/build_rust_deps.sh Lines 16 to 22 in b6066c3
Without this, |
eb8f951
to
63b60ea
Compare
So in order to get this merged, I made it so that it doesn't compile anything on iOS or when using bevy_dynamic. It's not perfect, but at least we'll still get the log in a lot of cases. I'd love to have it on more platforms, but I have no way to test either of those platform so unless someone wants to adopt this I can't do much more. |
bors try |
tryBuild failed: |
bors try |
Can you open a follow up issue to get this working on more platforms? |
# Objective - We already log the adapter info on startup when bevy_render is present. It would be nice to have more info about the system to be able to ask users to submit it in bug reports ## Solution - Use the `sysinfo` crate to get all the information - I made sure it _only_ gets the required informations to avoid unnecessary system request - Add a system that logs this on startup - This system is currently in `bevy_diagnostics` because I didn't really know where to put it. Here's an example log from my system: ```log INFO bevy_diagnostic: SystemInformation { os: "Windows 10 Pro", kernel: "19044", cpu: "AMD Ryzen 7 5800X 8-Core Processor", core_count: "8", memory: "34282242 KB" } ``` --- ## Changelog - Added a new default log when starting a bevy app that logs the system information
To be noted that for iOS most of sysinfo features are disabled because of how apps are run, preventing them to access a lot of information. |
I have experience writing OS bindings, if anyone needs help on that feel free to tag me. |
# Objective - We already log the adapter info on startup when bevy_render is present. It would be nice to have more info about the system to be able to ask users to submit it in bug reports ## Solution - Use the `sysinfo` crate to get all the information - I made sure it _only_ gets the required informations to avoid unnecessary system request - Add a system that logs this on startup - This system is currently in `bevy_diagnostics` because I didn't really know where to put it. Here's an example log from my system: ```log INFO bevy_diagnostic: SystemInformation { os: "Windows 10 Pro", kernel: "19044", cpu: "AMD Ryzen 7 5800X 8-Core Processor", core_count: "8", memory: "34282242 KB" } ``` --- ## Changelog - Added a new default log when starting a bevy app that logs the system information
# 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]>
# Objective - We already log the adapter info on startup when bevy_render is present. It would be nice to have more info about the system to be able to ask users to submit it in bug reports ## Solution - Use the `sysinfo` crate to get all the information - I made sure it _only_ gets the required informations to avoid unnecessary system request - Add a system that logs this on startup - This system is currently in `bevy_diagnostics` because I didn't really know where to put it. Here's an example log from my system: ```log INFO bevy_diagnostic: SystemInformation { os: "Windows 10 Pro", kernel: "19044", cpu: "AMD Ryzen 7 5800X 8-Core Processor", core_count: "8", memory: "34282242 KB" } ``` --- ## Changelog - Added a new default log when starting a bevy app that logs the system information
# 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]>
Objective
Solution
sysinfo
crate to get all the informationbevy_diagnostics
because I didn't really know where to put it.Here's an example log from my system:
Changelog