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 updated curl on evergreen windows hosts #7409

Merged
merged 1 commit into from
Mar 2, 2024
Merged

Use updated curl on evergreen windows hosts #7409

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion evergreen/config.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ functions:
fi

if [ -n "${curl_base|}" ]; then
set_cmake_var curl_vars CURL_LIBRARY PATH "$(./evergreen/abspath.sh ${curl_base}/lib/curl.lib)"
set_cmake_var curl_vars CURL_LIBRARY PATH "$(./evergreen/abspath.sh ${curl_base}/lib/libcurl.dll.a)"
Copy link
Contributor

@ironage ironage Mar 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Good catch! It would be nice to future proof it a bit; maybe something like this:

Pseudocode:
CURL_LIB_PATH = "curl.lib"
if (file exists "libcurl.dll.a") {
    CURL_LIB_PATH = "libcurl.dll.a"
}

Then it will still be compatible if we ever set a custom curl_base from a non windows runner as well as being robust while the windows machines are updated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I asked about this on slack a bit. Curl hadn't been updated in 10 years (I was actually the last one to update it in my past life as a build engineer), and it was updated at my request. I think in the intervening 10 years curl itself changed how its official windows builds worked.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In other words, this is already a special case and I think trying to generalize it right now is a bit of an over-optimization. If we ever need to add a different curl path we can just introduce an if statement here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Gotcha, sounds good to me 👍

set_cmake_var curl_vars CURL_INCLUDE_DIR PATH "$(./evergreen/abspath.sh ${curl_base}/include)"
set_cmake_var baas_vars REALM_CURL_CACERTS PATH "$(./evergreen/abspath.sh "${curl_base}/bin/cacert.pem")"
fi
Expand Down Expand Up @@ -221,6 +221,10 @@ functions:
export DEVELOPER_DIR="${xcode_developer_dir}"
fi

if [[ -n "${curl_base}" ]]; then
export PATH="${curl_base}/bin":$PATH
fi

# NOTE: These two values will be ANDed together for matching tests
TEST_FLAGS=
if [[ -n "${test_label}" ]]; then
Expand Down
Loading