-
-
Notifications
You must be signed in to change notification settings - Fork 366
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 Date/Time on Android Device for Fallback Logging #1356
Conversation
The Briefcase Android template currently has a minimum API level of 24, so if you can confirm it works on an emulator with that version, that should be good enough. |
Thanks. I changed |
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.
Looks good for the most part; one question about timezone, since it wasn't immediately obvious; it might not be revealed in testing because you're running locally UTC negative will work in your favor for testing purposes.
"""Obtain the device's current date/time.""" | ||
try: | ||
# request datetime in seconds since Unix epoch | ||
return datetime.fromtimestamp(int(self.run("shell", "date", "+%s"))) |
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.
Just confirming: is this using UTC epoch or local timezone epoch?
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.
hmm...so, if datetime.fromtimestamp()
isn't given a timezone, then it returns a naive date/time for the platform's date/time configuration.
In practice, this assumes the device and the host machine are configured for the same timezone....and daylight saving...and whatever else....
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 suppose this requirement could be avoided if a specific format is requested from date
on the device and an equivalent format is specified for datetime.strptime()
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.
Look, we can all agree that time was invented by the devil to punish programmers. :-)
We could definitely go to great lengths to ensure timezone compliant round trips - but I'm not sure that's actually required. We just need to make sure that the time that comes back from adb shell date
is in the same timezone as the log file filter. If that's true with fromtimestamp()
, then I'm 99% happy; the remaining 1% is to document the new datetime()
endpoint to make sure nobody tries to use it for anything else in future.
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.
Look, we can all agree that time was invented by the devil to punish programmers. :-)
If that isn't the truth....
We could definitely go to great lengths to ensure timezone compliant round trips - but I'm not sure that's actually required. We just need to make sure that the time that comes back from
adb shell date
is in the same timezone as the log file filter. If that's true withfromtimestamp()
, then I'm 99% happy; the remaining 1% is to document the newdatetime()
endpoint to make sure nobody tries to use it for anything else in future.
Well...I guess as long as 99% of the time users' devices have date/time configurations that match their machines that are running Briefcase....then we're good here :/
This is why I had to commit c2e4b6b after pushing; I discovered that GitHub's CI runners are configured for UTC (as opposed to my EDT) and were returning a different date/time for the same value returned from date
on the device.
I could play around with explicit date/time formats tomorrow to see what this takes.
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.
ed9c234 will ensure this logging date/time is local to the device.
- The Unix epoch is always UTC and `fromtimestamp()` returns a date/time that's local to the machine; therefore, if the timezone of the machine is different than the device, the date/time will not match the local date/time of the device. - To avoid this, a specific date/time format is requested from the device so it can be parsed on the machine in a timezone-unaware fashion.
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 can't reliably reproduce my original problem now, and I suspect it may actually have been an intermittent issue with it failing to find the PID even when the app is running. But anyway, this all looks fine to me.
fwiw...I was only able to not find a PID when I commented out Though, on a sufficiently slow machine (like my Pi 4b), every once in a while, it doesn't find a PID in 5 seconds and then runs |
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.
Looks good to me. The failure mode you describe on RPi/slow machines is definitely a problem, but it's sufficiently edge case that I can definitely live with it for now. The only immediately obvious fix I can think of would be to increase the 5s window to something more, but 5s is already inconvenient for people with fast machines, and there's essentially no delay that will be long enough for a sufficiently slow machine.
If we can find a different marker that allows us to differentiate between "The app has crashed" and "the app hasn't started... yet", that would be great - but in the meantime, this will be a significant improvement for the 95% case of app developers who have an app failing on startup and a device with a clock discrepancy.
Changes
Notes
date
command has supported this formatting for as long as we've needed it to.PR Checklist: