ARROW-8961: [C++] Add utf8proc library to toolchain #7452
Conversation
|
@xhochy I do not see utf8proc being built here: |
Was an upper- vs lowercase issue. Now it errors out. |
|
Would also be good to be sure we're not switching to unilib #7449 (comment) before merging this. |
It will be nearly the same PR. If this one works, we can replace all |
|
Appears that unilib is a no go. I'm not a fan of one-developer projects anyway |
|
@ursabot build |
cpp/cmake_modules/Findutf8proc.cmake
Outdated
There was a problem hiding this comment.
We don't know here whether resolved the static or shared version of utf8proc. But we don't actually use this information later on, so we don't need to write code to determine it.
cpp/thirdparty/versions.txt
Outdated
There was a problem hiding this comment.
Can you keep this file in alphabetical order?
|
@maartenbreddels This is passing now, feel free to rebase again. @pitrou @kou @kszucs Any suggestions for Crossbow jobs I should run here? |
|
@github-actions crossbow run -g cpp -g wheel |
|
@github-actions crossbow submit -g cpp -g wheel |
|
|
Revision: e98614e4a69ade342ec6bae3c2641117ecd1afdf Submitted crossbow builds: ursa-labs/crossbow @ actions-337
|
|
@kszucs Do we have some kind of race condition with github-actions/crossbow. The jobs were never run but the linked branch is actually from a different PR where I summoned the bot nearly at the same time. |
|
@github-actions crossbow submit -g cpp |
|
Revision: e98614e4a69ade342ec6bae3c2641117ecd1afdf Submitted crossbow builds: ursa-labs/crossbow @ actions-340
|
|
Valgrind failure looks totally unrelated: https://github.com/ursa-labs/crossbow/runs/788860352?check_suite_focus=true |
|
@github-actions crossbow submit -g wheel |
|
Revision: e98614e4a69ade342ec6bae3c2641117ecd1afdf Submitted crossbow builds: ursa-labs/crossbow @ actions-342
|
|
@github-actions crossbow submit test-debian-10-cpp |
|
Revision: f1609e6dfe0dee96f2794edc77749cf62cc07f21 Submitted crossbow builds: ursa-labs/crossbow @ actions-344
|
cpp/cmake_modules/Findutf8proc.cmake
Outdated
There was a problem hiding this comment.
Could you use utf8proc_ROOT because we use utf8proc for package name?
There was a problem hiding this comment.
This doesn't exist.
It seems that we don't need this backup URL. If github.com is down, both of them are failed.
There was a problem hiding this comment.
Could you use ARROW_UTF8PROC_URL like other packages?
(Could you also fix BZIP2_SOURCE_URL?)
There was a problem hiding this comment.
Could you use lib if we use lib in UTF8PROC_STATIC_LIB?
Or could you use ${CMAKE_INSTALL_LIBDIR} in UTF8PROC_STATIC_LIB?
|
@github-actions crossbow submit -g linux |
|
Revision: 7883277a36f5c5b274eff2f42de5a6c28146d7c7 Submitted crossbow builds: ursa-labs/crossbow @ actions-346
|
|
@github-actions crossbow submit -g linux conda |
|
Revision: 5ab75c4 Submitted crossbow builds: ursa-labs/crossbow @ actions-356
|
|
@github-actions crossbow submit -g conda-* |
|
@github-actions crossbow submit conda-* |
|
Revision: 5ab75c4 Submitted crossbow builds: ursa-labs/crossbow @ actions-357
|
|
@maartenbreddels Feel free to rebase again, this patch is ready for a merge. @wesm Should we merge this as-is or is delay it until @maartenbreddels branch is ready? |
|
@xhochy I'm fine with merging this -- it's a non-mandatory dependency right (sorry have not reviewed the patch yet and the PR description does not say)? |
|
TODO:
|
This is just a workaround. We should use target_link_libraries.
|
+1 |
|
Once the utf8_lower/utf8_upper patch lands I am going to make utf8proc not mandatory. See ARROW-9220. |
6 participants
No description provided.