Skip to content

Feature/tvos ios one build script #466

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 18 commits into from
Jun 24, 2021
Merged

Conversation

vimanyu
Copy link
Contributor

@vimanyu vimanyu commented Jun 15, 2021

Single build script that builds for ios and tvos.

python3 scripts/gha/build_ios_tvos.py

Build for iOS and tvOS.

optional arguments:
  -h, --help            show this help message and exit
  -b BUILD_DIR, --build_dir BUILD_DIR
                        Name of build directory.
  -s SOURCE_DIR, --source_dir SOURCE_DIR
                        Directory containing source code (top level CMakeLists.txt)
  -p PLATFORM [PLATFORM ...], --platform PLATFORM [PLATFORM ...]
                        List of platforms to build for.
  -a ARCHITECTURE [ARCHITECTURE ...], --architecture ARCHITECTURE [ARCHITECTURE ...]
                        List of architectures to build for.
  -t TARGET [TARGET ...], --target TARGET [TARGET ...]
                        List of CMake build targets
  -o OS [OS ...], --os OS [OS ...]
                        List of operating systems to build for.

In addition to building frameworks for all os/platform/architecture combinations, it will also build "universal" frameworks and "xcframeworks".
The xcframeworks being generated contain both ios and tvos libraries.

Other notable change is a new "merged" Info.plist template file that contains entries for both ios and tvos.

@vimanyu vimanyu self-assigned this Jun 15, 2021
@google-cla google-cla bot added the cla: yes label Jun 15, 2021
@vimanyu vimanyu added tests-requested: full Trigger a FULL set of integration tests (uses expanded test matrix). and removed cla: yes labels Jun 15, 2021
@github-actions github-actions bot added tests: in-progress This PR's integration tests are in progress. and removed tests-requested: full Trigger a FULL set of integration tests (uses expanded test matrix). labels Jun 15, 2021
@github-actions
Copy link

github-actions bot commented Jun 15, 2021

❌  Integration test FAILED

Requested by @vimanyu on commit 4bb2f22
Last updated: Tue Jun 15 03:27:36 PDT 2021
Download integration test detailed logs and artifacts

Failures Configs
firestore [TEST] [FAILURE] [Windows] [boringssl]
(1 failed tests)  CanLoadBundlesWithoutProgressUpdates

@google-cla google-cla bot added the cla: yes label Jun 15, 2021
@github-actions github-actions bot added the tests: failed This PR's integration tests failed. label Jun 15, 2021
@firebase-workflow-trigger firebase-workflow-trigger bot removed the tests: in-progress This PR's integration tests are in progress. label Jun 15, 2021
@jonsimantov
Copy link
Contributor

Is there still a build script in build_scripts/ios?

@vimanyu
Copy link
Contributor Author

vimanyu commented Jun 16, 2021

Is there still a build script in build_scripts/ios?

For now, yes as our documentation needs to be updated too. Infact, there are build scripts specifically for tvos.
I will clean things up after ensuring the new xcframeworks work as expected.

@vimanyu vimanyu added tests-requested: quick Trigger a quick set of integration tests. and removed tests: failed This PR's integration tests failed. labels Jun 18, 2021
@github-actions 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 Jun 18, 2021
@github-actions
Copy link

github-actions bot commented Jun 18, 2021

❌  Integration test FAILED

Requested by @vimanyu on commit 192c53b
Last updated: Thu Jun 24 13:55 PDT 2021
View integration test log & download artifacts

Failures Configs
installations [TEST] [FAILURE] [Android] [macos] [android_latest]
(1 failed tests)  TestGettingTokenTwiceMatches

Copy link
Contributor

@sunmou99 sunmou99 left a comment

Choose a reason for hiding this comment

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

argparse is good. But I wasn't sure if you noticed Google's practice of Command line parsing

@vimanyu
Copy link
Contributor Author

vimanyu commented Jun 18, 2021

argparse is good. But I wasn't sure if you noticed Google's practice of Command line parsing

