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 fmtlib for Output logger #2181

Merged
merged 21 commits into from
May 10, 2020
Merged

Use fmtlib for Output logger #2181

merged 21 commits into from
May 10, 2020

Conversation

fmatthew5876
Copy link
Contributor

Fix: #2098

In order to avoid rebase hell, I will add commits to change all of our log statements only after this PR gets approved.

Looks like linux packaging for this library is not great. In Ubuntu eoan for example it only has ancient version 5.1.2 which doesn't have pkg-config and breaks on trying to print char32_t.

Focal has 6.1.2 which should be fine.

vcpkg is up to date with latest 6.2.0

src/output.h Outdated Show resolved Hide resolved
@Ghabry
Copy link
Member

Ghabry commented Apr 29, 2020

So the minimal supported version should be 5.3.

Is for Debian in "Debian Stable Backports".
Fedora starting with Fedora 31.
OpenBSD Ports has 2.0 ??? o.O
Ubuntu starting from Ubuntu 20.04.
OpenSuse Leap 15.0

Other part are the console ports. I wonder how much frustration compiling this with newlib will give >.>

@fmatthew5876 fmatthew5876 force-pushed the fmtlib branch 2 times, most recently from 4cc2cbd to b14467d Compare April 29, 2020 21:34
@Ghabry
Copy link
Member

Ghabry commented Apr 29, 2020

Library building looks good. Havn't tested Player linking yet.

You also have to add "-lfmt" to all our Makefiles. (fortunately this will solve when the CMake migration is done...)

@fmatthew5876
Copy link
Contributor Author

I tested and this compiles and links successfully with

  • Ubuntu eoan cmake
  • Ubuntu focal autotools
  • Windows vcpkg

@fmatthew5876 fmatthew5876 force-pushed the fmtlib branch 2 times, most recently from cd43630 to f64161d Compare April 30, 2020 02:19
@carstene1ns carstene1ns added this to the 0.6.3 milestone Apr 30, 2020
@Ghabry
Copy link
Member

Ghabry commented Apr 30, 2020

Build fails with

tests/output.cpp:13:10: error: 'Post' is not a member of 'Output'
   13 |  Output::Post("Test %s", "post");

@fmatthew5876
Copy link
Contributor Author

Thanks for catching that, fixed the test.

@Ghabry
Copy link
Member

Ghabry commented Apr 30, 2020

Could youdo a full text search for Output::Post ? Also see a failure in the vita build.

@fmatthew5876
Copy link
Contributor Author

Hmm I did do a text search, but must have missed that one. Anyway, updated it.

builds/switch/Makefile Outdated Show resolved Hide resolved
Copy link
Member

@Ghabry Ghabry left a comment

Choose a reason for hiding this comment

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

Everything compiled succesfully.
Needs toolchain recompile before merge.

@fmatthew5876
Copy link
Contributor Author

fmatthew5876 commented Apr 30, 2020

Ok I've added commits to convert all log messages over. I also cleaned up and tested Window_NumberInput.

I did this by grepping for all files with Output::. Then opening them all in my editor and using search and replace in the edtior for the basic cases and manually fixing the complicated ones.

I didn't want to do something fully systematic like sed here because we have some snprintf usage. strftime() and other things like removing .c_str() which would take longer to get right in a script and probably still end up with mistakes.

This is tricky because any mistake here will unfortunately show up as a runtime exception (or just an assert() if exceptions are disabled on the platform). So let's review the diffs carefully.

@fmatthew5876
Copy link
Contributor Author

Self reviewed this, fixed a few minor things. From my perspective this is ready to go.

@Ghabry
Copy link
Member

Ghabry commented Apr 30, 2020

Does fmtlib support %s and %d in case we miss something?

@fmatthew5876
Copy link
Contributor Author

fmatthew5876 commented Apr 30, 2020

Does fmtlib support %s and %d in case we miss something?

It'll just print the %s and %d literally and ignore the arguments.

I did a ag Output:: | grep % and it returns me nothing. Also grepped for %. It's only comments, mods, strftime, and printfs. Pretty sure we got them all

@Ghabry
Copy link
Member

Ghabry commented May 2, 2020

Jenkins: Test this please <3

@carstene1ns carstene1ns added the Awaiting Rebase Pull requests with conflicting files due to former merge label May 3, 2020
@fmatthew5876 fmatthew5876 removed the Awaiting Rebase Pull requests with conflicting files due to former merge label May 3, 2020
Copy link
Member

@Ghabry Ghabry left a comment

Choose a reason for hiding this comment

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

(Note that many new commits - At least 10 - were added since the last review)

@fmatthew5876
Copy link
Contributor Author

Addressed review comments

@carstene1ns carstene1ns added the Awaiting Rebase Pull requests with conflicting files due to former merge label May 5, 2020
@fmatthew5876 fmatthew5876 removed the Awaiting Rebase Pull requests with conflicting files due to former merge label May 5, 2020
@fmatthew5876
Copy link
Contributor Author

fmatthew5876 commented May 5, 2020

Rebased, and fixed a few in input from the rebase.

Did another set of greps to verify no stray % leaked in.

@fmatthew5876 fmatthew5876 requested a review from carstene1ns May 7, 2020 02:46
Copy link
Member

@carstene1ns carstene1ns left a comment

Choose a reason for hiding this comment

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

I hope this works correctly with our newlib plaforms.

README.md Outdated Show resolved Hide resolved
builds/amiga/Makefile Show resolved Hide resolved
src/game_party.cpp Show resolved Hide resolved
@carstene1ns carstene1ns merged commit 34428bd into EasyRPG:master May 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Switch to using fmtlib for our logging
4 participants