(fix) Fixing build issue for googletest on Mac#855
Closed
varunu28 wants to merge 3 commits intocmu-db:masterfrom
Closed
(fix) Fixing build issue for googletest on Mac#855varunu28 wants to merge 3 commits intocmu-db:masterfrom
varunu28 wants to merge 3 commits intocmu-db:masterfrom
Conversation
Author
|
@apavlo I have updated the check to be applied only when googletest dependency is present in the system. |
Author
|
@apavlo Can you please re-trigger the build workflow to verify the latest commit? Thanks :) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Heads Up
Please note that this PR was created by someone who has almost no knowledge of C++ build system. So if you think something doesn't makes sense then there is a pretty good chance that you are right. I have almost no clue of the CMake build system & I am happy to learn the ropes here. Also for full transparency, I have leveraged LLM tools to understand the underlying root cause & triage this issue.
Why this change?
This change tries to address #834 .
Actual code change
The issue was happening as system headers were given a priority over the bundled third party dependencies due to usage of
SYSTEMkeyword. The fix removes theSYSTEMkeyword fromCMakeLists.txtfrom bothgoogletestdependencies & also adds bundledgoogletestinclude directories withBEFOREkeyword in order for CMake to include these paths in the beginning of dependency scan.Testing
makefinishes successfullyAlso invoked test & it failed(as expected) which verifies that the test library is working as intended
