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

cmake: applications that use bitpit should not relay on information gathered at configuration time #112

Merged
merged 1 commit into from
Dec 1, 2020

Conversation

andrea-iob
Copy link
Member

@alessandro-alaia noticed that BITPITConfig.cmake adds the external dependencies found during bitpit configuration to BITPIT_LIBRARIES, but, at the same time, it is also using the cmake find infrastructure to find the same dependencies.

Is there any specific reason why we need both?

In this pull request I'm proposing to remove the calls to the cmake find functions and relay entirely on the libraries found during bitpit configuration.

@edoardolombardi
Copy link
Member

Is there any specific reason why we need both?

I suppose no, there is no reason to have both finding dependencies.

Your proposal fix the double search, but I think that is not a good solution to hard set the path used during bitpit compilation in external dependencies.

A solution is to maintain the auto-find dependencies of bitpit (that I like) by keeping the find_package calls and appending the correct exposed variables to bitpit dependencies. Indeed, currently they are found but it seems to me that they are never used in bitpit configuration.
Obviously, we have to provide all the Find.cmake files not provided by packages installation, but I think that this logic is already adopted (i.e. for cblas, lapacke, petsc).

Just as an example we can modify the find dependencies loop in BITPITConfig.cmake:

# Find external packages
list(REVERSE _EXTERNAL_DEPENDENCIES)
foreach(_DEPENDENCY ${_EXTERNAL_DEPENDENCIES})
    find_package("${_DEPENDENCY}" REQUIRED)
    if (${_DEPENDENCY} STREQUAL "LibXml2")
        list(APPEND BITPIT_LIBRARIES ${LIBXML2_LIBRARY})
        list(APPEND BITPIT_INCLUDE_DIRS ${LIBXML2_INCLUDE_DIR})    
    elseif (${_DEPENDENCY} STREQUAL "PETSc")
        list(APPEND BITPIT_LIBRARIES ${PETSC_LIBRARIES})
        list(APPEND BITPIT_INCLUDE_DIRS ${PETSC_INCLUDES})    
        list(APPEND BITPIT_INCLUDE_DIRS ${PETSC_INCLUDE_CONF})    
        list(APPEND BITPIT_INCLUDE_DIRS ${PETSC_INCLUDE_DIR})    
    else()    
        list(APPEND BITPIT_LIBRARIES ${${_DEPENDENCY}_LIBRARIES})
        list(APPEND BITPIT_INCLUDE_DIRS ${${_DEPENDENCY}_INCLUDES})    
    endif()
endforeach()

(One if scope it should be coded for each package without default ${_DEPENDENCY}_LIBRARIES and ${_DEPENDENCY}_INCLUDES names)

In this way the last two hard sets are useless and they can be erased:

# Add external libraries
list(APPEND BITPIT_LIBRARIES "@BITPIT_SHARED_EXTERNAL_LIBRARIES@")

# Add external inclued paths
list(APPEND BITPIT_INCLUDE_DIRS "@BITPIT_EXTERNAL_INCLUDE_DIRS@")

@andrea-iob andrea-iob force-pushed the cmake.avoid.finding.external.dependencies branch from 072c7ca to 0e28707 Compare November 17, 2020 07:39
@andrea-iob
Copy link
Member Author

I've implemented your idea.

Tests were performed only on Linux.

@andrea-iob
Copy link
Member Author

Can someone tests these changes on Windows?

@andrea-iob andrea-iob changed the title cmake: applications that use bitpit doesn't need to find external dependencies cmake: applications that use bitpit should not relay on information gathered at configuration time Nov 25, 2020
@andrea-iob andrea-iob marked this pull request as ready for review November 25, 2020 15:20
@roccoarpa
Copy link
Contributor

Can someone tests these changes on Windows?

It works.
Tested with pull rebased on actual master tip commit : 53b8274

@andrea-iob andrea-iob force-pushed the cmake.avoid.finding.external.dependencies branch from 0e28707 to 316c403 Compare November 30, 2020 15:38
@andrea-iob
Copy link
Member Author

Thanks for the tests. I've rebased the branch on the current master. Once the automatic tests finish I will do the merge.

@andrea-iob andrea-iob merged commit 3daa3d8 into master Dec 1, 2020
@andrea-iob andrea-iob deleted the cmake.avoid.finding.external.dependencies branch December 1, 2020 08:04
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.

3 participants