Skip to content

Conversation

@rip-nsk
Copy link
Contributor

@rip-nsk rip-nsk commented Jan 31, 2019

No description provided.

@wesm
Copy link
Member

wesm commented Feb 1, 2019

rebased

@rip-nsk rip-nsk changed the title ARROW-4439 [C++] Improve FindBrotli.cmake ARROW-4439: [C++] Improve FindBrotli.cmake Feb 2, 2019
@rip-nsk
Copy link
Contributor Author

rip-nsk commented Feb 2, 2019

WIP

@rip-nsk rip-nsk changed the title ARROW-4439: [C++] Improve FindBrotli.cmake [WIP] ARROW-4439: [C++] Improve FindBrotli.cmake Feb 3, 2019
@xhochy
Copy link
Member

xhochy commented Feb 8, 2019

Rebased.

@wesm
Copy link
Member

wesm commented Feb 9, 2019

I just fixed the Linux build. Could you write in the PR description what you are trying to accomplish with this patch?

@wesm
Copy link
Member

wesm commented Feb 10, 2019

Looks like this also broke library detection in the manylinux1 build. I don't have time to fix it right now

@rip-nsk
Copy link
Contributor Author

rip-nsk commented Feb 10, 2019

I just fixed the Linux build. Could you write in the PR description what you are trying to accomplish with this patch?

The goal of this PR find libraries with respect to BROTLI_STATIC_LIB_SUFFIX/BROTLI_MSVC_STATIC_LIB_SUFFIX (BTW the can be "merged") for case when only static library is available

Difference from previous logic: BROTLI_STATIC_LIB_SUFFIX should be explicitly specified, there are no auto select between "-static" and "" anymore.

@rip-nsk
Copy link
Contributor Author

rip-nsk commented Feb 11, 2019

I don't know how brotli libs were found in /usr/lib64 in manylinux1 build before this PR.
My ubuntu installs brotli (using manylinux1/scripts/build_brotli.sh) to /usr/lib/x86_64-linux-gnu/libbrotlienc.a where it is successfully found.
lib64 is not present in PATH_SUFFIXES, FIND_LIBRARY_USE_LIB64_PATHS is not specified..
Is it possible that lib64/libbrotlienc.a is cached in image and just working unless something is invalidates make's cache?
update: I almost sure that it cmake cache issue:
flatbuffers (installed using scripts/build_flatbuffers.sh) can't be found because FindFlatbuffers.cmake is looking for flatbuffers in lib/lib64, but actual path is lib/${CMAKE_LIBRARY_ARCHITECTURE}/libflatbuffers.a

@rip-nsk rip-nsk changed the title [WIP] ARROW-4439: [C++] Improve FindBrotli.cmake ARROW-4439: [C++] Improve FindBrotli.cmake Feb 12, 2019
@rip-nsk
Copy link
Contributor Author

rip-nsk commented Mar 12, 2019

@wesm, @xhochy
Any reasons blocks this PR?

@wesm
Copy link
Member

wesm commented Mar 12, 2019

The big CMake refactor patch is blocking this. Can you rebase after that goes in?

@wesm
Copy link
Member

wesm commented Mar 12, 2019

Can you also write a PR description explaining what issue is being fixed?

@xhochy
Copy link
Member

xhochy commented Mar 12, 2019

This patch is probably already digested in the things I did in #3688 @rip-nsk Can you have a look there if I have covered all cases?

@rip-nsk
Copy link
Contributor Author

rip-nsk commented Mar 12, 2019

This patch is probably already digested in the things I did in #3688 @rip-nsk Can you have a look there if I have covered all cases?

As far I can see, #3688 doesn't use BROTLI_STATIC_LIB_SUFFIX, so if it is not equal to some known values (""|"-static"|"_static") library will not be found.
Also, it seems mixes static/shared libraries.

@wesm
Copy link
Member

wesm commented Mar 14, 2019

Can you have a look now that the CMake refactor has been merged?

@wesm
Copy link
Member

wesm commented Mar 20, 2019

I'm removing this from the 0.13 milestone and moving to 0.14. If you are able to fix this this week we can have a closer look. Please add a PR description for the changelog (there is not description on JIRA either)

@emkornfield
Copy link
Contributor

@rip-nsk do you think you will be able to rebase this to get it reviewed?

@wesm
Copy link
Member

wesm commented Apr 24, 2019

ping @rip-nsk

@rip-nsk
Copy link
Contributor Author

rip-nsk commented Apr 26, 2019

It is hard to "resolve conflicts" with base which is changed so much..
I'll recreate this PR later (not time for now)

@wesm wesm closed this Apr 26, 2019
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.

4 participants