Skip to content
This repository has been archived by the owner on Aug 2, 2022. It is now read-only.

Fix find_path bug in CMakeLists #280

Merged

Conversation

jmazanec15
Copy link
Member

Issue #, if available:
#279

Description of changes:
This PR fixes a build failure when using CMake >= 3.18. During the build process, we use find_path to check if the nmslib submodule has already been pulled. If it has not, we pull it (in line 37). Recently, Ubuntu 18 in Github actions upgraded from Cmake 3.17 to 3.19.

For some reason, I added the REQUIRED parameter in find_path. This had no effect in Cmake < 3.18. In Cmake >= 3.18, this causes CMake process to fail: https://cmake.org/cmake/help/v3.18/command/find_path.html. So, the fix is to remove REQUIRED parameter.

I verified the change works for CMake 3.19 on Ubuntu 18. I also verified that the fix does not break functionality for 3.17 on my Mac.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@jmazanec15 jmazanec15 added the Bug Fixes Change that fixes a bug label Dec 9, 2020
@codecov
Copy link

codecov bot commented Dec 9, 2020

Codecov Report

Merging #280 (61fdd9d) into master (fd32d2f) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #280   +/-   ##
=========================================
  Coverage     79.34%   79.34%           
  Complexity      359      359           
=========================================
  Files            58       58           
  Lines          1404     1404           
  Branches        126      126           
=========================================
  Hits           1114     1114           
  Misses          242      242           
  Partials         48       48           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9359f36...61fdd9d. Read the comment docs.

Copy link
Member

@vamshin vamshin left a comment

Choose a reason for hiding this comment

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

LGTM!

@jmazanec15 jmazanec15 changed the title remove required from find_path Fix find_path bug in CMakeLists Dec 9, 2020
@jmazanec15 jmazanec15 merged commit 8c802d9 into opendistro-for-elasticsearch:master Dec 10, 2020
jmazanec15 added a commit to jmazanec15/k-NN that referenced this pull request Dec 30, 2020
jmazanec15 added a commit to jmazanec15/k-NN that referenced this pull request Dec 30, 2020
jmazanec15 added a commit to jmazanec15/k-NN that referenced this pull request Dec 30, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Bug Fixes Change that fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants