Skip to content

Conversation

@zfergus
Copy link
Contributor

@zfergus zfergus commented Apr 9, 2025

The header files generated by bin2h.cmake cause an error when the GCC flag -Werror=missing-braces is enabled. I fixed this by adding another set of braces around ${arrayValues} on Line 98.

Edit: I pushed some more changes to my fork which automatically got included in this PR. My new changes make use of FMT in SPDLOG instead of the system-installed version.

Copy link
Member

@axsaucedo axsaucedo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the contribution, just getting around - would you be able to provide more context on the comments? + also re-commit with the DCO

@zfergus
Copy link
Contributor Author

zfergus commented May 28, 2025

Thank you for the contribution, just getting around - would you be able to provide more context on the comments? + also re-commit with the DCO

Thanks for the feedback. This PR got muddied because I needed to make changes to my fork to make the logger work in my project. I am happy to split it up into two PRs if necessary.

The changes are:

  1. Adding a second set of braces around arrayDefinition in bin2h.cmake to avoid errors when -Werror=missing-braces is enabled.
  2. Making spdlog and fmt options mutually exclusive. That is, spdlog comes with fmt built in, but they have slightly different include paths. This is more of a problem downstream than in Kompute, but I couldn't fix it without changing Kompute.

For (2), the problem is if I am using spdlog and kompute in my project, if Kompute uses fmt directly (rather than the one inside spdlog), I will get duplicate symbol errors. This error comes from the code including two different versions of fmt. Therefore, I changed Kompute to disable fmt if spdlog is enabled and instead use the fmt headers inside spdlog. This avoids the duplicate symbols by having the same headers, which have #pragma once at the top.


Edit: I updated the commits with the DCO.

@axsaucedo axsaucedo changed the title Update bin2h.cmake to avoid -Werror=missing-braces Make spdlog and fmt mutually exclusive and support -Werror=missing-braces May 29, 2025
Copy link
Member

@axsaucedo axsaucedo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok that makes sense, I have updated the title - we can move forward with single PR

Copy link
Member

@axsaucedo axsaucedo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems python tests are failing on build - once updated we can merge

zfergus and others added 3 commits May 29, 2025 16:10
Signed-off-by: zachferguson <[email protected]>
Signed-off-by: Zachary Ferguson <[email protected]>
Signed-off-by: zachferguson <[email protected]>
Signed-off-by: Zachary Ferguson <[email protected]>
@zfergus
Copy link
Contributor Author

zfergus commented May 29, 2025

Seems python tests are failing on build - once updated we can merge

I think I fixed this now. It seems that you use fmt for more than logging, so when the logger is disabled, you still need to link against fmt. I made it so disabling the logger also implies using fmt to fix this.

Copy link
Member

@axsaucedo axsaucedo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch - all clear now, thanks for covering these corner cases

@axsaucedo axsaucedo merged commit 342af81 into KomputeProject:master May 30, 2025
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants