Page MenuHomeWildfire Games

Updated cppformat
ClosedPublic

Authored by adrian on Feb 2 2020, 10:51 PM.
Tags
None
Subscribers
Restricted Owners Package, Restricted Owners Package, s0600204 and 2 others
Tokens
"Yellow Medal" token, awarded by asterix.

Details

Summary

I updated cppformat to version 1.1.0 (first stable version after bug fixed) and changed its name to fmt (like github repository). All previous modifications are preserved.

Test Plan

Just compile and run tests :)

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

adrian created this revision.Feb 2 2020, 10:51 PM
asterix added a subscriber: asterix.

Thank you much for this patch. I suspect Adrian that you will further update it until recent release (especially those optimizations)?

Stan added a subscriber: Stan.Feb 3 2020, 12:27 PM

Hey,

Can you reupload the patch so it can go through CI? It didn't go because you weren't added to the Contributors group.

Itms added a comment.Feb 3 2020, 2:25 PM

Thank you for the patch! We've been meaning to update cppformat (now fmtlib) for some time now. I had a half-finished patch for going to 2.1.1 (which was the last update before the renaming) in #3190, then was planning to update to the newest version in #4148.

However, your proposal to fix the bug in #5646 and use the opportunity to rename the lib also makes sense. It should then be easier to update to recent versions of the lib. Will you be interested in working on that after #5646 is fixed?

One issue we had with respect to upgrading cppformat is that it's not very clear which changes are from the upgrade, and which ones are from us for 0ad. Could you generate a diff between the upstream 1.1.0 version and the one you uploaded here? I think it should be possible and clean enough to create a source/third_party/fmt/patches/ directory in which we store patches for the changes we made. It would help with readability of our changes, and with sending fixes upstream if needed.

And thanks for the ping @asterix 👍

adrian added a comment.Feb 9 2020, 9:01 PM

Thanks a lot for the comments!

The problem with further updates is a lot of changes in fmt. Fmt version 6.x has completely different api in comparison to cppformat. Update to v1.1.0 wasn't very difficult task, but update to v6.x is much harder.
I propose to merge this changes into codebase (I can restore old cppformat name) and create another ticket to update fmt to version 6.x. Unfortunately I'm not sure how much time I will have in the future, so I think better option is to not "lock" this issue further.

@Stan How to upload once again? Do I need to create a new review or just edit this? This is my first time here :)

Itms added a comment.Feb 9 2020, 10:24 PM

I propose to merge this changes into codebase (I can restore old cppformat name) and create another ticket to update fmt to version 6.x. Unfortunately I'm not sure how much time I will have in the future, so I think better option is to not "lock" this issue further.

I agree, and actually I think it's very nice to use this small upgrade to rename the lib, so you can leave it like this.

However I'd still really like us to start versioning our changes to the lib, by adding a diff file containing the differences between upstream's 1.1.0 and this version, to our repo.

@Stan How to upload once again? Do I need to create a new review or just edit this? This is my first time here :)

Updating the revision with a new diff (for instance including the patch file I'm talking about) will have the effect wanted by Stan :)

adrian updated this revision to Diff 11321.Feb 10 2020, 6:51 PM
Itms set the repository for this revision to rP 0 A.D. Public Repository.Feb 10 2020, 7:04 PM

(you also needed to specify the repository in order to trigger a build, I fixed that)

adrian added a comment.EditedFeb 10 2020, 7:18 PM

Thanks!

Diff between 0ad and original version. Fortunately it's not so complicated.

1a2,6
>  * Slightly modified version of fmt, by Wildfire Games, for 0 A.D.
>  * Based on fmt v1.1.0 from https://github.com/fmtlib/fmt
>  */
> 
> /*
40d44
< #include <string>
43c47,49
< #if _SECURE_SCL
---
> #include "ps/CStr.h"
> 
> #if defined(_SECURE_SCL) && _SECURE_SCL
131a138,140
> # if defined(_MSC_VER)
> #  define FMT_USE_VARIADIC_TEMPLATES 1
> # else
134c143,144
<        (FMT_GCC_VERSION >= 404 && FMT_HAS_GXX_CXX11) || _MSC_VER >= 1800)
---
>        (FMT_GCC_VERSION >= 404 && __cplusplus >= 201103))
> # endif
141a152,153
> # elif defined(_MSC_VER)
> #  define FMT_USE_RVALUE_REFERENCES 1
145c157
<         (FMT_GCC_VERSION >= 403 && FMT_HAS_GXX_CXX11) || _MSC_VER >= 1600)
---
>         (FMT_GCC_VERSION >= 403 && __cplusplus >= 201103))
154c166
< #if FMT_USE_NOEXCEPT || FMT_HAS_FEATURE(cxx_noexcept) || \
---
> #if (defined(FMT_USE_NOEXCEPT) && FMT_USE_NOEXCEPT) || FMT_HAS_FEATURE(cxx_noexcept) || \
163c175
< #if FMT_USE_DELETED_FUNCTIONS || FMT_HAS_FEATURE(cxx_deleted_functions) || \
---
> #if (defined(FMT_USE_DELETED_FUNCTIONS) && FMT_USE_DELETED_FUNCTIONS) || FMT_HAS_FEATURE(cxx_deleted_functions) || \
290c302
< #if _SECURE_SCL
---
> #if defined(_SECURE_SCL) && _SECURE_SCL
486c498
< #if _SECURE_SCL
---
> #if defined(_SECURE_SCL) && _SECURE_SCL
871a884
>             FMT_MAKE_STR_VALUE(const CStr &, STRING)
1570c1583
< #if _SECURE_SCL
---
> #if defined(_SECURE_SCL) && _SECURE_SCL
2077c2090
< #if _MSC_VER
---
> #if defined(_MSC_VER) && _MSC_VER

@Itms what should be the next step? :)

Itms added a comment.Feb 12 2020, 9:29 PM

I will test this and hopefully commit it during the upcoming weekend. I am a bit short on free time these days, so unfortunately when things are related to libraries some of the steps include waiting for me. Don't hesitate to contribute to other areas of the code in the meantime. Or if you want you can start working on the future cppformat upgrade steps since they seem to involve a lot of work.

Angen resigned from this revision.Feb 13 2020, 9:46 PM

No problem. Thanks a lot!

Itms requested changes to this revision.Feb 17 2020, 8:05 PM

Hi! Things look good to me 👍

A few comments/nitpicks before I commit:

  • Can you add the diff to the commit, in unified style, as a file source/third_party/patches/fmt.diff (in the future, I'll post inline comments directly there...)
  • I don't know why we disable warnings outside the if _MSC_VER block, as those pragma instructions only work on MSVC. I think we can just add disabling of warning 4456 to the upstream version of format.cpp
  • Can you update the copyright year of test_fmt.h
  • We don't have to keep __cplusplus >= 201103 preprocessor conditionals, we can follow upstream and use FMT_HAS_GXX_CXX11
  • Can you harmonize the indentation for FMT_MAKE_STR_VALUE(const CStr &, STRING), so that it's the same as the previous lines (easier to spot in the unified diff with some lines of context)

If any comment I made is not clear, please still upload the patch containing the unified diff and I'll make inline comments on it 🙂

This revision now requires changes to proceed.Feb 17 2020, 8:05 PM
adrian updated this revision to Diff 11455.Mar 4 2020, 9:59 PM

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1836/display/redirect

adrian updated this revision to Diff 11456.Mar 4 2020, 10:01 PM

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/1317/display/redirect

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1837/display/redirect

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/1318/display/redirect

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/413/display/redirect

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/414/display/redirect

Hi @Itms,

Sorry for the delay. I've had a lot on my mind :)

I updated diff. Unfortunately I don't know what you mean with warning 4456. Thanks!

Itms added a comment.Mar 5 2020, 10:13 AM

Thank you! 👍

I clarified my comment about the warning, and I will take a look when I have time myself 😉

Sorry about the build failures, arcanist is reaaally stupid when it comes to handling renamed files.

source/third_party/patches/fmt.diff
155 ↗(On Diff #11456)

I was talking about this, we delete this part and add it back below. We could just move the line

# pragma warning(disable: 4456) // hides previous local declaration

to here and have only an addition, instead of 5 deletions & 4 additions. Also, we currently remove #if _MSC_VER without any actual reason.

s0600204 updated this revision to Diff 11546.Mar 25 2020, 4:09 PM
s0600204 added a subscriber: s0600204.

Re-upload existing patch, with moved files marked as such

Hopefully, Phabricator should now detect the moved files and patch correctly

Owners added subscribers: Restricted Owners Package, Restricted Owners Package.Mar 25 2020, 4:09 PM

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/477/display/redirect

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1905/display/redirect

s0600204 updated this revision to Diff 11554.Mar 26 2020, 1:31 AM

Re-add a couple of files inadvertently missed from earlier update

(Jenkins will still fail to successfully build. But at least it passes the file-patching stage.)

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/482/display/redirect

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1910/display/redirect

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/1386/display/redirect

asterix updated the Trac tickets for this revision.Mar 27 2020, 10:50 AM

Do to recent comment https://trac.wildfiregames.com/ticket/3190#comment:13 I suggest @Itms after this is commited to upgrade it straight to recent versions as done https://github.com/s0600204/0ad/tree/fmt .

Itms requested changes to this revision.Mar 28 2020, 6:52 PM

Thanks for the rebase @s0600204! Jenkins is indeed broken when source files are deleted (or moved) but there is still a build error on a clean build due to the test file.

source/ps/tests/test_fmt.h
20 ↗(On Diff #11554)

This line must be updated 🙂

This revision now requires changes to proceed.Mar 28 2020, 6:52 PM
s0600204 updated this revision to Diff 11607.Mar 29 2020, 5:33 PM

Re-rebase adrian's patch

And manually check I've included everything from it this time...

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/520/display/redirect

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1948/display/redirect

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/ps/CLogger.h
|   1| /*·Copyright·(C)·2019·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2020" year instead of "2019"

source/ps/CLogger.h
|  46| class·CLogger
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCLogger{' is invalid C code. Use --std or --language to configure the language.

source/ps/tests/test_fmt.h
|   1| /*·Copyright·(C)·2020·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2020"

source/ps/tests/test_fmt.h
|  22| class·TestFmt·:·public·CxxTest::TestSuite
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classTestFmt:' is invalid C code. Use --std or --language to configure the language.
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1953/display/redirect

Itms accepted this revision.Mar 31 2020, 10:24 AM

I will commit this today. Thanks adrian for the patch and s0600204 for the help!

This revision is now accepted and ready to land.Mar 31 2020, 10:24 AM
This revision was automatically updated to reflect the committed changes.