[SERVER-45445] Upgrade of libfmt to 7.1.3 Created: 09/Jan/20 Updated: 08/Jan/24 Resolved: 22/Apr/21 |
|
| Status: | Closed |
| Project: | Core Server |
| Component/s: | None |
| Affects Version/s: | None |
| Fix Version/s: | 5.0.0-rc0 |
| Type: | Task | Priority: | Major - P3 |
| Reporter: | Spencer Jackson | Assignee: | Mark Benvenuto |
| Resolution: | Fixed | Votes: | 0 |
| Labels: | None | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Issue Links: |
|
||||||||||||
| Backwards Compatibility: | Fully Compatible | ||||||||||||
| Sprint: | Dev Tools 2020-02-10, Dev Tools 2020-02-24, Security 2021-02-22, Security 2021-03-08, Security 2021-04-19, Security 2021-05-03 | ||||||||||||
| Participants: | |||||||||||||
| Linked BF Score: | 170 | ||||||||||||
| Description |
|
A new release has been made: https://github.com/fmtlib/fmt/releases/tag/7.0.1 |
| Comments |
| Comment by Billy Donahue [ 23/Apr/21 ] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Good emergency patch. It needs some follow-up action. It changes the source of the fmt library, so we need to track that change in https://github.com/mongodb-forks/fmt. From there we could make a more principled change and upstream it perhaps. If __cplusplus>=201703L then we don't need FMT_HAS_INCLUDE at all. Assume that any C++17 implementation has <string_view>. We could rearrange the if condition slightly to bypass the FMT_HAS_INCLUDE check for C++17 . | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Githook User [ 23/Apr/21 ] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Author: {'name': 'Mark Benvenuto', 'email': 'mark.benvenuto@mongodb.com', 'username': 'markbenvenuto'}Message: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Billy Donahue [ 21/Apr/21 ] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Woohoo! What a slugfest. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Githook User [ 21/Apr/21 ] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Author: {'name': 'Mark Benvenuto', 'email': 'mark.benvenuto@mongodb.com', 'username': 'markbenvenuto'}Message: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Billy Donahue [ 22/Mar/21 ] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
It might make sense to wait for libfmt-1.7.4 when these upstream issues will be fixed and integrated into a release. We'd still have to deal with the decimal point elision problem somehow. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Billy Donahue [ 20/Mar/21 ] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
I should write down the 4 fmtlib bugs I reported upstream while pursing this ticket. https://github.com/fmtlib/fmt/issues/2178 (fixed) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Billy Donahue [ 18/Mar/21 ] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Returning it to Security backlog. The C++ side is proven out, but the unforeseen problems at the next layer of the onion have now arisen. The problems now seems to be the fragile JSTests caring too much about round trip fidelity of decimal point placement even as an object goes from: JavaScript JSON => wire BSON => C++ BSONObj => LogV2 JSON => JavaScript JSON We can attack the jstest libraries and fix the logical problem. Or alternatively figuring out how to choose backward-compatible formats for logging floating point quantities. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Billy Donahue [ 18/Mar/21 ] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
The revert was two causes. Some compile failures. Could fix the compile failures with these tweaks: https://github.com/mongodb/mongo/compare/9154322...be337da More serious is default output format change for double. The meaning of format("{}"), 1.0) has changed. It used to produce "1.0" like python does. In 7.1.3, it produces "1" which is specified by C++20's std::format. If you want the old behavior you can use a hash mark for "alternate format", but that {:#} isn't even true. That forces a decimal point to appear but doesn't force digits to appear after it. Anyway we have jstests that are making a fragile STRING comparison of json objects obtained from the log, and basically that's what's failing. JavaScript serializes an object like the "command" of message 51803 (Slow query). And then it expects that exact substring to appear in the fetched log, which is rather messed up, because it's insisting that JavaScript and C++ use the same representation for JSON numerics, and we never had that guarantee. JSON has only one numeric type, and only the values really matter. So we could try to get into the JavaScript log matching library and make it use value comparisons instead of string comparisons. The difference is in apparent below in the formatting of command._id and command.batchSize, but not nShards. Lobster and jq hide these differences, btw. You have to look at raw output.
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Githook User [ 17/Mar/21 ] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Author: {'name': 'XueruiFa', 'email': 'xuerui.fa@mongodb.com', 'username': 'XueruiFa'}Message: Revert " This reverts commit dc68f885b80e70805eab8c5686ee0941bbdd806b. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Githook User [ 17/Mar/21 ] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Author: {'name': 'Billy Donahue', 'email': 'billy.donahue@mongodb.com', 'username': 'BillyDonahue'}Message: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Billy Donahue [ 17/Mar/21 ] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
The notes from the CR, now that it's finalized.
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Billy Donahue [ 14/Mar/21 ] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
I picked this back up and finished it. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Billy Donahue [ 08/Feb/21 ] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
sara.golemon sgolemon, I happened to do a fair amount of work on this ticket yesterday just for Sunday Shiggles. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Billy Donahue [ 13/Jul/20 ] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
There's a fmtlib v7.0.1 as of 2020-07-07. It will be a little more work to import it but looks very worthwhile. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Billy Donahue [ 21/Apr/20 ] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
While writing that up, I thought it might be easier to just try it than to write up my guesses about what it would be like to try it. It's all my own import scripts so there's no learning curve for me to do it. PQ: https://evergreen.mongodb.com/version/5e9f5a5b32f4170a47d26b36 I built the "core" scons metatarget and ran the logv2_test successfully. CR: https://mongodbcr.appspot.com/579040002/ Here's a deliberate attempt to fmt::format an unformattable type:
Which is is a big improvement. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Billy Donahue [ 21/Apr/20 ] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
For reference, we're currently on libfmt 6.1.1. They've moved on to 6.2.0 by now. There's some good stuff in there. No emergencies. 6.2.0 - Added experimental dynamic argument storage (#1170, #1584): This could be interesting for us. I think we're doing this using "fmt::internal" names and it could be an upgrade to do what we need to do in assembling format args without breaking the namespace barrier. Build failure diagnostics are improved. This could be pretty significant. If you've ever looked at a failed format call, it's very messy. Devs are confronted with these error messes when they write broken LOGV2 statements. The upgrade could help us out there. There are some subtle numeric rounding fixes as well. As a very recent import, upgrading fmt is easier than most of third_party has been. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Mira Carey [ 21/Apr/20 ] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
This is just a ticket to look at release notes and sum up if anything interesting came up |