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

Fix cross-compilation against Poco pre-built #2599

Merged
merged 9 commits into from
Apr 20, 2019
Merged

Fix cross-compilation against Poco pre-built #2599

merged 9 commits into from
Apr 20, 2019

Conversation

lmayencourt
Copy link

This two patches remove hard-coded absolute paths to zlib and pcre when building with POCO_UNBUNDLED inside the exported PocoFoundationTarget.cmake.
This enables cross-compiling against pre-built binaries of poco provided inside a sysroot.

  • The ZLIB::ZLIB imported target set by FindZLIB.cmake is used instead of ZLIB_LIBRARIES. The installed target will use the findZLIB cmake module to define the correct path of the zlib library.

  • Use the PCRE_NAMES variable instead of the PCRE_LIBARIES provided by FindPCRE.cmake. This will add pcre to INTERFACE_LINK_LIBRARIES and will let the linker find the PCRE library.

The current build will generate: (PocoFoundationTargets.cmake)

set_target_properties(Poco::Foundation PROPERTIES
   ...
 INTERFACE_LINK_LIBRARIES "pthread;dl;rt;/usr/lib/aarch64-linux-gnu/libpcre.so;/usr/lib/aarch64-linux-gnu/libz.so"
)

Which will fails to link, because the path /usr/lib/aarch64-linux-gnu/libxxx.so doesn't exist on the host machine.

and with this modification: (PocoFoundationTargets.cmake)

set_target_properties(Poco::Foundation PROPERTIES
  ...
  INTERFACE_LINK_LIBRARIES "pcre;ZLIB::ZLIB;pthread;dl;rt"
)

Which will link properly.

This was tested on ubuntu 18.04: natively on x86 and cross-compiled for aarch64.

Avoid hard-coded absolute paths to zlib by using the imported target
ZLIB::ZLIB set by FindZLIB.cmake instead of ZLIB_LIBRARIES.

Change-Id: Ia58fc0c37d40e84b04dc72cee94a77107d6c0f52
Signed-off-by: Louis Mayencourt <louis.mayencourt@arm.com>
Avoid hard-coded absolute paths to pcre by using the PCRE_NAMES
variable instead of PCRE_LIBRARIES provided by FindPCRE module.

Change-Id: I478dd393045ed3d108b7e9fcdda1d65d92c3df1a
Signed-off-by: Louis Mayencourt <louis.mayencourt@arm.com>
@lmayencourt lmayencourt changed the title Improve cross-compilation with pre-built Fix cross-compilation against Poco pre-built Feb 22, 2019
@filiperinaldi
Copy link

Hi @Bjoe, you seem to have some experience on Poco's build system. Do you have some time for a quick look into this PR? Thanks.

@obiltschnig obiltschnig requested a review from bachp March 14, 2019 20:28
Copy link
Member

@bachp bachp left a comment

Choose a reason for hiding this comment

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

I think the change for ZLIB to importet targets is very good.

For PCRE I'm not that happy. What you did here is similar to the hack we did in OpenEmbedded.

@@ -97,7 +97,10 @@ set_target_properties(Foundation
DEFINE_SYMBOL Foundation_EXPORTS
)

target_link_libraries(Foundation PUBLIC ${PCRE_LIBRARIES} ${ZLIB_LIBRARIES})
if (POCO_UNBUNDLED)
target_link_libraries(Foundation PUBLIC ${PCRE_NAMES} ZLIB::ZLIB)
Copy link
Member

Choose a reason for hiding this comment

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

Using PCRE_NAMES here seems very hacky. It's basically equivalent to just putting pcre.

I would prefer to change the FindPCRE.cmake module to provide an exported target that can be used.

Copy link
Author

Choose a reason for hiding this comment

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

Hi @bachp,
Thanks for your comments !

I'm also not really happy with my solution for pcre but I didn't find a simpler solution for this. My first idea was also to modify FindPCRE.cmake to provide an exported target, but this not enough:
The FindPCRE.cmake is available during the compilation, but it is not part of the installed files. This end up with pre-build looking for an exported target that can not be resolved. This can be solved by installing FindPCRE.cmake. The problem is not here with zlib because cmake have already a FindZLIB.cmake module.

