Skip to content
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

Seperate RN andriod and IOS into 2 separated Stages. #23400

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

jchen351
Copy link
Contributor

Description

Seperate RN andriod and IOS into 2 separated Stages.

Motivation and Context

Speed up the PR process.

                config.build_settings['EXCLUDED_ARCHS[sdk=iphonesimulator*]'] = "arm64"
# Conflicts:
#	js/react_native/e2e/ios/Podfile
#	js/react_native/ios/OnnxruntimeModule.xcodeproj/project.pbxproj
#	js/react_native/ios/Podfile
# Conflicts:
#	js/react_native/e2e/ios/Podfile
#	js/react_native/e2e/yarn.lock
#	js/react_native/ios/Podfile
#	tools/ci_build/github/azure-pipelines/templates/react-native-ci.yml
jobs:
- job: ReactNative_CI
- job: ReactNative_CI_Android
Copy link
Member

Choose a reason for hiding this comment

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

Could you use Linux machines to build Android?

Copy link
Member

Choose a reason for hiding this comment

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

They are more stable, and we have more capacities there.

Copy link
Member

Choose a reason for hiding this comment

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

If you do it in a new PR if you'd like.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@edgchen1 That is a question for Edward.

Copy link
Contributor

Choose a reason for hiding this comment

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

worth trying it. IIRC, we used to only be able to run the android emulator on Mac agents.

Copy link
Member

Choose a reason for hiding this comment

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

Now it runs well on Linux.

snnn
snnn previously approved these changes Jan 17, 2025
Copy link
Member

@snnn snnn left a comment

Choose a reason for hiding this comment

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

LGTM. Please also let the mobile team take a look.

targetPath: '$(Build.SourcesDirectory)/js/react_native/e2e/artifacts'
condition: succeededOrFailed()
displayName: Publish React Native Detox E2E test logs

- task: PublishPipelineArtifact@0
inputs:
artifactName: '${{parameters.PackageName}}'
artifactName: 'NPM_packages_android'
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the npm packaging pipeline sets the PackageName parameter.

- template: templates/react-native-ci.yml
parameters:
NpmPackagingMode: ${{ variables.NpmPackagingMode }}
BuildConfig: 'Release'
PoolName: 'onnxruntime-Ubuntu2204-AMD-CPU'
PackageName: 'onnxruntime-react-native'
InitialStageDependsOn: 'Precheck_and_extract_commit'
enable_code_sign: false

does it still work with these changes?

Copy link
Contributor Author

@jchen351 jchen351 Jan 17, 2025

Choose a reason for hiding this comment

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

I think the npm packaging pipeline sets the PackageName parameter.

- template: templates/react-native-ci.yml
parameters:
NpmPackagingMode: ${{ variables.NpmPackagingMode }}
BuildConfig: 'Release'
PoolName: 'onnxruntime-Ubuntu2204-AMD-CPU'
PackageName: 'onnxruntime-react-native'
InitialStageDependsOn: 'Precheck_and_extract_commit'
enable_code_sign: false

does it still work with these changes?

Good point, but the package 'onnxruntime-react-native' wasn't downloaded later. The default package name, NPM_packages, from RN_CI was used to get downloading artifacts.

Copy link
Contributor Author

@jchen351 jchen351 Jan 17, 2025

Choose a reason for hiding this comment

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

Both NPM_packages_android and NPM_packages_ios contians the same *.tgz. So, we just need to build one of it.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok. please verify that other pipelines which use the react-native-ci.yml template still work as expected.

skottmckay
skottmckay previously approved these changes Jan 17, 2025
@jchen351
Copy link
Contributor Author

Need to verify the NPM_packages with main branch from Nuget pkg pipeline which is currently broken.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants