Skip to content

Turn on cpp test to bypass abseil-py's build error #18

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

Merged
merged 3 commits into from
Jul 16, 2021

Conversation

cynthiajoan
Copy link
Contributor

No description provided.

@google-cla google-cla bot added the cla: yes label Jul 9, 2021
@cynthiajoan cynthiajoan requested a review from chkuang-g July 9, 2021 20:34
@a-maurice
Copy link
Collaborator

I don't think that this is a great solution, because it'll mean building a ton of tests that we don't actually want.

@cynthiajoan
Copy link
Contributor Author

cynthiajoan commented Jul 9, 2021

I don't think that this is a great solution, because it'll mean building a ton of tests that we don't actually want.

I agree that's not an ideal one, better solution is to use -DBUILD_TESTING=off, but unfortunately that doesn't work. And since we are not directly refer abseil-py, it's the libs like boringssl or curl did that, even the flag -DBUILD_TESTING=off is already set on cpp's CMakeList.txt, the error would still hit the second run.

Or another approach is, we add a mock unity test project which would not take extra time to build.

chkuang-g
chkuang-g previously approved these changes Jul 12, 2021
@cynthiajoan
Copy link
Contributor Author

Update the PR, only turn on stub flag for CPP. corresponding CPP PR is firebase/firebase-cpp-sdk#528

@cynthiajoan cynthiajoan requested a review from a-maurice July 16, 2021 18:17
build_bash.sh Outdated
@@ -36,6 +36,7 @@ fi

CMAKE_OPTIONS="${CMAKE_OPTIONS} -DUNITY_ROOT_DIR=${UNITY_ROOT_DIR}"
CMAKE_OPTIONS="${CMAKE_OPTIONS} -DFIREBASE_UNITY_BUILD_TESTS=ON"
CMAKE_OPTIONS="${CMAKE_OPTIONS} -DFIREBASE_CPP_BUILD_STUB_TESTS=ON"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you add a comment explaining why this is needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DONE

@cynthiajoan cynthiajoan merged commit 923f19f into main Jul 16, 2021
@firebase firebase locked and limited conversation to collaborators Jun 4, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants