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

[Bug Fix] Fix windows compilation #3330

Closed

Conversation

talregev
Copy link
Contributor

@talregev talregev commented May 26, 2023

Fix windows compilation.

@talregev talregev changed the title Fix windows compilation [Bug Fix] Fix windows compilation May 26, 2023
@talregev talregev force-pushed the TalR/fix_windows_compilation branch 6 times, most recently from 178e6b9 to f9e61d2 Compare May 26, 2023 19:14
@talregev talregev force-pushed the TalR/fix_windows_compilation branch from f9e61d2 to b06a5a6 Compare May 26, 2023 19:17
@scpeters
Copy link
Member

it looks like the windows build still has some issues

@talregev
Copy link
Contributor Author

talregev commented May 27, 2023

it looks like the windows build still has some issues

Yes. Gazebo using c++17 syntax and the 3rdparty is compile with c++11. This is abi issue.

@traversaro
Copy link
Collaborator

it looks like the windows build still has some issues

Yes. Gazebo using c++17 syntax and the 3rdparty is compile with c++11. This is abi issue.

Are you sure? The problem seems that (just on Windows) protobuf is now updated to version 23, that has several changes including the public dependency on abseil (see gazebosim/gz-msgs#346 for similar issue). I think changing https://github.com/gazebosim/gazebo-classic/blob/fbb3d12273b02a382085ebf1f1ba72c4da315112/cmake/gazebo-config.cmake.in#LL169C1-L171C56 to:

find_package(Protobuf CONFIG)
if(NOT Protobuf_FOUND)
  find_package(Protobuf REQUIRED)
endif()
if(Protobuf_VERSION VERSION_GREATER_EQUAL 4.0.0)
  list(APPEND @PKG_NAME@_LIBRARIES protobuf::libprotobuf protobuf::libprotoc)
else()
  list(APPEND @PKG_NAME@_INCLUDE_DIRS ${PROTOBUF_INCLUDE_DIRS})
  list(APPEND @PKG_NAME@_LIBRARIES ${PROTOBUF_LIBRARIES})
endif()

may work and reduce the risk of regression for older setups, but I need to do some tests to confirm this.

@talregev
Copy link
Contributor Author

It my analysis with google search and see similar issues.
I guess the new protobuf using c++17 syntax but the abseil is compile with c++11.

@traversaro
Copy link
Collaborator

I guess the new protobuf using c++17 syntax but the abseil is compile with c++11.

No, abseil is compiled with C++17, see conda-forge/conda-forge-pinning-feedstock#4075 (comment) . I guess that you see similar errors in Google searches as the abseil ABI depends on the C++ standard version used in compilation, and hence if there is a mismatch, some abseil symbol can be missing in the actual compiled library, resulting in a "error LNK2019: unresolved external symbol". However, this is not the only reason a linker may not find a symbol: in this case I think that the linker does not find the abseil symbol simply because nobody is linking the abseil shared libraries.

@traversaro
Copy link
Collaborator

traversaro commented May 27, 2023

Note that even if we fix compilation with new protobuf, the Windows build would still fail due to conda-forge/libignition-msgs1-feedstock#30 (that is the reason why there is a failure now only on Windows). So we need to wait also for conda-forge/libignition-msgs1-feedstock#83 to merged, or in the meanwhile we can pin manually libprotobuf to 3.21 .

@talregev
Copy link
Contributor Author

talregev commented May 27, 2023

Note that even if we fix compilation with new protobuf, the Windows build would still fail due to conda-forge/libignition-msgs1-feedstock#30 (that is the reason why there is a failure now only on Windows). So we need to wait also for conda-forge/libignition-msgs1-feedstock#83 to merged, or in the meanwhile we can pin manually libprotobuf to 3.21 .

Can you pin manually libprotobuf to 3.21?
I can close this PR.

I also send you a respond email regards your email you sending me about my other PRs.
Are you still wish to talk with me?

@traversaro
Copy link
Collaborator

Note that even if we fix compilation with new protobuf, the Windows build would still fail due to conda-forge/libignition-msgs1-feedstock#30 (that is the reason why there is a failure now only on Windows). So we need to wait also for conda-forge/libignition-msgs1-feedstock#83 to merged, or in the meanwhile we can pin manually libprotobuf to 3.21 .

Can you pin manually libprotobuf to 3.21?

I hope to fix conda-forge/libignition-msgs1-feedstock#83 soonish (1/2 days) and then fix Gazebo Classic compatibility with protobuf 4.23.2, so pinning should not be necessary.

I also send you a respond email regards your email you sending me about my other PRs.
Are you still wish to talk with me?

Sure! Unfortunatly there have been some unplanned activities that took my time, I will contact you!

@talregev
Copy link
Contributor Author

I hope to fix conda-forge/libignition-msgs1-feedstock#83 soonish (1/2 days) and then fix Gazebo Classic compatibility with protobuf 4.23.2, so pinning should not be necessary.

Thank you for your quick respond. I am closing this PR.

@talregev talregev closed this May 27, 2023
@talregev talregev deleted the TalR/fix_windows_compilation branch May 27, 2023 16:44
@talregev talregev restored the TalR/fix_windows_compilation branch June 3, 2023 06:40
@talregev talregev reopened this Jun 3, 2023
@talregev
Copy link
Contributor Author

talregev commented Jun 3, 2023

@traversaro Why you close this PR?
conda-forge/libignition-msgs1-feedstock#83

@traversaro
Copy link
Collaborator

@traversaro
Copy link
Collaborator

I hope to fix conda-forge/libignition-msgs1-feedstock#83 soonish (1/2 days) and then fix Gazebo Classic compatibility with protobuf 4.23.2, so pinning should not be necessary.

Thank you for your quick respond. I am closing this PR.

Unfortunatly this turned out to be more complicated then expected, as it also required several changes deep down in google's stack (protobuf&abseil):

Anyhow, we should be converging soonish.

@talregev talregev closed this Jun 17, 2023
@talregev talregev deleted the TalR/fix_windows_compilation branch June 17, 2023 14:00
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.

3 participants