#9250 closed patch
OSYSTEM: Add logging API as proposed by Max on -devel
| Reported by: | lordhoto | Owned by: | lordhoto |
|---|---|---|---|
| Priority: | normal | Component: | --Other-- |
| Version: | Keywords: | ||
| Cc: | Game: |
Description
Hello,
this implements the API described in "PROPOSAL: Removing (f)printf usage".
Most of the changes are done, but it might be noteworthy that I was only able to check the compilation and result on Linux
Only the Symbian port still needs adaption. The Symbian port currently has it's own "FatalError" function which takes a string and displays that as an error message to the user and then quits. I am unsure in how to adapt that to the new API though. Also the Symbian port formerly didn't output any warnings at all as far as I can tell (at least there's some check which only called fputs in case in case __SYMBIAN32__ wasn't defined).
Another bits is that in case there's no g_system setup properly yet debug/warning/error won't output anything at all anymore. This sounds bad, at least in case the backend wants to use those for reporting of initialization problems. I am open for suggestions here.
I also noticed that it seems the SamsungTV backend doesn't have a working "exit" implementation, at least it's replaced by an endless loop. Since when there's no g_system we can't call OSystem::fatalError I had to leave that in ::error.
Last but not least I still had to include some backends headers for the default implemtation of OSystem::logMessage, since it relies on fputs and fflush. I am not sure whether we really want that, on the other hand I miss an good idea on how to avoid that right now though (except with offering no default implementation).
Ticket imported from: #3104630. Ticket imported from: patches/1355.
Attachments (1)
Change History (14)
by , 14 years ago
| Attachment: | osystem_log.patch added |
|---|
comment:1 by , 14 years ago
I don't think Symbian should just pop up an dialog in case a error message is passed via logMessage, since actually afterwards the debugger might be opened up with the same message too, of course that highly depends on the engine which is running (i.e. whether it implements an debugger).
Maybe we need some additional option to let the engine worry about that in case we have no error handler setup?
comment:2 by , 14 years ago
That should of course read "let the backend worry about that" and not engine :-). Sorry.
comment:3 by , 14 years ago
Hehe, actually I also have a patch for this on my local system. :). The Symbian part was also one of the "catches" for me, and also the WinCE, Android (__android_log_assert is an assert, and should hence not be called by logMessage).
By the way, you can test compilation of other ports by logging to our buildbot, making a ScummVM checkout in your home directory, and using the custom build chains there. I did that with some of my recent changes. Works pretty well and is relatively easy.
Regarding the g_system problem. It is a rather small problem, since g_system is one of the very first things any backend's main() function should init. But this still leaves a small chance of debug/error being called from an OSystem subclass constructor, or anything called from that.
In the gsoc2010-opengl branch, we added an OSystem::init method which is to be invoked by the backend's main() right after constructing the OSystem object. OSystem subclass constructors then are meant to be "trivial", i.e. not contain any debug/error calls, and not construct other complex objects. This would effectively resolve this issue, I think.
So once that branch is merged, the g_system problem should be essentially resolved by adding an "assert(g_system);" at the start of error, warning & debug (well -- except that some backends implement assert() by calling our error() function... so maybe "exit" is better).
Regarding SamsungTV: That infinite loop stuck me as a very *very* problematic hack from the start. The porter (aquadran, I think) would be better of doing a #define exit(x) do {} while (true) to cover this, or use some other means to provide a "working" exit (working in the sense that it keeps the promise of not returning ;).
comment:4 by , 14 years ago
Yeah I forgot to mention that Android bit, guess we should ask our porter about that too. From what it looks the Android system might have some logging functionallity, so maybe that can be used directly too.
Ah nice, didn't know about the buildbot stuff, i.e. I didn't even know I have an account there :-P. Is there any docs on how to use that?
Well I still see a possible problem with the ::init call. Say the OSystem (or a superclass of a specific OSystem implementation) calls debug/warning/error where before the OSystem initializes its logging system. Of course that should be relatively local to backends and I guess with proper documentation that would be easy to handle too.
Guess we should ask Powel about the exit stuff, I actually wondered why he didn't do that yet anyway, but didn't get around to asking him.
comment:5 by , 14 years ago
You did not have an account on buildbot, but I just made one for you. Use this to login ssh -p2222 lordhoto@buildbot.scummvm.org I'll give you the PW via another channel.
BTW, I wouldn't wait too long for porters, best way to draw their attention is to just make the change, make sure it compiles, and then point it out to everybody on -devel (maybe specifically listing ports that should be double checked).
Re: g_system issue: Yes, there remains a small window during which calling g_system->logMessage() would be impossible or unsafe. But as you already point out, this is limited to a fairly small part of every port's main() function. I think it's acceptable if we document this limitation clearly, and notify everybody about it. Most ports should be able to setup the logging functionality very early on anyway. And before they have set it up, there is no real point doing any logging anyway, is there?
comment:6 by , 14 years ago
I really would like to see this go into SVN as soon as possible, so that we can implement file logging. This in turn is important for implementing the "report unknown MD5s to user" feature (as discussed on -devel).
So, I think we should be aggressive about this. Johannes, can you simply commit this, and then follow up with a post to -devel titled "PORTERS: Review OSystem::logMessage support" or so, in which you briefly sum up what was changed, and what porters should review.
If we wait for every affected porter to get in touch with us about this, we'll have to wait years...
comment:7 by , 14 years ago
Actually I didn't even came around to write Angus a mail about it yet nor did I find enough time to do test compilation on buildbot. I will try to find some time on friday and then commit it and notify our porters about the changes.
Any opinion on the include of backend headers for the default implementation? This still feels somewhat hacky, but I see no way around this except to duplicate the stdout/stderr based code on those backends which support it.
comment:8 by , 14 years ago
OK, then let's wait until you managed some tests.
I don't like the use of the backend headers that much, but I think we should leave it as is for now, and resolve this later. I actually would prefer on the long run if we did not provide a default implementation, even if that means that a few backends have to duplicate a few lines of code. A very small price to pay, IMO (and we already do that for getTimeAndDate). With the modularized backends code from GSoC, the impact of this would be even lower.
comment:9 by , 14 years ago
FYI, I tested the patch on buildbot and it compiled fine for the ds, dc, wince and ps2 ports. I think we can "risk" committing it.
comment:10 by , 14 years ago
I committed the patch now. It seems we don't have any toolchains for SamsungTV or Android on buildbot, with all the others I tried (PSP) it worked fine. I also fixed some regression in the WinCE code when the windows debugger is enabled, but I didn't try to compile that now.
comment:11 by , 14 years ago
| Owner: | changed from to |
|---|---|
| Status: | new → closed |
comment:12 by , 14 years ago
To be preicse I fixed some code problems in PSP and Android. After that PSP compiled fine :-).
Anyway I guess we should notify our porters about the change now so they can look at them and integrate them properly. Mostly Android, SamsungTV and Symbian comes to mind.
comment:13 by , 6 years ago
| Component: | → --Other-- |
|---|
Patch against r54114