Build warnings (-Wsign-conversion) under Clang #4
Comments
|
That does look like a problem. But for what its worth, we shouldn't be building at all on macOS. Instead you should be getting spdlog from brew. This PR added instructions for macOS. |
|
@clalancette I was building from ros2.repos so maybe spdlog_vendor should be removed from that file? |
No, the vendor packages are intentionally part of the workspace on all platforms. They choose between which version of the vendor package is used. Sometimes they even provide CMake modules to find a vendor package which doesn't provide it out-of-the-box. As @clalancette mentioned you need to follow the install instructions and install the dependency. Assuming that resolves the question I will go ahead and close this. |
|
@dirk-thomas @clalancette That doesn't resolve the issue, and I don't know why you think it would. Please reopen this ticket; there are code quality issues here that need investigation. These mistakes should be reviewed and either fixed or suppressed as appropriate. |
|
Please post the compiler warnings you are seeing after having installed the dependency as described in the instructions and I will reopen the ticket. |
|
@dirk-thomas I think we're on different pages. You're telling me not to build a package and then look at that package for build warnings in the source code that I did not build? Well anyway, here are the build warnings in Linux CI with Clang. They are exactly the same as I originally reported: https://ci.ros2.org/view/All/job/nightly_linux_clang_libcxx/354/clang/#issuesContent |
|
I totally agree that there are warnings here, and they should be looked at. In all likelihood, these will need to be fixed upstream in spdlog (actually upstream in fmt, which spdlog vendors). That being said, this is going to be low priority for us. The reason for that is that on our default platforms for Foxy (Ubuntu Focal, macOS Mojave, and Windows 10), we don't see these issues. Why? Because on Ubuntu and macOS, spdlog_vendor correctly finds the system package, so we don't build anything (and hence don't see the warnings). We do build on Windows 10, but since it isn't clang we don't see the warnings there either. The reason you saw the issues again when you ran the nightly_linux_clang job is that that job is still configured to use Ubuntu Bionic, which doesn't have the up-to-date spdlog system package. And thus it falls back to building again. So I am going to reopen, as it is a bug, but I'm not sure whether we'll have time to address it before Foxy. If you'd like to contribute a patch to spdlog/fmt to fix those, that would be great; we could then update our vendored spdlog appropriately. |
@rotu Please create a ticket upstream about this and post a reference here. After that I propose we close this ticket as a duplicate. |
|
I opened a ticket upstream: |
MacOS Catalina 10.15.4 Beta
Apple clang version 11.0.0 (clang-1100.0.33.17)
The text was updated successfully, but these errors were encountered: