Opened 6 years ago

Last modified 7 months ago

#3190 new task

[PATCH] Update cppformat

Reported by: leper Owned by: Itms
Priority: Should Have Milestone: Work In Progress
Component: Core engine Keywords: patch
Cc: Patch: phab:D2689

Description (last modified by s0600204)

Currently we use 0.11 while the latest upstream version is 2.1.1 (naming scheme changed, so just 7 versions outdated).

https://github.com/cppformat/cppformat/releases

Also see #3011 for some changes to the bundled version (that should be reapplied or dropped depending on the change (since some where done upstream in a similar/different way)). The remaining changes should be submitted upstream if applicable.

Attachments (2)

cppformat.patch (160.2 KB ) - added by fsincos 4 years ago.
Update to version 2.1.0.
cppformatmod.patch (2.1 KB ) - added by fsincos 4 years ago.
isnan conflict resolved (+- warnings on MSVC), workaround for another warning (d’après historic_bruno)

Download all attachments as: .zip

Change History (18)

comment:1 by Itms, 5 years ago

Description: modified (diff)
Milestone: BacklogAlpha 21

comment:2 by fsincos, 4 years ago

The attached patches do the following:

cppformat: Update to version 2.1.0 (straight from Github).

cppformatmod: There were some compiler errors related to the isnan function, so I commented it out. This seems to have fixed the errors. It was only a dummy function and shouldn't be used on any sane system anyway.

The patched game compiled flawlessly, passed all tests and "survived" a test game on my system (Linux 4.5.0, GCC 5.3.1). Someone else needs to test this on Windows, OS X, *BSD though.

There are some FMT_THROW calls in there (which I didn't run into) which could be changed to print a warning, similar to patch 0006 from #3011.

comment:3 by fsincos, 4 years ago

Description: modified (diff)
Keywords: review added
Summary: Update cppformat[PATCH] Update cppformat

by fsincos, 4 years ago

Attachment: cppformat.patch added

Update to version 2.1.0.

comment:4 by stanislas69, 4 years ago

Looks like you forgot to merge your last two patches, don't know if it's intended.

comment:5 by fsincos, 4 years ago

I wanted to keep the changes from the upstream version in a separate patch so it's easier to reapply / upstream whatever changes we make (especially for updating it in the future).

comment:6 by stanislas69, 4 years ago

Keywords: patch added

Ah yeah makes sense by the way you forgot the patch keyword =) I added it

comment:7 by historic_bruno, 4 years ago

  • I tested your patches with MSVC, and while the build succeeds, it's quite noisy:
    5>e:\devel\ps\source\third_party\cppformat\format.h(964): warning C4510: 'fmt::internal::DummyStream' : default constructor could not be generated
    5>          e:\devel\ps\source\third_party\cppformat\format.h(961) : see declaration of 'fmt::internal::DummyStream'
    5>e:\devel\ps\source\third_party\cppformat\format.h(964): warning C4610: struct 'fmt::internal::DummyStream' can never be instantiated - user defined constructor required
    
    Those appear frequently.
  • It looks like our posix.h defines a conflicting isnan, can you just add an undef before the problematic line? If that works, it's a little cleaner than commenting it out.
  • #3753 has a patch on cppformat to fix an MSVC error. The update doesn't solve the problem, but the patch still applies cleanly after yours.

by fsincos, 4 years ago

Attachment: cppformatmod.patch added

isnan conflict resolved (+- warnings on MSVC), workaround for another warning (d’après historic_bruno)

comment:8 by Itms, 4 years ago

Owner: set to Itms

I rebased and completed the patch above, and would like to commit the result. I tested it on Windows, it should be tested on Linux too.

https://github.com/na-Itms/0ad/tree/cppformat

Future work: #4148

comment:9 by Itms, 4 years ago

Keywords: review removed

Build fails currently on OSX. I'm taking a look at the issue on my virtual machine.

comment:10 by Itms, 4 years ago

Milestone: Alpha 21Alpha 22

comment:11 by elexis, 4 years ago

Milestone: Alpha 22Work In Progress

Moving to the Work In Progress milestone, since there is a patch asking for feedback, but since it is not strictly bound to a specific release.

comment:12 by leper, 3 years ago

In 19884:

Fix undefined macro usage in cppformat.

This is quite noisy with -Wexpansion-to-defined in both clang 3.9 and gcc 7.
(Refs #3190 and #4148. Upstream might have fixed this, but somewhat clean build logs seem worth it.)

Reviewed By: echotangoecho
Differential Revision: https://code.wildfiregames.com/D680

comment:13 by s0600204, 7 months ago

Description: modified (diff)

Without intending to step on any toes: I've got a git branch that builds on top of the changes of phab:D2613 and makes the jump straight to fmt 6.1.2 (which is the latest version at time of writing): https://github.com/s0600204/0ad/tree/fmt

Once D2613 is in, then if so desired I'll create a revision on Phabricator for consideration.

For linux: as fmt is available from linux distro repos, the build system now attempts to use that by default. This should work with any version of fmt from 4.0 upwards. However, for systems that don't have a recent enough version (see https://repology.org/project/fmt/versions), the minimum relevant source is bundled and can be compiled by passing --with-bundled-fmt to update-workspaces.sh.

For windows/osx: I don't have a system to test on, so some work there would be necessary. However, as said above, the relevant source is bundled, so it just be a case of tying into the respective build systems.

comment:14 by Itms, 7 months ago

Hi s0600204, amazing job! I'll do my best to commit D2613: right now your rebase of it needs a fix, then I'll test manually on whatever slave Jenkins keeps failing on.

comment:15 by Itms, 7 months ago

In 23562:

Update cppformat from v0.11.0 to v1.1.0, fixes #5646, refs #3190.
Use the opportunity to rename the lib from cppformat to fmt, refs #4148.

Patch By: adrian
Rebased By: s0600204
Differential Revision: https://code.wildfiregames.com/D2613

comment:16 by s0600204, 7 months ago

Patch: phab:D2689
Note: See TracTickets for help on using tickets.