If installing a module to find PCRE as part of Poco is not an issue, I would be happy to push the required changes.

Copy link
Member

Choose a reason for hiding this comment

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

I think installing the modified FindPCRE.cmake along Poco would be an acceptable solution. In my opinion you can continue.

Copy link
Contributor

Choose a reason for hiding this comment

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

First, thanks @lmayencourt . I fail to see this issue when I refactor/cleaup all XXX_LIBRARIES places to use the modern art of cmake.
I agree with @bachp , because what with the "include path" for PCRE ?
Let me cleanup the FindPCRE.cmake and install when POCO_UNBUNDLED is true.
I would like to use your merge request, is this ok for you @lmayencourt?

Finally, I found another place where ZLIB_LIBRARIES is used: PDF/CMakeList.txt:129 ....
I can also fix this.

@Bjoe
Copy link
Contributor

Bjoe commented Mar 20, 2019

Hi @Bjoe, you seem to have some experience on Poco's build system. Do you have some time for a quick look into this PR? Thanks.

Sorry for the late answer....

@Bjoe
Copy link
Contributor

Bjoe commented Mar 20, 2019

@lmayencourt I create a pull request in your repo with my changes. see https://github.com/lmayencourt/poco/pull/1 Can you test please. Thanks

@Bjoe
Copy link
Contributor

Bjoe commented Mar 24, 2019

@lmayencourt I have added to the cmake build to install FindPCRE see https://github.com/lmayencourt/poco/pull/1 . It will be installed in CMAKE_INSTALL_PREFIX/lib/cmake/Poco/FindPCRE.cmake .... I'm not sure if this is a "good place". Please test.

Btw. I find that "expat" and "sqlite" has the same issue/problem. I will fix this also if this solution works fine.

@Bjoe
Copy link
Contributor

Bjoe commented Apr 8, 2019

@lmayencourt Thanks for merging my fixes ....
@bachp Can you please review again?

Again, at the moment I install FindXYZ.cmake (for example FindPCRE.cmake) on Unix in CMAKE_INSTALL_PREFIX/lib/cmake/Poco/FindPCRE.cmake and set the CMAKE_MODULE_PATH in PocoFoundationConfig.cmake via list(APPEND CMAKE_MODULE_PATH "${CMAKE_CURRENT_LIST_DIR}") .... I didn't like this and I hope this solution did not create any side effects .... at the moment this is my quickest idea ... if any one have a better solution, please give me a hint or fix this here.

@Bjoe Bjoe requested a review from bachp April 8, 2019 11:47
@Bjoe
Copy link
Contributor

Bjoe commented Apr 8, 2019

Btw. we should also merge this to poco-1.9.1 branch.
I can do this also but please wait before you merge....

Copy link
Member

@bachp bachp left a comment

Choose a reason for hiding this comment

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

Looks good

@@ -40,7 +40,7 @@ if (POCO_UNBUNDLED)
src/pcre_ucd.c
src/pcre_tables.c
)

Copy link
Member

Choose a reason for hiding this comment

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

Unneeded whitespace?

@Bjoe Bjoe merged commit bd98f5c into pocoproject:develop Apr 20, 2019
Bjoe pushed a commit to Bjoe/poco that referenced this pull request Apr 20, 2019
* Use ZLIB and PCRE imported target to improve portability
* Set EXPAT and SQLite3 library for XML and SQL if POCO_UNBUNDLED is true in cmake build
Bjoe pushed a commit to Bjoe/poco that referenced this pull request Apr 20, 2019
* Use ZLIB and PCRE imported target to improve portability
* Set EXPAT and SQLite3 library for XML and SQL if POCO_UNBUNDLED is true in cmake build
Bjoe added a commit that referenced this pull request Apr 22, 2019
* Use ZLIB and PCRE imported target to improve portability
* Set EXPAT and SQLite3 library for XML and SQL if POCO_UNBUNDLED is true in cmake build
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