Page MenuHomePhabricator
Differential D10339

[identity] Switch Identity unit tests to autoscaling CI runners
ClosedPublic
Actions

Authored by atul on Dec 14 2023, 10:24 AM.
Tags
None
Referenced Files
F6266995: D10339.id34863.diff
Fri, Apr 25, 11:05 AM
Unknown Object (File)
Fri, Apr 25, 6:54 AM
Unknown Object (File)
Fri, Apr 25, 5:54 AM
Unknown Object (File)
Fri, Apr 25, 4:13 AM
Unknown Object (File)
Fri, Apr 25, 1:55 AM
Unknown Object (File)
Thu, Apr 24, 11:49 PM
Unknown Object (File)
Thu, Apr 24, 10:20 PM
Unknown Object (File)
Thu, Apr 24, 10:12 PM
View All Files
Subscribers
None

Details

Reviewers
ashoat
ginsu
tomek
rohan
bartek
varun
Commits
rCOMM9d4decdf1064: [identity] Switch Identity unit tests to autoscaling CI runners
Summary

Pretty much identical to D10328 but for Identity.

Context: https://linear.app/comm/issue/ENG-5770/office-mba-runner-out-of-storage#comment-d61c38e9

Test Plan

See if CI passes

40743c.png (2×2 px, 543 KB)

Diff Detail

Repository
rCOMM Comm
Branch
arcpatch-D10339
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

atul updated this revision to Diff 34647.Dec 14 2023, 10:24 AM
atul created this revision.

after updating herald rule

Harbormaster returned this revision to the author for changes because remote builds failed.Dec 14 2023, 10:28 AM
Harbormaster failed remote builds in B25080: Diff 34647!
atul updated this revision to Diff 34648.Dec 14 2023, 10:30 AM

fix indent

atul mentioned this in D10340: [tunnelbroker] Switch Tunnelbroker unit tests to autoscaling CI runners.Dec 14 2023, 10:35 AM
atul published this revision for review.Dec 14 2023, 10:36 AM
atul edited the test plan for this revision. (Show Details)
atul mentioned this in D10341: [emscripten] Switch Emscripten CI job to autoscaling runners.Dec 14 2023, 10:44 AM
Harbormaster completed remote builds in B25079: Diff 34646.Dec 14 2023, 10:46 AM
Harbormaster failed remote builds in B25081: Diff 34648!Dec 14 2023, 11:11 AM
varun requested changes to this revision.Dec 14 2023, 12:13 PM
varun added inline comments.
.buildkite/identity_tests.yml
4 ↗(On Diff #34648)

do we really need to install any of this?

5 ↗(On Diff #34648)

i don't think we need protobuf to run unit tests

This revision now requires changes to proceed.Dec 14 2023, 12:13 PM
Harbormaster completed remote builds in B25081: Diff 34648.Dec 14 2023, 1:02 PM
varun added inline comments.Dec 14 2023, 10:17 PM
.buildkite/identity_tests.yml
5 ↗(On Diff #34648)

we might need this actually... not entirely sure

bartek added inline comments.Dec 15 2023, 3:07 AM
.buildkite/identity_tests.yml
5 ↗(On Diff #34648)

we might need this actually... not entirely sure

Indeed https://phab.comm.dev/D10340#299698

atul mentioned this in rCOMM1a7b52ca376b: [tunnelbroker] Switch Tunnelbroker unit tests to autoscaling CI runners.Dec 19 2023, 12:15 AM
atul requested review of this revision.Dec 19 2023, 12:50 PM

We need all the apt install and install_protobuf stuff

varun accepted this revision.Dec 19 2023, 12:55 PM
In D10339#301015, @atul wrote:

We need all the apt install and install_protobuf stuff

would you mind explaining why?

This revision is now accepted and ready to land.Dec 19 2023, 12:55 PM
atul updated this revision to Diff 34862.Dec 19 2023, 1:19 PM

without apt install

Harbormaster failed remote builds in B25234: Diff 34862!Dec 19 2023, 1:23 PM
atul updated this revision to Diff 34863.Dec 19 2023, 1:23 PM

back to working and land

atul added a comment.Dec 19 2023, 1:23 PM
In D10339#301021, @varun wrote:
In D10339#301015, @atul wrote:

We need all the apt install and install_protobuf stuff

would you mind explaining why?

Basically what @bartek explained in Tunnelbroker diff. Building requires prost which requires protoc. Here's what we get without:

f82a0c.png (898×2 px, 271 KB)

varun added a comment.Dec 19 2023, 1:25 PM

sorry, to clarify, i understand why we need protobuf. but i didn’t understand why we need all the apt install stuff. guessing they’re all dependencies of the install script?

atul added a comment.Dec 19 2023, 1:30 PM
In D10339#301040, @varun wrote:

sorry, to clarify, i understand why we need protobuf. but i didn’t understand why we need all the apt install stuff. guessing they’re all dependencies of the install script?

Yeah they're necessary for install_protobuf which is necessary because protoc from apt-get is ancient and I think we're preferring building from source instead of grabbing binary from GitHub releases?

varun added a comment.Dec 19 2023, 1:31 PM

got it, thanks for explaining!

This revision was landed with ongoing or failed builds.Dec 19 2023, 1:39 PM
Closed by commit rCOMM9d4decdf1064: [identity] Switch Identity unit tests to autoscaling CI runners (authored by atul). · Explain Why
This revision was automatically updated to reflect the committed changes.
atul added a commit: rCOMM9d4decdf1064: [identity] Switch Identity unit tests to autoscaling CI runners.
Harbormaster completed remote builds in B25235: Diff 34863.Dec 19 2023, 2:01 PM
atul mentioned this in rCOMM1c776334d2ce: [emscripten] Switch Emscripten CI job to autoscaling runners.Dec 20 2023, 2:53 PM

Revision Contents
Changeset List

PathSize
.buildkite/
2 lines
12 lines

Diff 34862

View Options

.buildkite/blob_unittests.yml

require File.join(File.dirname(`node --print "require.resolve('expo/package.json')"`), "scripts/autolinking") require File.join(File.dirname(`node --print "require.resolve('expo/package.json')"`), "scripts/autolinking")
require File.join(File.dirname(`node --print "require.resolve('react-native/package.json')"`), "scripts/react_native_pods") require File.join(File.dirname(`node --print "require.resolve('react-native/package.json')"`), "scripts/react_native_pods")
require File.join(File.dirname(`node --print "require.resolve('@react-native-community/cli-platform-ios/package.json')"`), "native_modules") require File.join(File.dirname(`node --print "require.resolve('@react-native-community/cli-platform-ios/package.json')"`), "native_modules")
platform :ios, '12.0' platform :ios, '12.0'
require 'json' require 'json'
podfile_properties = JSON.parse(File.read('./Podfile.properties.json')) rescue {}
target 'Comm' do def common_comm_target_pods
pod 'ReactNativeKeyboardTrackingView', :path => '../../node_modules/react-native-keyboard-tracking-view'
pod 'ReactNativeKeyboardInput', :path => '../../node_modules/react-native-keyboard-input'
pod 'react-native-ffmpeg/min-lts', :podspec => '../../node_modules/react-native-ffmpeg/react-native-ffmpeg.podspec'
pod 'react-native-video/VideoCaching', :podspec => '../../node_modules/react-native-video/react-native-video.podspec'
pod 'SQLCipher-Amalgamation', :path => '../../node_modules/@commapp/sqlcipher-amalgamation' pod 'SQLCipher-Amalgamation', :path => '../../node_modules/@commapp/sqlcipher-amalgamation'
pod 'gRPC-C++', :podspec => './pod-patch/.patched/gRPC-C++/1.40.0/gRPC-C++.podspec.json' pod 'gRPC-C++', :podspec => './pod-patch/.patched/gRPC-C++/1.40.0/gRPC-C++.podspec.json'
pod 'gRPC-C++/Protobuf', :podspec => './pod-patch/.patched/gRPC-C++/1.40.0/gRPC-C++.podspec.json' pod 'gRPC-C++/Protobuf', :podspec => './pod-patch/.patched/gRPC-C++/1.40.0/gRPC-C++.podspec.json'
pod 'gRPC-Core', :podspec => './pod-patch/.patched/gRPC-Core/1.40.0/gRPC-Core.podspec.json' pod 'gRPC-Core', :podspec => './pod-patch/.patched/gRPC-Core/1.40.0/gRPC-Core.podspec.json'
pod 'Protobuf-C++', '3.15.8' pod 'Protobuf-C++', '3.15.8'
end
target 'Comm' do
pod 'ReactNativeKeyboardTrackingView', :path => '../../node_modules/react-native-keyboard-tracking-view'
pod 'ReactNativeKeyboardInput', :path => '../../node_modules/react-native-keyboard-input'
pod 'react-native-video/VideoCaching', :podspec => '../../node_modules/react-native-video/react-native-video.podspec'
pod 'react-native-ffmpeg/min-lts', :podspec => '../../node_modules/react-native-ffmpeg/react-native-ffmpeg.podspec'
common_comm_target_pods
use_expo_modules! use_expo_modules!
config = use_native_modules! config = use_native_modules!
podfile_properties = JSON.parse(File.read('./Podfile.properties.json')) rescue {}
use_react_native!( use_react_native!(
:path => config[:reactNativePath], :path => config[:reactNativePath],
# to enable hermes on iOS, change `false` to `true` and then install pods # to enable hermes on iOS, change `false` to `true` and then install pods
:hermes_enabled => podfile_properties['expo.jsEngine'] == 'hermes' :hermes_enabled => podfile_properties['expo.jsEngine'] == 'hermes'
) )
# Enables Flipper. # Enables Flipper.
# #
# Note that if you have use_frameworks! enabled, Flipper will not work and # Note that if you have use_frameworks! enabled, Flipper will not work and
# you should disable these next few lines. # you should disable these next few lines.
use_flipper!({ "Flipper-DoubleConversion" => "1.1.7" }) use_flipper!({ "Flipper-DoubleConversion" => "1.1.7" })
end
target 'NotificationService' do
common_comm_target_pods
pod 'OLMKit', :path => "../../node_modules/olm"
pod 'RCT-Folly', :podspec => "../../node_modules/react-native/third-party-podspecs/RCT-Folly.podspec"
# Lines below are only needed to compile and use Expo Secure Store in Notification Service.
# If Apple disapproves 'APPLICATION_EXTENSION_API_ONLY' flag then we can safely delete them and use
# customized Expo Secure Store that does not depend on ExpoModulesCore
podfile_properties = JSON.parse(File.read('./Podfile.properties.json')) rescue {}
use_expo_modules!
use_react_native!(
:path => "../../node_modules/react-native",
:hermes_enabled => podfile_properties['expo.jsEngine'] == 'hermes'
)
end
post_install do |installer| post_install do |installer|
ashoatUnsubmitted
Not Done
Inline Actions

Does this apply to all targets now? Are we sure we want to apply it to all targets? Is it possibly that we're inadvertedly applying it to some targets we haven't considered? (Targets that aren't the Comm app, or the notification service extension)

ashoat: Does this apply to all targets now? Are we sure we want to apply it to all targets? Is it…
marcinAuthorUnsubmitted
Done
Inline Actions

There are two changes here compared to master branch:

  1. post_install hook is moved from target 'Comm' scope to global Podfile scope
  2. some lines are added that disable APPLICATION_EXTENSION_API_ONLY flag to enable CommonCpp sources compilation in 'NotificationService' target.

Let me explain why those changes should not raise concern.
Ad. 1) According to this doc: https://www.rubydoc.info/github/CocoaPods/Core/Pod%2FPodfile%2FDSL:post_install post_install is a hook applied just before XCode project is written to disk. According to this doc: https://www.rubydoc.info/gems/cocoapods-core/0.34.1/Pod/Podfile/DSL Podfile hooks always exist in global scope. Even if this post_install hook was written inside target 'Comm' scope its behaviour would be the same as if it was in global scope. This is even more confirmed by the fact that an attempt to write multiple post_install hooks (e.g. each per target) leads to errors: https://github.com/CocoaPods/CocoaPods/issues/6172.
Ad. 2) Now that we established that we cannot have multiple post_install hooks and the one that we have should be in global scope we can admit it is impossible to use this flag anywhere else.

marcin: There are two changes here compared to master branch: 1) post_install hook is moved from target…
ashoatUnsubmitted
Not Done
Inline Actions

Thanks for the explanation!

ashoat: Thanks for the explanation!
react_native_post_install(installer) react_native_post_install(installer)
__apply_Xcode_12_5_M1_post_install_workaround(installer) __apply_Xcode_12_5_M1_post_install_workaround(installer)
# Excluding arm64 because M1 simulator isn't supported yet # Excluding arm64 because M1 simulator isn't supported yet
# Excluding 32-bit x86 because nobody uses it and it causes compilation issues # Excluding 32-bit x86 because nobody uses it and it causes compilation issues
installer.pods_project.build_configurations.each do |config| installer.pods_project.build_configurations.each do |config|
config.build_settings["EXCLUDED_ARCHS[sdk=iphonesimulator*]"] = "arm64 i386" config.build_settings["EXCLUDED_ARCHS[sdk=iphonesimulator*]"] = "arm64 i386"
ashoatUnsubmitted
Not Done
Inline Actions

It's a bit confusing to me that we exclude both of these here, but in the project.pbxproj only arm64 is mentioned. Do you know why that is?

ashoat: It's a bit confusing to me that we exclude both of these here, but in the `project.pbxproj`…
marcinAuthorUnsubmitted
Done
Inline Actions

Build settings in the XCode project refer to targets: Comm and NotificationService. This line changes build settings for pods. I did not modify this line and according to the explanation above it has alway been that we exclude both architectures for all pods. I do not know why it is that, but it is not related to any change introduced in this diff. Target 'Comm' excluded only amr64 architecture before I started to work on this issue and the very recent changes I introduced were to unify excluded architectures for both Comm and NotificationService. Would you like me to try whether we can exclude i386 arch for XCode targets as well?

marcin: Build settings in the XCode project refer to targets: Comm and NotificationService. This line…
ashoatUnsubmitted
Not Done
Inline Actions

No, that's alright – thanks for this explanation

ashoat: No, that's alright – thanks for this explanation
config.build_settings['OTHER_CPLUSPLUSFLAGS'] = '-DDONT_AUTOINSTALL_REANIMATED' config.build_settings['OTHER_CPLUSPLUSFLAGS'] = '-DDONT_AUTOINSTALL_REANIMATED'
end end
# Lines below are only needed to compile and use Expo Secure Store in Notification Service.
# If Apple disapproves 'APPLICATION_EXTENSION_API_ONLY' flag then we can safely delete them and use
# customized Expo Secure Store that does not depend on ExpoModulesCore
installer.pods_project.targets.each do |target|
target.build_configurations.each do |config|
config.build_settings['APPLICATION_EXTENSION_API_ONLY'] = 'NO'
end
end end
ashoatUnsubmitted
Not Done
Inline Actions

There's an extraneous space at the end of this line

ashoat: There's an extraneous space at the end of this line
end end
View Options

.buildkite/identity_tests.yml

require File.join(File.dirname(`node --print "require.resolve('expo/package.json')"`), "scripts/autolinking") require File.join(File.dirname(`node --print "require.resolve('expo/package.json')"`), "scripts/autolinking")
require File.join(File.dirname(`node --print "require.resolve('react-native/package.json')"`), "scripts/react_native_pods") require File.join(File.dirname(`node --print "require.resolve('react-native/package.json')"`), "scripts/react_native_pods")
require File.join(File.dirname(`node --print "require.resolve('@react-native-community/cli-platform-ios/package.json')"`), "native_modules") require File.join(File.dirname(`node --print "require.resolve('@react-native-community/cli-platform-ios/package.json')"`), "native_modules")
platform :ios, '12.0' platform :ios, '12.0'
require 'json' require 'json'
podfile_properties = JSON.parse(File.read('./Podfile.properties.json')) rescue {}
target 'Comm' do def common_comm_target_pods
pod 'ReactNativeKeyboardTrackingView', :path => '../../node_modules/react-native-keyboard-tracking-view'
pod 'ReactNativeKeyboardInput', :path => '../../node_modules/react-native-keyboard-input'
pod 'react-native-ffmpeg/min-lts', :podspec => '../../node_modules/react-native-ffmpeg/react-native-ffmpeg.podspec'
pod 'react-native-video/VideoCaching', :podspec => '../../node_modules/react-native-video/react-native-video.podspec'
pod 'SQLCipher-Amalgamation', :path => '../../node_modules/@commapp/sqlcipher-amalgamation' pod 'SQLCipher-Amalgamation', :path => '../../node_modules/@commapp/sqlcipher-amalgamation'
pod 'gRPC-C++', :podspec => './pod-patch/.patched/gRPC-C++/1.40.0/gRPC-C++.podspec.json' pod 'gRPC-C++', :podspec => './pod-patch/.patched/gRPC-C++/1.40.0/gRPC-C++.podspec.json'
pod 'gRPC-C++/Protobuf', :podspec => './pod-patch/.patched/gRPC-C++/1.40.0/gRPC-C++.podspec.json' pod 'gRPC-C++/Protobuf', :podspec => './pod-patch/.patched/gRPC-C++/1.40.0/gRPC-C++.podspec.json'
pod 'gRPC-Core', :podspec => './pod-patch/.patched/gRPC-Core/1.40.0/gRPC-Core.podspec.json' pod 'gRPC-Core', :podspec => './pod-patch/.patched/gRPC-Core/1.40.0/gRPC-Core.podspec.json'
pod 'Protobuf-C++', '3.15.8' pod 'Protobuf-C++', '3.15.8'
end
target 'Comm' do
pod 'ReactNativeKeyboardTrackingView', :path => '../../node_modules/react-native-keyboard-tracking-view'
pod 'ReactNativeKeyboardInput', :path => '../../node_modules/react-native-keyboard-input'
pod 'react-native-video/VideoCaching', :podspec => '../../node_modules/react-native-video/react-native-video.podspec'
pod 'react-native-ffmpeg/min-lts', :podspec => '../../node_modules/react-native-ffmpeg/react-native-ffmpeg.podspec'
common_comm_target_pods
use_expo_modules! use_expo_modules!
config = use_native_modules! config = use_native_modules!
podfile_properties = JSON.parse(File.read('./Podfile.properties.json')) rescue {}
use_react_native!( use_react_native!(
:path => config[:reactNativePath], :path => config[:reactNativePath],
# to enable hermes on iOS, change `false` to `true` and then install pods # to enable hermes on iOS, change `false` to `true` and then install pods
:hermes_enabled => podfile_properties['expo.jsEngine'] == 'hermes' :hermes_enabled => podfile_properties['expo.jsEngine'] == 'hermes'
) )
# Enables Flipper. # Enables Flipper.
# #
# Note that if you have use_frameworks! enabled, Flipper will not work and # Note that if you have use_frameworks! enabled, Flipper will not work and
# you should disable these next few lines. # you should disable these next few lines.
use_flipper!({ "Flipper-DoubleConversion" => "1.1.7" }) use_flipper!({ "Flipper-DoubleConversion" => "1.1.7" })
end
target 'NotificationService' do
common_comm_target_pods
pod 'OLMKit', :path => "../../node_modules/olm"
pod 'RCT-Folly', :podspec => "../../node_modules/react-native/third-party-podspecs/RCT-Folly.podspec"
# Lines below are only needed to compile and use Expo Secure Store in Notification Service.
# If Apple disapproves 'APPLICATION_EXTENSION_API_ONLY' flag then we can safely delete them and use
# customized Expo Secure Store that does not depend on ExpoModulesCore
podfile_properties = JSON.parse(File.read('./Podfile.properties.json')) rescue {}
use_expo_modules!
use_react_native!(
:path => "../../node_modules/react-native",
:hermes_enabled => podfile_properties['expo.jsEngine'] == 'hermes'
)
end
post_install do |installer| post_install do |installer|
ashoatUnsubmitted
Not Done
Inline Actions

Does this apply to all targets now? Are we sure we want to apply it to all targets? Is it possibly that we're inadvertedly applying it to some targets we haven't considered? (Targets that aren't the Comm app, or the notification service extension)

ashoat: Does this apply to all targets now? Are we sure we want to apply it to all targets? Is it…
marcinAuthorUnsubmitted
Done
Inline Actions

There are two changes here compared to master branch:

  1. post_install hook is moved from target 'Comm' scope to global Podfile scope
  2. some lines are added that disable APPLICATION_EXTENSION_API_ONLY flag to enable CommonCpp sources compilation in 'NotificationService' target.

Let me explain why those changes should not raise concern.
Ad. 1) According to this doc: https://www.rubydoc.info/github/CocoaPods/Core/Pod%2FPodfile%2FDSL:post_install post_install is a hook applied just before XCode project is written to disk. According to this doc: https://www.rubydoc.info/gems/cocoapods-core/0.34.1/Pod/Podfile/DSL Podfile hooks always exist in global scope. Even if this post_install hook was written inside target 'Comm' scope its behaviour would be the same as if it was in global scope. This is even more confirmed by the fact that an attempt to write multiple post_install hooks (e.g. each per target) leads to errors: https://github.com/CocoaPods/CocoaPods/issues/6172.
Ad. 2) Now that we established that we cannot have multiple post_install hooks and the one that we have should be in global scope we can admit it is impossible to use this flag anywhere else.

marcin: There are two changes here compared to master branch: 1) post_install hook is moved from target…
ashoatUnsubmitted
Not Done
Inline Actions

Thanks for the explanation!

ashoat: Thanks for the explanation!
react_native_post_install(installer) react_native_post_install(installer)
__apply_Xcode_12_5_M1_post_install_workaround(installer) __apply_Xcode_12_5_M1_post_install_workaround(installer)
# Excluding arm64 because M1 simulator isn't supported yet # Excluding arm64 because M1 simulator isn't supported yet
# Excluding 32-bit x86 because nobody uses it and it causes compilation issues # Excluding 32-bit x86 because nobody uses it and it causes compilation issues
installer.pods_project.build_configurations.each do |config| installer.pods_project.build_configurations.each do |config|
config.build_settings["EXCLUDED_ARCHS[sdk=iphonesimulator*]"] = "arm64 i386" config.build_settings["EXCLUDED_ARCHS[sdk=iphonesimulator*]"] = "arm64 i386"
ashoatUnsubmitted
Not Done
Inline Actions

It's a bit confusing to me that we exclude both of these here, but in the project.pbxproj only arm64 is mentioned. Do you know why that is?

ashoat: It's a bit confusing to me that we exclude both of these here, but in the `project.pbxproj`…
marcinAuthorUnsubmitted
Done
Inline Actions

Build settings in the XCode project refer to targets: Comm and NotificationService. This line changes build settings for pods. I did not modify this line and according to the explanation above it has alway been that we exclude both architectures for all pods. I do not know why it is that, but it is not related to any change introduced in this diff. Target 'Comm' excluded only amr64 architecture before I started to work on this issue and the very recent changes I introduced were to unify excluded architectures for both Comm and NotificationService. Would you like me to try whether we can exclude i386 arch for XCode targets as well?

marcin: Build settings in the XCode project refer to targets: Comm and NotificationService. This line…
ashoatUnsubmitted
Not Done
Inline Actions

No, that's alright – thanks for this explanation

ashoat: No, that's alright – thanks for this explanation
config.build_settings['OTHER_CPLUSPLUSFLAGS'] = '-DDONT_AUTOINSTALL_REANIMATED' config.build_settings['OTHER_CPLUSPLUSFLAGS'] = '-DDONT_AUTOINSTALL_REANIMATED'
end end
# Lines below are only needed to compile and use Expo Secure Store in Notification Service.
# If Apple disapproves 'APPLICATION_EXTENSION_API_ONLY' flag then we can safely delete them and use
# customized Expo Secure Store that does not depend on ExpoModulesCore
installer.pods_project.targets.each do |target|
target.build_configurations.each do |config|
config.build_settings['APPLICATION_EXTENSION_API_ONLY'] = 'NO'
end
end end
ashoatUnsubmitted
Not Done
Inline Actions

There's an extraneous space at the end of this line

ashoat: There's an extraneous space at the end of this line
end end