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

GH-42232: [C++] Use non-stale c-ares download URL #42250

Merged
merged 3 commits into from
Jun 22, 2024

Conversation

jonkeane
Copy link
Member

@jonkeane jonkeane commented Jun 22, 2024

Rationale for this change

The URL to download the c-ares dependency is stale. Note: we weren't seeing this bonk on most CI because we have the fallback of our own copy of these. But the R script that downloads all of our thirdparty deps always uses the official URL which is the one that had moved.

Resolves #42232

What changes are included in this PR?

Use the appropriate URL, muck with . -> _

Are these changes tested?

The current CI should pass + the R maximal offline build should pass too

Are there any user-facing changes?

More reliable retrieval of all thirdparty dependencies

@jonkeane jonkeane requested a review from kou June 22, 2024 16:57
Copy link

⚠️ GitHub issue #42232 has been automatically assigned in GitHub to PR creator.

@jonkeane
Copy link
Member Author

@github-actions crossbow submit test-r-offline-maximal

Copy link

Revision: bc09a4a

Submitted crossbow builds: ursacomputing/crossbow @ actions-a6ca746f66

Task Status
test-r-offline-maximal GitHub Actions

@@ -144,7 +146,7 @@ DEPENDENCIES=(
"ARROW_BOOST_URL boost-${ARROW_BOOST_BUILD_VERSION}.tar.gz https://apache.jfrog.io/artifactory/arrow/thirdparty/7.0.0/boost_${ARROW_BOOST_BUILD_VERSION//./_}.tar.gz"
"ARROW_BROTLI_URL brotli-${ARROW_BROTLI_BUILD_VERSION}.tar.gz https://github.com/google/brotli/archive/${ARROW_BROTLI_BUILD_VERSION}.tar.gz"
"ARROW_BZIP2_URL bzip2-${ARROW_BZIP2_BUILD_VERSION}.tar.gz https://sourceware.org/pub/bzip2/bzip2-${ARROW_BZIP2_BUILD_VERSION}.tar.gz"
"ARROW_CARES_URL cares-${ARROW_CARES_BUILD_VERSION}.tar.gz https://c-ares.haxx.se/download/c-ares-${ARROW_CARES_BUILD_VERSION}.tar.gz"
"ARROW_CARES_URL cares-${ARROW_CARES_BUILD_VERSION}.tar.gz https://github.com/c-ares/c-ares/releases/download/cares-${ARROW_CARES_BUILD_VERSION_UNDERSCORES}/c-ares-${ARROW_CARES_BUILD_VERSION}.tar.gz"
Copy link
Member

Choose a reason for hiding this comment

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

How about using ${...//PATTERN/REPLACED} bash feature instead of defining ..._UNDERSCORES like ARROW_BOOST_BUILD_VERSION did?
(This data is used by only download_dependencies.sh.)

Suggested change
"ARROW_CARES_URL cares-${ARROW_CARES_BUILD_VERSION}.tar.gz https://github.com/c-ares/c-ares/releases/download/cares-${ARROW_CARES_BUILD_VERSION_UNDERSCORES}/c-ares-${ARROW_CARES_BUILD_VERSION}.tar.gz"
"ARROW_CARES_URL cares-${ARROW_CARES_BUILD_VERSION}.tar.gz https://github.com/c-ares/c-ares/releases/download/cares-${ARROW_CARES_BUILD_VERSION//./_}/c-ares-${ARROW_CARES_BUILD_VERSION}.tar.gz"

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, can do. I didn't see the boost version up there

@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Jun 22, 2024
@jonkeane
Copy link
Member Author

@github-actions crossbow submit test-r-offline-maximal

@github-actions github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Jun 22, 2024
Copy link

Revision: e363e1f

Submitted crossbow builds: ursacomputing/crossbow @ actions-495d6f0b18

Task Status
test-r-offline-maximal GitHub Actions

Copy link
Member

@kou kou left a comment

Choose a reason for hiding this comment

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

+1

@kou kou merged commit 3fe4df3 into apache:main Jun 22, 2024
29 of 35 checks passed
@kou kou removed the awaiting change review Awaiting change review label Jun 22, 2024
@github-actions github-actions bot added the awaiting merge Awaiting merge label Jun 22, 2024
@jonkeane jonkeane deleted the 42232_cares_stale branch June 23, 2024 00:04
@jonkeane
Copy link
Member Author

Thanks!

Copy link

After merging your PR, Conbench analyzed the 7 benchmarking runs that have been run so far on merge-commit 3fe4df3.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 9 possible false positives for unstable benchmarks that are known to sometimes produce them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[C++] c-ares download URL is stale?
2 participants