Skip to content

add missing ostream formatters#2696

Closed
kammoh wants to merge 2 commits intoThe-OpenROAD-Project:masterfrom
kammoh:add_fmt_formatters
Closed

add missing ostream formatters#2696
kammoh wants to merge 2 commits intoThe-OpenROAD-Project:masterfrom
kammoh:add_fmt_formatters

Conversation

@kammoh
Copy link

@kammoh kammoh commented Jan 3, 2023

Fixes compile on [at least] the following platforms:

  • Fedora 37 (spdlog:1.10.0, fmt:9.1.0)
  • macOS Ventura (homebrew spdlog:1.11.0 fmt:9.1.0)

https://fmt.dev/latest/api.html#format-api
https://fmt.dev/latest/api.html#ostream-api

@QuantamHD
Copy link
Collaborator

@vvbandeira
Copy link
Member

Looks like this change is not compatible with the current version we support (and use on our CI):

In file included from /OpenROAD/src/dst/src/LoadBalancer.cc:29:
/OpenROAD/src/dst/src/LoadBalancer.h:36:10: fatal error: fmt/ostream.h: No such file or directory
   36 | #include <fmt/ostream.h>
      |          ^~~~~~~~~~~~~~~
compilation terminated.

@kammoh After signing your commit and passing DCO, I would suggest adding some conditional for the proposed fix. Maybe an ifdef depending on the version of fmt as to not break all other installs.

Fixes compile on [at least] the following platforms:
- Fedora 37 (spdlog:1.10.0, fmt:9.1.0)
- macOS Ventura (homebrew spdlog:1.11.0 fmt:9.1.0)

https://fmt.dev/latest/api.html#format-api
https://fmt.dev/latest/api.html#ostream-api
@kammoh kammoh force-pushed the add_fmt_formatters branch from c7e91f0 to 0ce11ce Compare January 5, 2023 20:58
@maliberty
Copy link
Member

I am opposed to the changes in fmt 9 and don't want to accept these changes (see fmtlib/fmt#3088). This has been discussed in #2386. As I said there, we should use FMT_DEPRECATED_OSTREAM instead.

@maliberty
Copy link
Member

This change is not sufficient to handle all the missing formatting fmt9 would require (which would be very extensive).

@QuantamHD QuantamHD added the bug Something isn't working label Jan 9, 2023
@kammoh
Copy link
Author

kammoh commented Jan 10, 2023

I agree that when/if the ADL-based opt-in feature is available, it would be a cleaner approach. But right now this is the only way to be able compile OpenROAD on recent Linux and macOS setups.

@maliberty
Copy link
Member

I believe FMT_DEPRECATED_OSTREAM is a better solution. Is there a reason we can't use it?

@maliberty
Copy link
Member

I believe FMT_DEPRECATED_OSTREAM is a better solution. Is there a reason we can't use it?

any response?

@tobim tobim mentioned this pull request Aug 27, 2023
@vvbandeira vvbandeira marked this pull request as draft October 7, 2024 20:04
@maliberty
Copy link
Member

stale

@maliberty maliberty closed this Aug 31, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants