drop the dependencies on Seastar's operator<<(.., const std::vector<>&)

[tchaikov]

in Seastar, we have, for instance:

template <typename T>
inline
std::ostream& operator<<(std::ostream& os, const std::vector<T>& v) {
    bool first = true;
    os << "{";
    for (auto&& elem : v) {
        if (!first) {
            os << ", ";
        } else {
            first = false;
        }
        os << elem;
    }
    os << "}";
    return os;
}

but, quote from the issue reported by Benny at https://github.com/scylladb/seastar/issues/1544

This is out of scope for the seastar library.

In particular, the implementation for operator<<(std::ostream&, const std::unordered_map) is unusual and conflicts with similar helpers in scylla utils/to_string.hh

quote from Kefu's comment on the Seastar issue:

Seastar is not a formatting library, neither should it help user to print standard containers or even range-like containers or string-like objects -- this is covered by fmtlib already.

currently, we have 6 ways to print a range

1. fmt::join() to print a range with customized delimiter
2. fmt::format() to print a range. this is also supported by fmt/range.h
3. operator<<(ostream&, ...) overloads provided by scylladb/utils/to_string.hh
4. to_string() overloads provided by scylladb/utils/to_string.hh
5. operator<<(ostream&, ...) overloads provided by seastar/core/sstring.hh
6. to_string() overloads provided by seastar/core/sstring.hh

i think we should consolidate them, and only use fmtlib for printing out various objects and containers of them. but we should at least ready Scylla for this change. to make this happen, there is a strong prerequisite: to make fmt::is_formatable<Printable>::value true, where Printable is any type which want to print. fmt/ranges.h uses this predicate to tell if an element can be printed using its builtin range-like formatter. currently, despite that seastar::sstring can be printed using fmt::format() either because of FMT_DEPRECATED_OSTREAM or because of the fmt::formatter<seastar::sstring> partial specialization, we cannot print std::pair<int, sstring> or std::unordered_map<int, sstring> using Seastar's builtin support for range-like container if we provide a proper specialization for seastar::sstring without using fmt::ostream_formatter. because !detail::has_fallback_formatter<T, Char>::value> is false, so is fmt::is_formatable<sstring>::value. see https://github.com/fmtlib/fmt/blob/master/include/fmt/core.h#L1505-L1509. the reason why has_fallback_formatter<> is true is that we have FMT_DEPRECATED_OSTREAM defined in scylla.

we need to use a bottom-up approach to achieve this goal:

1. replace all os << foo << " and " << bar with fmt::print(os, "{} and {}", foo, bar)
2. use fmt::join() for printing containers. https://github.com/scylladb/scylladb/pull/13163 is a step in this direction.
3. use specialization of fmt::formatter<Printable> in the place of ostream<<(ostream&, const Printable&)
4. if we have to keep ostream<<(..), we can implement it using the former.
5. repeat step 1,2,3 and 4 until we don't depend on FMT_DEPRECATED_OSTREAM anymore
6. drop FMT_DEPRECATED_OSTREAM from configure.py
7. use the builtin range-like formatting in the place of fmt::join() where the delimiter and the open/close mark used by fmtlib's builtin implementation fulfills our needs
8. remove the helpers in seastar/core/sstring.hh and scylladb/utils/to_string.hh

as scylla is a large scale project, we might want to address this in multiple iterations.

as put above, we are not likely to fix all of the operator<< formatter in a single PR. i am proposing a general steps to create a PR for addressing this issue here:

1. remove FMT_DEPRECATED_OSTREAM line in configure.py, so we don't rely on the fallback ostream formatter created by fmtlib
2. recompile the scylla tree
3. find the errors like

   /usr/include/fmt/core.h:1756:3: error: static assertion failed due to requirement 'formattable': Cannot format an argument. To make type T formattable provide a formatter<T> specialization: https://fmt.dev/latest/api.html#udt
    static_assert(
    ^
   /usr/include/fmt/core.h:1777:10: note: in instantiation of function template specialization 'fmt::detail::make_value<fmt::basic_format_context<fmt::appender, char>, utils::tagged_uuid<tasks::task_id_tag> &>' requested here
    return make_value<Context>(val);
           ^

4. identify the type which we need to add a fmt::formatter specialization for. in following steps, the type is named Printable. in this case, Printable is utils::tagged_uuid<tasks::task_id_tag>
5. specialize fmt::formatter for utils::tagged_uuid<tasks::task_id_tag> or better off specializing for utils::tagged_uuid<> either using the fmt::ostream_formatter , or define a proper formatter using fmtlib.
6. optionally update some callers using the operator<<() of this type. if the operator<<() only has a single caller, we can also use fmt::streamed. see https://fmt.dev/dev/api.html#_CPPv4I0EN3fmt8streamedEN6detail13streamed_viewI1TEERK1T . but still, in the long run, we should have a fmt::formatter<> for this type. despite that this step is optional, it might be important, as it helps us to reduce the number of callers relying on operator<<().
7. change the ostream& operator<<(ostream& os, const Printable&) to use the new formatter, if the formatters are complicated. so we have less repeatings. like, fmt::print(os, printable).
8. and recompile the tree to see if the fix eliminates the compiling error. it's fine if the tree still does not compile. as we have lots of places using the formatter generated using FMT_DEPRECATED_OSTREAM, but a fix should at least address a single error.
9. add FMT_DEPRECATED_OSTREAM back
10. compile the tree, to ensure the change does not break the build.

see also https://gist.github.com/tchaikov/9f4cb52487189f51aa87ca8fd56dfd4f for some recipes on how to fix the callers of existing operator<<.

[tchaikov]

the date of f38's EOL will be Tue 2024-05-14 ^1 which is only 3 months ahead. we should be better prepared for switching to a supported distro for our base image, otherwise we would have to maintain an unsupported fedora linux release. the successor of f38 is f39, to run scylla on f39 would need to overcome some problems, one of them is this issue. in addition to it, we also have #16676 .

the take away is, we need to speed up the change. the patch adapted from the work at #15599 for building scylla on f39 on my local branch is like

146 files changed, 582 insertions(+), 191 deletions(-)

in which, most of the fmt::formatter<> specializations are implemented using fmt::ostream_formatter.

[denesb]

@tchaikov if you need to speed things up and you have these patches ready, you can start sending PRs which bundle some number of them (say a dozen or so), titled Convert operator<< to fmt::formatter 1/N, then 2/N, .... It is fine to bundle these very similar patches into PRs. If you can find a common theme, like all patches touching the same subsystem, that is fine, but it is not required. Just don't put too many patches into a bundle, because all maintainers will run away if they see 100 patches in a PR :laughing:.

[tchaikov]

hi Botond, thank you for the suggestion! actually, only a very small part of the changes is ready in my local branch, i always pick some of types as my warm-up exercise on daily basis. but taking the load of reviewers and the work of test failures annotations into consideration, to send the patches in a small bundles is still a big win, as it would be a much more efficient.

i will go in this way.

[nyh]

the date of f38's EOL will be Tue 2024-05-14 1 which is only 3 months ahead. we should be better prepared for switching to a supported distro for our base image, otherwise we would have to maintain an unsupported fedora linux release. the successor of f38 is f39, to run scylla on f39 would need to overcome some problems, one of them is this issue. in addition to it, we also have #16676 .

I have made the mistake of upgrading to Fedora 39, and was sad - and frankly shocked - that Scylla no longer compiles on it without dbuild. I am shocked because Fedora 39 is already 4 months old - nobody was bothered by this except you???

I think we need to fix this quickly - and not drag it over weeks. Is it really 100 patches left??? :-(

[nyh]

I just created by first formatter patch, https://github.com/scylladb/scylladb/pull/17432. It's one of a bazillion operator<< that need to be translated into an ugly template. This is an incredibly wasteful and stupid chore. I can't believe the fmt guys actually went ahead and did this. The "operator<<" thing worked well for decades, why did they decide to axe it? And why replace it by a template that needs to sit in a header file? :-(

[mykaul]

I have made the mistake of upgrading to Fedora 39, and was sad - and frankly shocked - that Scylla no longer compiles on it without dbuild. I am shocked because Fedora 39 is already 4 months old - nobody was bothered by this except you???

I build using dbuild happily on F39, so wasn't bothered that much.

[avikivity]

@nyh there's a lazier but worse patch, see https://github.com/scylladb/scylladb/pull/15599/commits/4c6b0085e37d34d354a863cbcfbf8f81e99c0009. It requires one line per type.

[nyh]

I build using dbuild happily on F39, so wasn't bothered that much.

dbuild is a real pain in the @$@!, I have been avoiding it whenever I can and wan't planning to make a habit out of using it. It's makes things like running scripts, tests, etc., super complicated and annoying. It's amazing that Fedora 39 has been out in 4 months and nobody besides Kefu has been doing anything to fix this :-( I see I will need to work on this myself now to make the progress faster :-(

[nyh]

@nyh there's a lazier but worse patch, see 4c6b008. It requires one line per type.

In what sense is it worse? Why aren't we doing that, if it's possible (I thought they took out this possibility!)

[tchaikov]

guess it's just me who wanted to pretend to be a perfectionist before giving up. i will send a rebased change based on Avi's 4c6b008 if we are satisfied with fmt::ostream_formatter . but please note, it creates a new fmt::memory_buffer instance for every formatted variable, and copy this chunk of memory to the output context. we can do this in parallel though: on one hand to have the lazy change in master, and on the other hand go on with s/operator<</fmt::formatter/ odyssey.

[avikivity]

@nyh there's a lazier but worse patch, see 4c6b008. It requires one line per type.

In what sense is it worse? Why aren't we doing that, if it's possible (I thought they took out this possibility!)

It's worse in that it keeps the dependency on two formatting systems. It's better in that it's a lot less work.

What was removed in fmt 10 is the ability to automatically fall back to ostream.

[avikivity]

And yes it's much slower. But that's minor as non of this is on a fast path.

[nyh]

guess it's just me who wanted to pretend to be a perfectionist before giving up. i will send a rebased change based on Avi's 4c6b008 if we are satisfied with fmt::ostream_formatter . but please note, it creates a new fmt::memory_buffer instance for every formatted variable, and copy this chunk of memory to the output context. we can do this in parallel though: on one hand to have the lazy change in master, and on the other hand go on with s/operator<</fmt::formatter/ odyssey.

I agree. I'll be happy to commit something which makes the project build on modern distros, even if later we can make it even better. The sooner the better ;-)

[tchaikov]

17968 should close this issue. but i didn't add Fixes to the PR or any of the commits in it. because i was afraid there were fall outs caused by this change. but after almost a week, it is relatively quiet. so i am closing this issue.

please note, the tree does not compile with {fmt} v10. https://github.com/scylladb/seastar/pull/2203 was created to address this.