Feature/fix android windows builds #346

vimanyu · 2021-03-25T08:51:21Z

Fixes a series of issues that were causing Windows Android builds to error out.

flatbuffers was not getting downloaded as an external library and gradle/cmake build was failing to find cmake function build_flatbuffers
Android Gradle CMake was invoking system level CMake as a child process to download and build external libraries. In addition to using the same cmake as the parent process, some CMake options had to be passed through to the child process to make it work correctly. For example, Android Gradle CMake prefers to use the generator "Android Gradle - Ninja" and ninja as the make program.
strings.exe is a special utility tool required on Android/Windows. Our android build script was downloading strings.exe to the repo root but since the PATH was not modified, this downloaded tool wasn't found. Additionally, there is another conflicting strings.exe on Github runners which meant that we have to prepend our repo root directory to the PATH variable on Github runners.
strings.exe prompts for accepting license on first invocation. Found an undocumented flag -accepteula that accepts the license which is run in a dummy github workflow step. Just running with this fails exits the command with error code but we ignore that error.

For downloading external third party libraries, our cmake system runs a child process cmake command to configure/build external libraries. For Android/Windows combination, a specific cmake configuration is used using a toolchain from the android ndk and it's own distinct make program. We want to ensure that these values are passed to the child cmake too.

-accepteula works just on Windows. Linux/Mac runners are hanging indefinitely when this flag is supplied. Took it out from gradle and instead added a "dummy" step in workflow to just accept the license.

github-actions · 2021-03-25T11:34:35Z

❌ Integration test FAILED

Requested by @DellaBitta on commit b0fe3b7
Last updated: Thu Mar 25 06:44:53 PDT 2021
View integration test results

DellaBitta

Approved, but also please check my comments.

.github/workflows/android.yml

DellaBitta · 2021-03-25T12:32:11Z

