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

Win32: Fix CAFS issues wrt CMake-3.13+ #561

Merged
merged 2 commits into from
Feb 4, 2019

Conversation

KineticTheory
Copy link
Collaborator

@KineticTheory KineticTheory commented Jan 10, 2019

Background

  • After upgrading cmake to 3.13.2, I found some new issues with the CAFS logic as used by Jayenne. This commit attempts to fix those issues.
  • The search for MS-MPI from a MinGW Makefile project (GNU Fortran) now fails because a simple C program that links to libmpi.a fails. The failure is expected w/o extra work on our part. I needed to interrupt the build logic for this unique case so that the build system still reports MPI as 'found.'

Description of changes

  • Store the value of MPI_C_WORKS as discovered by the master build (e.g. Visual Studio) in draco-config.cmake.
  • Using the above variable along with the values of WIN32, MPI_C_LIBRARIES, and CMAKE_GENERATOR, allow the FindMPI command to return TRUE in very special circumstances.
  • Minor formatting teaks.

Status

After upgrading cmake to 3.13.2, I found some new issues with the CAFS logic
as used by Jayenne.  This commit attempts to fix those issues.

+ The search for MS-MPI from a MinGW Makefile project (GNU Fortran) now fails
  because a simple C program that links to libmpi.a fails.  The failure is
  expected w/o extra work on our part.  I needed to interrupt the build logic
  for this unique case so that the build system still reports MPI as 'found.'
+ Store the value of `MPI_C_WORKS` as discovered by the master build (e.g.
  Visual Studio) in `draco-config.cmake`.
+ Using the above variable along with the values of `WIN32`, `MPI_C_LIBRARIES`,
  and `CMAKE_GENERATOR`, allow the `FindMPI` command to return `TRUE` in very
  special circumstances.
+ Minor formatting teaks.
@KineticTheory KineticTheory added this to the Draco-7_1_0 milestone Jan 10, 2019
@KineticTheory KineticTheory self-assigned this Jan 10, 2019
@KineticTheory KineticTheory changed the title Wi32: Fix CAFS issues wrt CMake-3.13+ Win32: Fix CAFS issues wrt CMake-3.13+ Jan 10, 2019
@codecov
Copy link

codecov bot commented Jan 10, 2019

Codecov Report

Merging #561 into develop will increase coverage by 0.3%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           develop    #561     +/-   ##
=========================================
+ Coverage     93.3%   93.6%   +0.3%     
=========================================
  Files          377     377             
  Lines        17676   17608     -68     
=========================================
+ Hits         16496   16498      +2     
+ Misses        1180    1110     -70

@KineticTheory
Copy link
Collaborator Author

@clevelam All CDash test pass. This is ready for final review and merge.

if( NOT "${MPI_C_FOUND}" AND "${Draco_MPI_C_WORKS}"
AND "${MPI_C_LIBRARIES}" MATCHES "msmpi"
AND "${CMAKE_GENERATOR}" STREQUAL "MinGW Makefiles")
if( EXISTS "${MPI_C_LIBRARIES}" AND EXISTS "${MPI_C_INCLUDE_DIRS}" )
Copy link
Collaborator

Choose a reason for hiding this comment

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

This logic seems complicated it. Can it be simplified to set both the variables true only on the existence of the MPI libraries and directories?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes -- it is very complicated because I need to work around a bug in cmake. I think I can eliminate the first two checks. I'll try do do this -- but it may break the no-MPI build.

#check_cxx_compiler_flag( "-Wunused-macros" HAS_WUNUSED_MACROS )
check_cxx_compiler_flag( "-Wzero-as-null-pointer-constant" HAS_WZER0_AS_NULL_POINTER_CONSTANT )
check_cxx_compiler_flag( "-Wzero-as-null-pointer-constant"
HAS_WZER0_AS_NULL_POINTER_CONSTANT )
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this really spelled ZER'0' with the number 0 not ZERO with the letter o? that seems strange.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll fix this. Actually, I will fix the spelling and comment out the check as it is currently unused.

@clevelam clevelam merged commit 2db402e into lanl:develop Feb 4, 2019
@KineticTheory KineticTheory deleted the win32_fix_cmake312_CAFS_issue branch February 7, 2019 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants