Skip to content

Sysinfo improvements #9243

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

Closed

Conversation

GuillaumeGomez
Copy link
Contributor

Keeping a hard-set list of supported targets might not be the best, it'd force the code to be updated every time sysinfo supports a new target (which doesn't happen often but I guess it's better to consider it?). For example, sysinfo supports FreeBSD but it wasn't listed in the supported targets. So instead, just listing known problematic targets in the cfgs seems like a better strategy. But maybe it's not what you prefer? In any case, very curious to hear more about it.

For the rest, I simply updated sysinfo version since it got a lot of fixes (mostly Windows).

@GuillaumeGomez
Copy link
Contributor Author

The miri failure is quite impressive. O.o

@mockersf
Copy link
Member

The miri failure should be fixed in tomorrow nightly by rust-lang/rust#113946

@GuillaumeGomez
Copy link
Contributor Author

I'm realizing that the dynamic_linking feature is used the wrong way: cargo features are supposed to be additive. Having the opposite (like no_dynamic_linking) would allow to write:

[features]
no_dynamic_linking = ["sysinfo"]
default-features = ["no_dynamic_linking"]

[dependencies]
sysinfo = { version = "0.29.6", optional = true }

And then in the code we could simply check with cfg(feature = "no_dynamic_linking"). Simplifying the cfg conditions even further.

@mockersf
Copy link
Member

In other part of the engine dynamic_linking is additive, it's only an issue here because sysinfo fails with it.

As far as I know It's not possible to have a feature expose globally (dynamic_linking) that would enable no_dynamic_linking in a dependency when it's not present

Comment on lines +24 to +25
# iOS
[target.'cfg(all(target_os="ios"))'.dependencies]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't this also needed for macOS? from sysinfo docs it seems it is https://docs.rs/sysinfo/latest/sysinfo/#use-in-binaries-running-inside-the-macos-or-ios-sandboxstores

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only if running it in a VM. Talk about a nightmare...

@mockersf mockersf added the A-Diagnostics Logging, crash handling, error reporting and performance analysis label Jul 23, 2023
@alice-i-cecile alice-i-cecile added C-Code-Quality A section of code that is hard to understand or change C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Jul 31, 2023
@GuillaumeGomez
Copy link
Contributor Author

Closing as I don't think this is going anywhere for the time being. I'll send a PR once the new sysinfo release will be done.

@GuillaumeGomez GuillaumeGomez deleted the sysinfo-improvements branch November 8, 2023 12:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Diagnostics Logging, crash handling, error reporting and performance analysis C-Code-Quality A section of code that is hard to understand or change C-Usability A targeted quality-of-life change that makes Bevy easier to use
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants