Feature/gha include firestore clean #116
Merged
Conversation
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
objc_spec doesn't crash locally on macs at the very least. But it is crashing on github runners. objc_integration test require a simulator and are more tricky to setup.
Firestore had to be updated from nanopb 0.3.9.5 to 0.3.9.6 in order for it to build with python3. This prevented builds of firebase-cpp-sdk when we included firestore with python3
Fixes errors with undefined symbols when including firestore in our cpp sdk builds
The buildtrees and downloads directory are used by vcpkg for building the requested libraries. buildtrees contains all build intermediates and also the source code for debugging. If you want to debug with source code for these dependencies, you shouldn't delete the buildtrees directory. In most cases, these directories are not needed and can take gigabytes of unnecessary space.
jonsimantov reviewed Aug 19, 2020
scripts/gha/build_desktop.py Outdated
Comment on lines 78 to 79
| if os.path.exists(buildtrees_dir_path): | ||
| shutil.rmtree(buildtrees_dir_path) |
There was a problem hiding this comment.
Maybe put this in utils as a utility function?
a-maurice reviewed Aug 20, 2020
.github/workflows/desktop.yml Outdated
| run: | | ||
| cd build | ||
| ctest --verbose | ||
| ctest --verbose -E "firestore_(objc_spec|objc_integration)_test" |
There was a problem hiding this comment.
I think it would be better to exclude them via https://github.com/firebase/firebase-cpp-sdk/blob/master/cmake/CTestCustom.cmake.
Either way, putting a comment explaining why it is being excluded would be good.
There was a problem hiding this comment.
Yeah, I was contemplating where to put this. Ended up going with this approach because it is more visible and will be a reminder on every workflow update/run. But I noticed that there are few tests already explicitly disabled in CTestCustom.cmake. So maybe having one list of all disabled tests is better. What do you think?
This will ensure that these tests are excluded even when run locally by users when they are not using the provided workflow run python scripts and keeps all excluded tests in one place.
a-maurice approved these changes Aug 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
4 participants
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.
Since we decided to support just Python3 for building our SDK, firestore's nanopb dependency had to be updated to 0.3.9.6 as 0.3.9.5 has build errors in Python3. nanopb is updated across all firebase open source sdks and google3.
This commit specifically fixes issues with finding modules with Python3.
nanopb/nanopb@63f8e5e
Relevant PRs,
google/nanopb-podspec#14
firebase/firebase-ios-sdk#6214
cl/326682471