cmake/external_rules.cmake


  execute_process(
-    COMMAND ${ENV_COMMAND} cmake --build . -- ${cmake_build_args}
+    COMMAND ${ENV_COMMAND} ${CMAKE_COMMAND} --build . -- ${cmake_build_args}


Should there really be a space between -- and ${cmake_build_args} ?

I'm just not clear on why we use the "--" at all - typically that's how you separate sets of arguments, for example the arguments afterwards are sent to a subprocess, or are considered a list of filenames even if they start with a "-". But why is that necessary here?

(For example, if you have a filename called "-q", you can't normally run "grep hello -q" because grep will interpret "-q" as an argument. So you must do "grep hello -- -q" which tells grep that everything following the "--" is a filename. But I'm still not sure why that's required for this cmake call.)

The more you know.

Good catch. I didn't spot this and am not sure why this exists. I substituted hard coded cmake to using a variable.

What's with the "--" though?

The only change that I made is "cmake" converted to "${CMAKE_COMMAND}". Will investigate why the "-- ${cmake_build_args}" exists in another PR as I don't want to introduce new changes for this Windows/Android task.

DellaBitta · 2021-03-25T12:32:25Z

scripts/gha/print_matrix_configuration.py

  "android": {
    "matrix": {
-      "os": ["ubuntu-latest", "macos-latest"],
+      "os": ["ubuntu-latest", "macos-latest", "windows-latest"],


Noticed errors in Build logs complaining of CMakeLists not specifying minimum version.

vimanyu · 2021-03-29T22:22:30Z

Integration tests builds are also fixed now.
https://github.com/firebase/firebase-cpp-sdk/runs/2221512221?check_suite_focus=true

The only error was with the Storage test which is unrelated to issues with building on Windows for Android.

[  FAILED  ] FirebaseStorageTest.TestLargeFilePauseResumeAndDownloadCancel

vimanyu added 17 commits March 24, 2021 19:17

turning on windows for android workflows

05b149e

also passing through generator

1d60760

adding strings.exe to PATH

b2af47f

building just one abi for quick iteration

358d1f2

fixed typo

95acaef

run strings once to accept license

aa81708

print out modified path for debugging

765bfd1

tring powershell add path

c354526

splitting work on strings.exe

953229c

accept license for strings.exe

358de86

more debug output

b842583

removing accept license task for strings.exe

88a92ce

more descriptive comment

c5cc0a7

removing --stacktrace from gradlew

4c3b15c

restoring all android abis

70d4171

linux and mac strings cmd does not recognize flag.

b0fe3b7

-accepteula works just on Windows. Linux/Mac runners are hanging indefinitely when this flag is supplied. Took it out from gradle and instead added a "dummy" step in workflow to just accept the license.

vimanyu requested review from DellaBitta and jonsimantov March 25, 2021 08:51

google-cla bot added the cla: yes label Mar 25, 2021

DellaBitta added the tests-requested: quick Trigger a quick set of integration tests. label Mar 25, 2021

github-actions bot added tests: in-progress This PR's integration tests are in progress. and removed tests-requested: quick Trigger a quick set of integration tests. labels Mar 25, 2021

DellaBitta previously approved these changes Mar 25, 2021

View reviewed changes

github-actions bot added the tests: failed This PR's integration tests failed. label Mar 25, 2021

firebase-workflow-trigger bot removed the tests: in-progress This PR's integration tests are in progress. label Mar 25, 2021

passing on options from parent cmake process to child process

cbba709

vimanyu dismissed DellaBitta’s stale review via cbba709 March 25, 2021 19:27

avoid command from exiting with error code

72a757a

vimanyu added 2 commits March 25, 2021 13:41

add min cmake version

c1b74e5

Noticed errors in Build logs complaining of CMakeLists not specifying minimum version.

fixed cmake child process invocation for gtest and absl

c405c64

jonsimantov approved these changes Mar 29, 2021

View reviewed changes

jonsimantov removed the tests: failed This PR's integration tests failed. label Mar 29, 2021

vimanyu merged commit 6071a27 into main Mar 29, 2021

vimanyu deleted the feature/fix-android-windows-builds branch March 29, 2021 22:22

vimanyu self-assigned this Mar 29, 2021

firebase locked and limited conversation to collaborators Apr 29, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Feature/fix android windows builds #346

Feature/fix android windows builds #346

Uh oh!

vimanyu commented Mar 25, 2021

Uh oh!

github-actions bot commented Mar 25, 2021 •

edited

Loading

Uh oh!

DellaBitta left a comment

Uh oh!

Uh oh!

DellaBitta Mar 25, 2021

Uh oh!

jonsimantov Mar 25, 2021

Uh oh!

DellaBitta Mar 25, 2021

Uh oh!

vimanyu Mar 25, 2021

Uh oh!

jonsimantov Mar 29, 2021

Uh oh!

vimanyu Mar 29, 2021

Uh oh!

DellaBitta Mar 25, 2021

Uh oh!

vimanyu commented Mar 29, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Feature/fix android windows builds #346

Feature/fix android windows builds #346

Uh oh!

Conversation

vimanyu commented Mar 25, 2021

Uh oh!

github-actions bot commented Mar 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

❌ Integration test FAILED

Uh oh!

DellaBitta left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DellaBitta Mar 25, 2021

Choose a reason for hiding this comment

Uh oh!

jonsimantov Mar 25, 2021

Choose a reason for hiding this comment

Uh oh!

DellaBitta Mar 25, 2021

Choose a reason for hiding this comment

Uh oh!

vimanyu Mar 25, 2021

Choose a reason for hiding this comment

Uh oh!

jonsimantov Mar 29, 2021

Choose a reason for hiding this comment

Uh oh!

vimanyu Mar 29, 2021

Choose a reason for hiding this comment

Uh oh!

DellaBitta Mar 25, 2021

Choose a reason for hiding this comment

Uh oh!

vimanyu commented Mar 29, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions bot commented Mar 25, 2021 •

edited

Loading