-
Notifications
You must be signed in to change notification settings - Fork 27
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
[3.1] more cleanup to EosioTester.cmake to use CMAKE_INSTALL_FULL_LIBDIR instead #648
Conversation
endif() | ||
list(APPEND CMAKE_MODULE_PATH ${EOSIO_ROOT}/lib/cmake/eosio) | ||
list(APPEND CMAKE_MODULE_PATH ${EOSIO_ROOT}/lib64/cmake/eosio) | ||
include(EosioTester) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this section above could use some 👀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If EOSIO_ROOT
is not defined, would it be likely EOS_ROOT_DIR
not defined either? It is safer to check if EOS_ROOT_DIR
is defined before use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@EOS_ROOT_DIR@
is always defined because it's replaced as part of the file configuration -- the resulting file will always be one of two values depending of it's building the file for the in-tree-build case (former) vs the installed case (latter)
Lines 194 to 195 in d668e01
set(EOS_ROOT_DIR "${CMAKE_BINARY_DIR}/lib") | |
configure_file(${CMAKE_SOURCE_DIR}/CMakeModules/eosio-config.cmake.in ${CMAKE_BINARY_DIR}/lib/cmake/eosio/eosio-config.cmake @ONLY) |
Lines 199 to 200 in d668e01
set(EOS_ROOT_DIR "${CMAKE_INSTALL_FULL_LIBDIR}") | |
configure_file(${CMAKE_SOURCE_DIR}/CMakeModules/eosio-config.cmake.in ${CMAKE_BINARY_DIR}/modules/eosio-config.cmake @ONLY) |
So the intention seems to be if EOSIO_ROOT
is not defined then a sensible default should be used.
In-tree build
Previously
set(EOS_ROOT_DIR ${CMAKE_BINARY_DIR})
- (no EOSIO_ROOT meant)
set(EOSIO_ROOT "@EOS_ROOT_DIR@")
list(APPEND CMAKE_MODULE_PATH ${EOSIO_ROOT}/lib/cmake/eosio)
list(APPEND CMAKE_MODULE_PATH ${EOSIO_ROOT}/lib64/cmake/eosio)
This ultimately meant for something like a build in /home/moomoo/mandel/build
it'd end up adding these two paths to the CMAKE_MODULE_PATH
/home/moomoo/mandel/build/lib/cmake/eosio
/home/moomoo/mandel/build/lib64/cmake/eosio
Now
set(EOS_ROOT_DIR ${CMAKE_BINARY_DIR}/lib)
- (no EOSIO_ROOT means just)
list(APPEND CMAKE_MODULE_PATH @EOS_ROOT_DIR@/cmake/eosio)
So this means only /home/moomoo/mandel/build/lib/cmake/eosio
is added to the CMAKE_MODULE_PATH
but afaict this is fine since you'll never get a lib64 in such an in-tree case.
Installed
Previously
set(EOS_ROOT_DIR ${CMAKE_INSTALL_PREFIX})
- (no EOSIO_ROOT meant)
set(EOSIO_ROOT "@EOS_ROOT_DIR@")
list(APPEND CMAKE_MODULE_PATH ${EOSIO_ROOT}/lib/cmake/eosio)
list(APPEND CMAKE_MODULE_PATH ${EOSIO_ROOT}/lib64/cmake/eosio)
This ultimately meant for something like an install with -DCMAKE_INSTALL_PREFIX=/usr
you'd end up adding these two paths to the CMAKE_MODULE_PATH
/usr/lib/cmake/eosio
/usr/lib64/cmake/eosio
But this is wrong! The files were installed to /usr/lib/x86_64-linux-gnu/cmake/eosio
instead.
Now
set(EOS_ROOT_DIR ${CMAKE_INSTALL_FULL_LIBDIR})
-- critically this matches where the actual files will beinstall()
ed- (no EOSIO_ROOT means just)
list(APPEND CMAKE_MODULE_PATH @EOS_ROOT_DIR@/cmake/eosio)
This will only end up with ${CMAKE_INSTALL_FULL_LIBDIR}/cmake/eosio
as the CMAKE_MODULE_PATH
but that'll always match up the actual installed path: /usr/lib/x86_64-linux-gnu/cmake/eosio
EOSIO_ROOT?
EOSIO_ROOT
is the wild card here... I'm guessing some documentation or previous user workflow manually sets that and expects it to work. So I've decided to just keep the old behavior as-is since I'm not really sure all the different scenarios for that. Only the "automatic"/"fall back" logic when EOSIO_ROOT
is not specified is changed.
…_install_fixups_31x
Some parts of EosioTester.cmake were still incorrect. It is looking for libraries in
@CMAKE_INSTALL_PREFIX@/lib64
and@CMAKE_INSTALL_PREFIX@/lib
however for something like Ubuntu 20.04, libraries get installed to${CMAKE_INSTALL_PREFIX}/x86_64-linux-gnu
! Change this to look for libraries in${CMAKE_INSTALL_FULL_LIBDIR}
instead -- the same location libraries areinstall()
ed to.Then fix up libbuiltins & libff to be consistent and install to
${CMAKE_INSTALL_FULL_LIBDIR}
too.Needs eosnetworkfoundation/mandel-fc#43 & eosnetworkfoundation/libff#3
Indirect work on #642