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.
Details
- Reviewers
Itms Silier - Commits
- rP23562: Update cppformat from v0.11.0 to v1.1.0, fixes #5646, refs #3190.
- Trac Tickets
- #5646
#3190
Just compile and run tests :)
Diff Detail
- Repository
- rP 0 A.D. Public Repository
- Branch
- arcpatch-D2613
- Lint
Lint OK - Unit
No Unit Test Coverage - Build Status
Buildable 11326 Build 20462: Vulcan Build Jenkins Build 20461: Vulcan Build (macOS) Jenkins Build 20460: Vulcan Build (Windows) Jenkins Build 20459: arc lint + arc unit
Event Timeline
Thank you much for this patch. I suspect Adrian that you will further update it until recent release (especially those optimizations)?
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.
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 ?
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 :)
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 :)
(you also needed to specify the repository in order to trigger a build, I fixed that)
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
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.
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 ?
Build failure - The Moirai have given mortals hearts that can endure.
Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1836/display/redirect
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!
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 | ||
---|---|---|
156 | 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. |
Re-upload existing patch, with moved files marked as such
Hopefully, Phabricator should now detect the moved files and patch correctly
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
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
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 .
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