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

Remove libcurl_vendor #71

Closed
wants to merge 14 commits into from
Closed

Remove libcurl_vendor #71

wants to merge 14 commits into from

Conversation

sloretz
Copy link
Contributor

@sloretz sloretz commented Feb 3, 2022

This is a step towards ros2/ros2#1150 - I want to remove libcurl_vendor so I don't have to figure out how to change where it installs headers. I think this has been discussed in #62

Signed-off-by: Shane Loretz <[email protected]>
@sloretz sloretz self-assigned this Feb 3, 2022
@sloretz
Copy link
Contributor Author

sloretz commented Feb 3, 2022

CI (build: --packages-above-and-dependencies resource_retriever test: --packages-above resource_retriever)

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@clalancette
Copy link
Contributor

We tried this before, and failed: #64 . That said, if you can make it work I'd be happy to do this.

@sloretz
Copy link
Contributor Author

sloretz commented Feb 3, 2022

Another windows attempt - will probably fail due to .dll.a/.a issue found in #64, but I want to see if the location of curl.exe can be used as a hint to find_package: Build Status

Signed-off-by: Shane Loretz <[email protected]>
@sloretz
Copy link
Contributor Author

sloretz commented Feb 3, 2022

Windows test again Build Status

@sloretz
Copy link
Contributor Author

sloretz commented Feb 3, 2022

Again Build Status

Signed-off-by: Shane Loretz <[email protected]>
@sloretz
Copy link
Contributor Author

sloretz commented Feb 3, 2022

Another idea: Build Status

@sloretz
Copy link
Contributor Author

sloretz commented Feb 4, 2022

Not out of ideas yet: Build Status

Signed-off-by: Shane Loretz <[email protected]>
@sloretz
Copy link
Contributor Author

sloretz commented Feb 4, 2022

Again Build Status

Signed-off-by: Shane Loretz <[email protected]>
@sloretz
Copy link
Contributor Author

sloretz commented Feb 4, 2022

Again Build Status

Signed-off-by: Shane Loretz <[email protected]>
@sloretz
Copy link
Contributor Author

sloretz commented Feb 4, 2022

Again Build Status

Signed-off-by: Shane Loretz <[email protected]>
@sloretz
Copy link
Contributor Author

sloretz commented Feb 4, 2022

Still going: Build Status

Signed-off-by: Shane Loretz <[email protected]>
@sloretz
Copy link
Contributor Author

sloretz commented Feb 4, 2022

Adding paths to help find_program and find_library find the chocolatey install: Build Status

Signed-off-by: Shane Loretz <[email protected]>
@sloretz
Copy link
Contributor Author

sloretz commented Feb 4, 2022

Using CMAKE_PREFIX_PATH Build Status

@sloretz
Copy link
Contributor Author

sloretz commented Feb 4, 2022

Looking a little more promising Build Status

@sloretz
Copy link
Contributor Author

sloretz commented Feb 4, 2022

Ok, I know how to make removing libcurl_vendor work, but I don't think it's worth it.

This PR finds the chocolatey version of curl and builds against it by finding the chocolatey curl.exe. To do that it has to ignore the curl.exe shipped in Windows 10 in C:/Windows/System32. That's no problem, except it will find a curl.exe in C:/ProgramData/chocolatey/bin/curl.exe .

That executable is a "shim" created with a chocolatey tool called shimgen. The point of shimgen is to allow executables to be run without the directory containing the .dll's they rely on being on PATH. To link against curl, we want to know what the directory with the .dlls is. This PR gets it by running the found chocolatey curl.exe with the argument --shimgen-noop. That causes it to output text that says where the real executable is. That output includes this line:

path to executable: C:\ProgramData\chocolatey\lib\curl\tools\curl-7.81.0-win64-mingw\bin\curl.exe

This PR parses that output and adds the path which contains bin/curl.exe (C:\ProgramData\chocolatey\lib\curl\tools\curl-7.81.0-win64-mingw\) to CMAKE_PREFIX_PATH. That allows the builtin FindCURL.cmake to find and build against the chocolatey version of curl. The problem is, that still doesn't add the .dll files to PATH at runtime.

It would be possible to make an environment hook that also finds chocolately curl and uses the --shimgen-noop output to get a path to add to PATH when call install/setup.bat is run, but that seems like a lot of work for not a lot of gain. Additionally, the chocolatey attitude seems to want to make these libraries unavailable. I'm surprised it installs the header files at all given how it hides the libraries. Maybe the header files won't be in the package in the future.

Compare that with the chocolatey package we ship for log4cxx where we explicitly add it's libraries to PATH. I don't think the curl package shipped in chocolatey is ever going to expose that, so let's not build against it.

# Add lib directory to path for dlls.
Install-ChocolateyPath "$env:ChocolateyPackageFolder\lib" -PathType "Machine

@sloretz sloretz closed this Feb 4, 2022
@sloretz sloretz deleted the sloretz__remove_libcurl_vendor branch February 4, 2022 19:17
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