I personally feel the benefit of using a standard library module argparse for simple standalone scripts like this is that we do not have any unnecessary external dependencies.

@vimanyu vimanyu added tests-requested: quick Trigger a quick set of integration tests. and removed tests: failed This PR's integration tests failed. labels Jun 22, 2021
@github-actions github-actions bot added tests: in-progress This PR's integration tests are in progress. tests: failed This PR's integration tests failed. and removed tests-requested: quick Trigger a quick set of integration tests. labels Jun 22, 2021
@vimanyu vimanyu removed the tests: failed This PR's integration tests failed. label Jun 23, 2021
@vimanyu
Copy link
Contributor Author

vimanyu commented Jun 23, 2021

The iOS build workflow takes longer than before: https://github.com/firebase/firebase-cpp-sdk/actions/workflows/ios.yml
Originally, the builds for different architectures are in subprocesses.
Could you add similar features here with Thread?

Thanks for pointing this out. I did some quick tests to see what works best and I think multiprocessing is working great. For debugging purpose, threading/multiprocess makes it a little hard as the log output will be interleaved but we can look into it once it becomes a bigger issue.

Options (and local tests from macbook pro with 12 logical cores):
Comparison with current state of things (building everything serially): 29 minutes

  1. Use cmake --build -j <cpu_count>
    This options is great as just the build for a specific architecture is parallelized by cmake but it is slightly slower than multiprocessing.
    Time: 9m 32 s

  2. Multithread
    Cmake configure serially but cmake builds are done in parallel on threads.
    Time: 10m

  3. Multiprocess
    Cmake configure serially but cmake builds are done in parallel in multiple processes.
    Time: 7m 56s

Note:
Cmake configure in parallel was causing problems and it might be because of cocoapods installing in parallel.
Since github runners have only 3 cores, these times are different but in general multiprocessing is definitely faster.

@vimanyu vimanyu requested a review from sunmou99 June 23, 2021 19:55
Copy link
Contributor

@sunmou99 sunmou99 left a comment

Choose a reason for hiding this comment

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

What's the plan about tvos?
If add tvos to the packaging workflow, probably need to update the release note later.

@vimanyu
Copy link
Contributor Author

vimanyu commented Jun 23, 2021

What's the plan about tvos?
If add tvos to the packaging workflow, probably need to update the release note later.

Thinking more about it, I think I will revert changes to github workflow files for now. It definitely helped to test the packaging workflow and make sure nothing broke in the process but we should truly include tvos once all the work on simulators on github is done.

vimanyu added 3 commits June 23, 2021 16:26
Waiting for simulation tests to run on Github
before including tvos in build and packaging.
@vimanyu
Copy link
Contributor Author

vimanyu commented Jun 23, 2021

Have removed changes to workflow files from this PR as we will wait until the work on tvos simulator integration tests on Github is done.
Though, this script was put through a test in cpp-packaging workflow and worked,
https://github.com/firebase/firebase-cpp-sdk/actions/runs/965593738

@vimanyu vimanyu requested a review from sunmou99 June 23, 2021 23:57
@sunmou99 sunmou99 removed the tests: in-progress This PR's integration tests are in progress. label Jun 24, 2021
Copy link
Contributor

@sunmou99 sunmou99 left a comment

Choose a reason for hiding this comment

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

LGTM

@vimanyu vimanyu merged commit 192c53b into main Jun 24, 2021
@vimanyu vimanyu deleted the feature/tvos-ios-one-build-script branch June 24, 2021 18:41
@github-actions github-actions bot added tests: in-progress This PR's integration tests are in progress. tests: failed This PR's integration tests failed. labels Jun 24, 2021
@firebase-workflow-trigger firebase-workflow-trigger bot removed the tests: in-progress This PR's integration tests are in progress. label Jun 24, 2021
@firebase firebase locked and limited conversation to collaborators Jul 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes tests: failed This PR's integration tests failed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants