Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Core] Fix spdlog build issue #35287

Closed

Conversation

peytondmurray
Copy link
Contributor

@peytondmurray peytondmurray commented May 12, 2023

Why are these changes needed?

Currently an issue with spdlog is preventing Ray from compiling. This PR fixes that issue.

I'm not very familiar with spdlog, but as I understand it spdlog contains a bundled (vendored in) version of the fmt library which it uses to write output. I noticed the bazel build file for spdlog included fmt/*.h and fmt/bundled/*.h. I'm not sure what the difference between the two is here, but when building only using the bundled version of fmt, I'm able to compile successfully. I'd be grateful for someone more familiar with this part of the code base to comment (@vitsai ?)

Related issue number

Closes #35200.

Checks

  • I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
  • I've run scripts/format.sh to lint the changes in this PR.
  • I've included any doc changes needed for https://docs.ray.io/en/master/.
    • I've added any new APIs to the API Reference. For example, if I added a
      method in Tune, I've added it in doc/source/tune/api/ under the
      corresponding .rst file.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
  • Testing Strategy
    • Unit tests
    • Release tests
    • This PR is not tested :(

Signed-off-by: pdmurray <peynmurray@gmail.com>
@peytondmurray
Copy link
Contributor Author

@vitsai I'm able to build Ray just fine with this modification locally. Do you have any insight as to why spdlog fails to build on CI?

@xieus
Copy link

xieus commented May 24, 2023

@vitsai Could you review this PR as requested? Thanks.

@vitsai vitsai self-assigned this May 24, 2023
@vitsai
Copy link
Contributor

vitsai commented May 24, 2023

It looks like this change breaks the arm64 build. What platform are you seeing build errors on?

@peytondmurray
Copy link
Contributor Author

Indeed it does break the arm64 build, so this approach doesn't quite seem like the right solution. Here's my platform info:

$ uname -rms
Linux 6.3.3-arch1-1 x86_64

@vitsai vitsai closed this May 25, 2023
@vitsai
Copy link
Contributor

vitsai commented May 25, 2023

Closing this for now, we can continue discussion on the issue thread.

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.

[Core] Ray fails to build due to spdlog char8_t keyword issue
3 participants