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

Change CMake logic for lib cdi_ndi to ensure upstream code can build when NDI is not found #818

Merged
merged 3 commits into from
May 4, 2020

Conversation

keadyk
Copy link
Collaborator

@keadyk keadyk commented May 1, 2020

Background

  • previously, the cdi_ndi library wasn't built at all when NDI was not found
  • this is fine in theory, but it made things tricky in upstream code; because the NDI_FOUND cmake variable was not available, there was no obvious way to protect against includes of draco cdi_ndi header files that were not installed.

Purpose of Pull Request

  • Instead of omitting the cdi_ndi lib entirely when NDI is not found, stub out the routines that need NDI (have them throw assertions)
  • This means that upstream code can be built without protecting cdi_ndi includes in macros -- the calls to cdi_ndi will just throw if it is not found
  • This is needed to get a corresponding pull request in gitlab going (I will update the description of that PR shortly)

Description of changes

  • update CMakeLists logic for cdi_ndi - there was previously a confusing "if(NDI_FOUND) and not environment(travis)" statement wrapping the cmake logic. I think it was trying to protect against building the cdi_ndi lib in CI -- but you'd think that would be automatic because NDI_FOUND would be false... (?!)
  • Move ndi.h include into config file, and remove from other cdi_ndi files
  • Wrap actual calls to ndi routines / references to env. variables in NDI_FOUND macro
  • @KineticTheory I am open to other ways to do this in CMake, and also more than willing to explain why I did things this way :)

Status

@keadyk keadyk requested a review from KineticTheory May 1, 2020 16:09
@KineticTheory
Copy link
Collaborator

@keadyk I'll review this over the weekend.

@keadyk
Copy link
Collaborator Author

keadyk commented May 1, 2020

@KineticTheory Thank you -- no rush!! Hopefully my latest commit fixes the last few warnings :)

@codecov
Copy link

codecov bot commented May 1, 2020

Codecov Report

Merging #818 into develop will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           develop    #818   +/-   ##
=======================================
  Coverage     94.2%   94.2%           
=======================================
  Files          367     367           
  Lines        17259   17259           
=======================================
  Hits         16263   16263           
  Misses         996     996           

Copy link
Collaborator

@KineticTheory KineticTheory left a comment

Choose a reason for hiding this comment

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

LGTM

@KineticTheory KineticTheory merged commit 6a81a5a into lanl:develop May 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants