When running several instances of 0 A.D. simultaneously, you need to have a logfile for each instance.
patch is copied and adapted from #3339
Details
Details
- Reviewers
elexis - Commits
- rP20141: Command line option for pid+timestamp in OOS dump, mainlog and interestinglog
- Trac Tickets
- #3339
Start 0ad with and without the command line option -unique-logs and check the name of the created logs
Diff Detail
Diff Detail
- Repository
- rP 0 A.D. Public Repository
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Event Timeline
There are a very large number of changes, so older changes are hidden. Show Older Changes
Comment Actions
Build is green
Updating workspaces. Build (release)... Build (debug)... Running debug tests... Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!
http://jw:8080/job/phabricator/114/ for more details.
Comment Actions
Build is green
Updating workspaces. Build (release)... Build (debug)... Running debug tests... Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!
http://jw:8080/job/phabricator/117/ for more details.
Comment Actions
Build is green
Updating workspaces. Build (release)... Build (debug)... Running debug tests... Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!
http://jw:8080/job/phabricator/119/ for more details.
Comment Actions
That file can easily reach 200mb in some multiplayer game, so I suggest to not add one with every match.
- Do we need the PID in the filename (for example to match a logfile to an actual process?) If it's solely useful for preventing overwrites, the approach from rP17776 could be reused (not really important since it's only for devs and those likely even care about the PID).
- Should we use a human readable date instead of the timestamp? Same answer as the PID question I guess.
- The original ticket indended to add this filename extension to avoid overwriting of simultaneous sim-logs on OOS.
It was closed as won't fix since it was kind of frowned upon to add developer debugging code, but reality shows that the debug code is generic enough to be used in many situations and that these situations actually do appear basically every release. For OOS dumps, we also have the rejointest test tool from rP18940 now, which covers most use cases. However if it's a bug that is caused by the GUI changing the simstate, like rP18804, then the rejointest tool won't work, while the original patch would. So I suggest adding it too in the same go. Would have to wait for D16 though.
| source/ps/CLogger.cpp | ||
|---|---|---|
| 78 ↗ | (On Diff #165) | Vladislav were happy if we use nullptr as that has the correct type (though it only actually comes into play with overrides iirc) |
Comment Actions
Build is green
Updating workspaces. Build (release)... Build (debug)... Running debug tests... Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!
http://jw:8080/job/phabricator/269/ for more details.
Comment Actions
Build is green
Updating workspaces. Build (release)... Build (debug)... Running debug tests... Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!
http://jw:8080/job/phabricator/270/ for more details.
Comment Actions
Nevermind, clicked on "show older changes" to see if all comments were addressed and it opened the old diff >.>
Comment Actions
- It were preferable if those filenames were guaranteed to have the same postfix (i.e. timestamp)
- Ordering a debug_printf for the mainlog.html filename, since printing one more line to the command line output is cheap and since the output can be useful (in case of having to lookup the log) and certainly useful in case of using the proposed option
- Removing the duplication would be nice, as both strings always end with the same postfix. Using a CStrW instead of a stringstream might work. Maybe ask leper before implementing my C++ ideas
Comment Actions
Build has FAILED
Updating workspaces.
Build (release)...
../../../source/ps/CLogger.cpp: In constructor ‘CLogger::CLogger()’:
../../../source/ps/CLogger.cpp:74:67: error: cannot pass objects of non-trivially-copyable type ‘std::string {aka class std::basic_string<char>}’ through ‘...’
debug_printf("Writing the mainlog at %s\n", mainlogPath.string8());
^
make[1]: *** [obj/engine_Release/CLogger.o] Error 1
make: *** [engine] Error 2
make: *** Waiting for unfinished jobs....Link to build: http://jw:8080/job/phabricator/435/
See console output for more information: http://jw:8080/job/phabricator/435/console
Comment Actions
Build is green
Updating workspaces. Build (release)... Build (debug)... Running release tests... Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK! Running debug tests... Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!
http://jw:8080/job/phabricator/436/ for more details.
| binaries/system/readme.txt | ||
|---|---|---|
| 65 ↗ | (On Diff #653) | becoming? |
| source/main.cpp | ||
| 85 ↗ | (On Diff #653) | |
| 89 ↗ | (On Diff #653) | g_LogPostfix to denote connotation? |
| 436 ↗ | (On Diff #653) | Not ideal to initialize a a variable of the CLogger in main.cpp. The var should be defined here if its used globally |
| source/network/NetClientTurnManager.cpp | ||
| 121 ↗ | (On Diff #653) | Since when do we use append? Can't we just create the OsPath directly without creating a string? |
| 138 ↗ | (On Diff #653) | Is the CStr actually needed? Didn't string8() already convert to that? |
| source/ps/CLogger.cpp | ||
| 37 ↗ | (On Diff #653) | Is it possible to make this a member variable of the logger and then query for the global logger? And use a const getter function instead of exposing write access? |
Comment Actions
Build is green
Updating workspaces. Build (release)... Build (debug)... Running release tests... Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK! Running debug tests... Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
http://jw:8080/job/phabricator/1010/ for more details.
Comment Actions
Build is green
Updating workspaces. Build (release)... Build (debug)... Running release tests... Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK! Running debug tests... Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
http://jw:8080/job/phabricator/1011/ for more details.
Comment Actions
Got some OOS and needed this to save both oos dump files. So let's get this over with.
| source/main.cpp | ||
|---|---|---|
| 436 ↗ | (On Diff #653) | With an empty string yes, but I meant the one assigning it the non-empty one below. If it were a member of CLogger, it could have been 'initialized' in Gamesetup.cpp where we have the ctor of CLogger and args too. But since this isn't really about the Gamesetup (TM), having it in main.cpp works for me. |
| source/network/NetClientTurnManager.cpp | ||
| 122 ↗ | (On Diff #1653) | 2 spaces |
| 123 ↗ | (On Diff #1653) | There are both string and wstring variants of OsString in os_path.h. Test it on windows if you want to be sure. It must compile because the previous code compiled. |
| 139 ↗ | (On Diff #1653) | ok to use string8 since it's about display |
| source/ps/CLogger.cpp | ||
| 77 ↗ | (On Diff #1653) | (Could be inlined, idc) |
| 37 ↗ | (On Diff #653) | Mmh, you convinced me. But following that argument, this CStrW should be defined in main.cpp` then. Also just CStrW Foo; instead of assigning this new empty string. |
| source/ps/CLogger.cpp | ||
|---|---|---|
| 37 ↗ | (On Diff #653) |
Don't think so: If --unique-logs is not set, g_UniqueLogPostfix will be uninitialized. |
Comment Actions
Build is green
Updating workspaces. Build (release)... Build (debug)... Running release tests... Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK! Running debug tests... Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK! Checking XML files...
http://jw:8080/job/phabricator/1410/ for more details.
Comment Actions
Executing section Default... Executing section Source... Executing section JS... Executing section XML GUI... Executing section Python... Executing section Perl...
http://jw:8080/job/phabricator_lint/83/ for more details.
Comment Actions
Accepting if you take any damage by that getpid hunk and that you apply the requests below.
Tested by adding that true || to the oh no part of the server turnmanager.
Don't really want to test this again, so if you apply the remarks you can commit it. Otherwise yell.
| binaries/system/readme.txt | ||
|---|---|---|
| 68 ↗ | (On Diff #2307) | these files |
| source/main.cpp | ||
| 476 ↗ | (On Diff #2307) | Merge those 5 lines into one by avoiding the stringstream and using C++11 std::to_wstring to convert the numbers? If g_UniqueLogPostfix is initialized here, why wouldn't it also be defined in this file? |
| 85 ↗ | (On Diff #653) | I'd lie if I'd claim to be able to review this hunk |
| source/network/NetClientTurnManager.cpp | ||
| 123 ↗ | (On Diff #1653) | (OsString was definitely needed for filestream constructors) |
| 139 ↗ | (On Diff #1653) | (string8 is actually needed if we believe our VisualReplay experience) |
| source/ps/CLogger.cpp | ||
| 77 ↗ | (On Diff #1653) | Actually why isn't that inlined? |
| 37 ↗ | (On Diff #653) | What value other than an empty string would a CStrW be if not getting a value assigned explicitly? Null pointer or uninitialized pointer in a non-pointer type? This prints out "ab": CStr abc; debug_printf("a%sb", abc.c_str()); |
| source/main.cpp | ||
|---|---|---|
| 476 ↗ | (On Diff #2307) | Yes. In that class. Not as a global in that file. So that we would have had g_OutGlobalLogger->GetPrefix which would have been more readable... |
Comment Actions
Build is green
Updating workspaces. Build (release)... Build (debug)... Running release tests... Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK! Running debug tests... Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK! Checking XML files...
http://jenkins-master:8080/job/phabricator/1989/ for more details.
Comment Actions
Executing section Default... Executing section Source... Executing section JS... Executing section XML GUI... Executing section Python... Executing section Perl...
http://jenkins-master:8080/job/phabricator_lint/504/ for more details.