Skip to content

Add compilation cache in react native CI#15329

Merged
mszhanyi merged 6 commits into
mainfrom
zhanyi/reactcache
Apr 6, 2023
Merged

Add compilation cache in react native CI#15329
mszhanyi merged 6 commits into
mainfrom
zhanyi/reactcache

Conversation

@mszhanyi
Copy link
Copy Markdown
Contributor

@mszhanyi mszhanyi commented Apr 3, 2023

Description

  1. Replacing jobs with stages for better debugging and maintainance
  2. Added compilation cache to accelerate the workflow.
  3. Splited building protobuf and major code as 2 tasks

Motivation and Context

Reduced compilation time about one hour.
test run:
https://dev.azure.com/onnxruntime/onnxruntime/_build/results?buildId=943695&view=logs&j=de302ec2-2305-57e0-e8c6-cd89c569f2a3&t=8b360243-7783-51da-8079-2304089d3d1d

@mszhanyi mszhanyi requested a review from a team April 3, 2023 01:25
@mszhanyi mszhanyi marked this pull request as draft April 3, 2023 01:25
@mszhanyi mszhanyi marked this pull request as ready for review April 3, 2023 03:53
@mszhanyi mszhanyi requested review from edgchen1 and skottmckay April 3, 2023 04:43
- name: CACHE_DIR
type: string

- name: ChangeEveryCommit
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: could the parameter names have a uniform casing convention? e.g., either ParameterName or PARAMETER_NAME

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks. changed CACHE_DIR to CacheDir

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

cool. how about TODAY?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated in #15440

- script: |
brew install ccache
which ccache || brew install ccache
echo "##vso[task.prependpath]/usr/local/opt/ccache/libexec"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

is it possible that ccache is installed in a different directory? with the which ccache call above, it seems like it could be.
if it is, would the path /usr/local/opt/ccache/libexec still be the right one?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

In CI, it's unlikely.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

so if there is an existing ccache, we assume it is a brew installation of ccache.
maybe something like brew list ccache &> /dev/null || brew install ccache above then?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It‘s a big risk that there's any unknown installation in build machine.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It‘s a big risk that there's any unknown installation in build machine.

sorry, I didn't quite understand this point. could you please elaborate?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Unknow installation pose security risk.
https://en.wikipedia.org/wiki/2020_United_States_federal_government_data_breach
On May 12, 2021, President Biden issued an [Executive Order on Improving the Nations Cybersecurity](https://www.whitehouse.gov/briefing-room/presidential-actions/2021/05/12/executive-order-on-improving-the-nations-cybersecurity/).  It put compliance requirements on build system and software development lifecycle of the software provided to the U.S. government. At Microsoft, all customer facing cloud services and all the latest released versions of our on-prem products will need to conform with the Executive Order.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I got the part that unknown installations are a security risk. I didn't understand how it's being mitigated. and aren't we using hosted Mac agents which have known installed software?

Comment thread tools/ci_build/github/azure-pipelines/templates/react-native-ci.yml
@mszhanyi mszhanyi requested a review from edgchen1 April 4, 2023 14:50
@mszhanyi mszhanyi merged commit 962d8d2 into main Apr 6, 2023
@mszhanyi mszhanyi deleted the zhanyi/reactcache branch April 6, 2023 02:39
snnn pushed a commit that referenced this pull request Apr 8, 2023
### Description
It seems like #15329
re-worked some jobs in `react-native-ci.yml` into stages. When this
template is used from within `npm-packaging-pipeline.yml`, there is
problem in that there is a stage that contains multiple stages as jobs.
Per my understanding, this is not acceptable to Azure DevOps. So,
re-working some portion of `npm-packaging-pipeline.yml` to accomadate
changes in #15329

### Motivation and Context
Fix NPM packaging pipeline
Validating test run with fix:
https://aiinfra.visualstudio.com/Lotus/_build/results?buildId=297391&view=results
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.

3 participants