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

Use oslog for ios #13364

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Conversation

therealbnut
Copy link

Objective

On mobile devices, it's best to use the OS's native logging due to the difficulty of accessing the console. This is already done for Android.

This is an updated version of #4462.

Solution

This PR uses Absolucy's tracing-oslog (ZLib license) for iOS in order to use Apple's os_log.

Testing

I ran examples/mobile with the logging from examples/app/logs.rs on an iOS device, I then checked the logs could be filtered in the MacOS Console.app.

Changelog

  • Change bevy_log to use Apple's os_log on iOS.

Questions for Reviewers

It's worth noting that the dependency this adds hasn't had bug fixes released in a few years, so we may want to consider one or more of:

  1. a feature flag to opt-in, and it would also allow os_log on MacOS
  2. merge as-is and have some (minor?) upstream bugs
  3. hold off on this PR until a suitable alternative dependency arises
  4. maintain our own implementation

Future work

In a follow-up PR it might be good to make the subsystem field have a better default value, like this one. That value can be retrieved programmatically if we bind another system API (For posterity in Swift this is Bundle.main.bundleIdentifier, but the C/ObjC equivalent is likely easier to bind). This would almost always be the correct value, while the current default is unlikely to ever be correct.

@alice-i-cecile
Copy link
Member

I don't have the experience with iOS to be able to meaningfully review this, but I can comment on the dependency question.

It's worth noting that the dependency this adds hasn't had bug fixes released in a few years

Great question! So, in the interim, I'm in favor of proceeding as is and living with the small bugs. Longer term, we should work with robius to get this crate adopted by a shared community working on supporting cross-platform app development in Rust. @kevinaboos gave a talk about their work at RustNL and this seems like a prime candidate.

@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior O-iOS Specific to the iOS mobile operating system A-Diagnostics Logging, crash handling, error reporting and performance analysis X-Contentious There are nontrivial implications that should be thought through D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes labels May 14, 2024
@therealbnut
Copy link
Author

Longer term, we should work with robius

That looks like an interesting project, thanks for your feedback!

Copy link
Contributor

@IceSentry IceSentry left a comment

Choose a reason for hiding this comment

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

The implementation looks good to me, but I don't have an iOS device to test it. Personally, I'd be tempted to say to put it behind a feature flag, but I'll let the iOS specialist decide that.

@mockersf
Copy link
Member

mockersf commented May 14, 2024

there's a bug in the current released version of tracing-oslog that makes it randomly crash in a multithreaded app so... that will be blocked for now

it also brings two rust security advisories that all users of Bevy would have

@mockersf mockersf added the S-Blocked This cannot move forward until something else changes label May 14, 2024
@therealbnut
Copy link
Author

@mockersf yeah, I agree, and the security warning about an old bindgen version in the same crate.

I believe the crash you’re referring to is fixed on main, just not released? It also wouldn’t be hard to update the bindgen and other dependencies.

Maybe getting a new crate release is the real blocker. Alice had some suggestions there, but I’m not sure where to start on that.

@mockersf
Copy link
Member

Yup...

Maybe getting a new crate release is the real blocker

You can try opening an issue, or trying to reach the crate author. they have a few contact points on their GitHub profile, and seem active

@therealbnut
Copy link
Author

You can try opening an issue, or trying to reach the crate author

There's already an issue which has been open for about a year.

I've just made a PR to add Default to OsLogger (to address my "Future work" suggestion). If that goes well I'll ask if they mind publishing a release :)

@kevinaboos
Copy link

I don't have the experience with iOS to be able to meaningfully review this, but I can comment on the dependency question.

It's worth noting that the dependency this adds hasn't had bug fixes released in a few years

Great question! So, in the interim, I'm in favor of proceeding as is and living with the small bugs. Longer term, we should work with robius to get this crate adopted by a shared community working on supporting cross-platform app development in Rust. @kevinaboos gave a talk about their work at RustNL and this seems like a prime candidate.

Definitely, we'd be more than happy to dig into this and maintain such a crate. Haven't dealt with logging on iOS specifically in too much detail yet, but we'd be up for doing so.

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-Bug An unexpected or incorrect behavior D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes O-iOS Specific to the iOS mobile operating system S-Blocked This cannot move forward until something else changes X-Contentious There are nontrivial implications that should be thought through
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants