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.

Sign up for GitHub

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

Jump to bottom

Use CURL::libcurl instead of variables #3030

Merged
merged 9 commits into from
Jun 24, 2021
Merged

Use CURL::libcurl instead of variables #3030

merged 9 commits into from
Jun 24, 2021

Conversation

sloretz
Copy link
Contributor

@sloretz sloretz commented Jun 21, 2021

Maybe this will fix the build failures in this job 🤷
https://build.osrfoundation.org/job/gazebo-ci-pr_any-windows7-amd64/ws/ws/

The trouble is gazebo_common is linking against libcurl.dll instead of libcurl_imp.lib. I have no idea why that happens. The exported CMake targets on Windows seem reasonable to me, so maybe using them will solve the issue.

@sloretz
Use CURL::libcurl instead of variables
bbf0e3d 
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
@sloretz sloretz self-assigned this Jun 21, 2021
@scpeters
Copy link
Member

looks like it works on windows but not Ubuntu bionic

CMake Error at cmake/GazeboUtils.cmake:99 (add_library):
  Target "libgazebo" links to target "CURL::libcurl" but the target was not
  found.  Perhaps a find_package() call is missing for an IMPORTED target, or
  an ALIAS target is missing?
Call Stack (most recent call first):
  gazebo/CMakeLists.txt:108 (gz_add_library)

sloretz added 2 commits June 21, 2021 16:47
@sloretz
Make an interface target for CURL on bionic
2d35a06 
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
@sloretz
Try an IMPORTED target instead of an INTERFACE one
73e7865 
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
traversaro
traversaro reviewed Jun 22, 2021
View reviewed changes
gazebo/common/CMakeLists.txt Outdated
@@ -237,7 +229,7 @@ target_compile_definitions(gazebo_common
target_link_libraries(gazebo_common
PUBLIC
${Boost_LIBRARIES}
${CURL_LIBRARIES}
CURL::libcurl
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not something introduced in this PR, but it seems that CURL is not used in public headers, so probably CURL::libcurl can be linked as a PRIVATE linked library, instead of a PUBLIC one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made PRIVATE in 9057ef9

sloretz added 4 commits June 22, 2021 09:19
@sloretz
Debug print and SHARED target
ce586af 
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
@sloretz
set IMPORTED_LOCATION
2e7b694 
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
@sloretz
CURL is a PRIVATE dep
9057ef9 
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
@sloretz
Remove debug print statement
3ec4db6 
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
scpeters
scpeters reviewed Jun 23, 2021
View reviewed changes
Copy link
Member

@scpeters scpeters left a comment

Choose a reason for hiding this comment

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

I think most of the plugins don't need to link to curl; I only see 3 that should need it:

grep -rnI include\ .curl/ plugins
plugins/rest_web/RestUiLoginDialog.cc:19:#include <curl/curl.h>
plugins/rest_web/RestApi.cc:20:#include <curl/curl.h>
plugins/StaticMapPlugin.cc:18:#include <curl/curl.h>

In fact, I think we could remove the curl include from RestUiLoginDialog.cpp, since it doesn't actually use it for anything. I'll try testing some changes and suggest a patch

tools/CMakeLists.txt Outdated
@@ -53,6 +45,7 @@ if (WIN32)
endif()

target_link_libraries(gz
CURL::libcurl
Copy link
Member

Choose a reason for hiding this comment

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

I don't see any usage of curl by gz; I would remove this line

I'm guessing the cmake code you've removed in this file is vestigial

Copy link
Member

Choose a reason for hiding this comment

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

dba62f3

scpeters added 2 commits June 23, 2021 16:20
@scpeters
Remove unneeded CURL include, linkage
dba62f3 
Signed-off-by: Steve Peters <scpeters@openrobotics.org>
@scpeters
Link to CURL for two plugins only
c47c2b0 
Only StaticMapPlugin and RestWebPlugin use CURL, so
only add linking for these.

Signed-off-by: Steve Peters <scpeters@openrobotics.org>
@scpeters
Copy link
Member

I just pushed my changes directly; hope you don't mind

@sloretz
Copy link
Contributor Author

sloretz commented Jun 24, 2021

All CI jobs appear to have built 🎉 . I don't see any new CMake warnings. As for test failures, there are lots, but that doesn't seem unusual looking at the past jobs. Anything left to do before this can be merged?

scpeters
scpeters approved these changes Jun 24, 2021
View reviewed changes
Copy link
Member

@scpeters scpeters left a comment

Choose a reason for hiding this comment

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

looks good to me

@scpeters scpeters merged commit 7a43a8d into gazebosim:gazebo11 Jun 24, 2021
@sloretz sloretz deleted the windows_curl_issue branch June 24, 2021 18:31
j-rivero added a commit that referenced this pull request Jul 8, 2021
@j-rivero
Backport PR #3030 to Gazebo9 for CURL fixes
2270bb4
@j-rivero j-rivero mentioned this pull request Jul 8, 2021
Merged
scpeters added a commit that referenced this pull request Aug 19, 2021
@j-rivero @scpeters
Backport PR #3030 to Gazebo9 for CURL fixes (#3041)
16b99c6 
Signed-off-by: Jose Luis Rivero <jrivero@osrfoundation.org>

Co-authored-by: Steve Peters <scpeters@openrobotics.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@traversaro traversaro traversaro left review comments

@scpeters scpeters scpeters approved these changes

Assignees

@sloretz sloretz

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sloretz @scpeters @traversaro