-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
[PERF][MAUI] Add iOS Startup Scenario (Runtime side) #67670
[PERF][MAUI] Add iOS Startup Scenario (Runtime side) #67670
Conversation
Tagging subscribers to this area: @hoyosjs Issue DetailsThis adds tests and flows to run the iOS Startup scenario for Maui. The majority of the work completed revolves around moving the Maui iOS flow from running on Windows (we only had Size on Disk) to running on a MacOS device connected to an iPhone. It also included shifting the app run test to use the xHarnessAppBundleToTest Task. More specifically, the changes included:
TODO:
|
@@ -268,14 +293,18 @@ if [[ "$run_from_perf_repo" == true ]]; then | |||
performance_directory=$workitem_directory | |||
setup_arguments="--perf-hash $commit_sha $common_setup_arguments" | |||
else | |||
git clone --branch main --depth 1 --quiet https://github.com/dotnet/performance.git $performance_directory | |||
git clone --branch iosstartup --depth 1 https://github.com/akoeplinger/performance.git $performance_directory |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just curious, how long are we going to depend on @akoeplinger 's fork here? can we get this into dotnet/performance?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hopefully we won't depend on it at all. The matching PR is up, I just have not linked it yet. dotnet/performance#2355 The PR should be merged and then the above code changed back before final merge. I have a mini TODO list at the top with that as one of the items.
- AdditionalHelixPreCommands: 'export ORIGPYPATH=$PYTHONPATH;export CRYPTOGRAPHY_ALLOW_OPENSSL_102=true;sudo apt-get -y install python3-venv;python3 -m venv $HELIX_WORKITEM_PAYLOAD/.venv;source $HELIX_WORKITEM_PAYLOAD/.venv/bin/activate;export PYTHONPATH=;python3 -m pip install -U pip;pip3 install --user azure.storage.blob==12.0.0 --force-reinstall;pip3 install --user azure.storage.queue==12.0.0 --force-reinstall;export PERFLAB_UPLOAD_TOKEN="$(PerfCommandUploadTokenLinux)"' | ||
- AdditionalHelixPostCommands: 'export PYTHONPATH=$ORIGPYPATH' | ||
- IsInternal: --internal | ||
- ${{ if and(ne(parameters.osGroup, 'windows'), eq(parameters.osSubGroup, '_musl')) }}: | ||
- AdditionalHelixPreCommands: 'export ORIGPYPATH=$PYTHONPATH;sudo apk add py3-virtualenv;python3 -m venv $HELIX_WORKITEM_PAYLOAD/.venv;source $HELIX_WORKITEM_PAYLOAD/.venv/bin/activate;export PYTHONPATH=;python3 -m pip install -U pip;pip3 install --user azure.storage.blob==12.0.0 --force-reinstall;pip3 install --user azure.storage.queue==12.0.0 --force-reinstall;export PERFLAB_UPLOAD_TOKEN="$(PerfCommandUploadTokenLinux)"' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the drops to apk add py3-virtualenv
and the --user
drops here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The dropping of apk is due to Mac not having apk as a provider based on my testing and what I found. This is alright in this case, and should be investigated for the linux case, because newer verisons of python come with the venv module by default. for the --user that was causing errors once the venv was being properly used. I haven't had a chance to look at this in the other scenarios yet. Issue with slightly more detail: dotnet/performance#2356
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But these are the commands that should be only executed on our Alpine machines. The eq(parameters.osSubGroup, '_musl')
should be ensuring that. Are you saying that the pre and post commands in this block are being executed on OSX devices?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case, the Alpine machine use apk add and the non-musl machines uses apt-get, neither of which are on the OSX devices.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are not getting executed on the OSX device. I see what the original comment was about now, pushing a fix to re-add the lines to this group.
displayName: Build Startup tool (MAC) | ||
env: | ||
PERFLAB_TARGET_FRAMEWORKS: net6.0 | ||
condition: and(succeeded(), and(ne(variables['Agent.Os'], 'Windows_NT'), eq(variables['Agent.Os'], 'Darwin'))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this say "not equals Windows and equals Darwin"? Why not just "equals Darwin"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed, I agree it should just be Darwin, must have been blind to it.
displayName: Build SizeOnDisk tool (MAC) | ||
env: | ||
PERFLAB_TARGET_FRAMEWORKS: net6.0 | ||
condition: and(succeeded(), and(ne(variables['Agent.Os'], 'Windows_NT'), eq(variables['Agent.Os'], 'Darwin'))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same question here - can't this just be "equals Darwin"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above
@@ -178,6 +178,9 @@ steps: | |||
cp MauiTesting.csproj MauiTesting.csproj.bak | |||
sed -i'' -e 's/net6.0-ios;net6.0-maccatalyst/net6.0-ios/g' MauiTesting.csproj | |||
|
|||
# Test xharness with the app id set to net.dot.mauitesting. This is necessary for the iOS on device signing | |||
sed -i'' -e 's/com.companyname.mauitesting/net.dot.mauitesting/g' MauiTesting.csproj |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of using sed
I think in this case we could override the ID on the msbuild command line via /p:ApplicationId=net.dot.mauitesting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am assuming you are talking about during the dotnet publish step? Let me try that out.
<MicrosoftDotNetXHarnessCLIVersion>1.0.0-prerelease.22206.1</MicrosoftDotNetXHarnessCLIVersion> | ||
<XharnessPath>$(HELIX_CORRELATION_PAYLOAD)\microsoft.dotnet.xharness.cli\$(MicrosoftDotNetXHarnessCLIVersion)\tools\net6.0\any\Microsoft.DotNet.XHarness.CLI.dll</XharnessPath> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this can be removed now, the xharness in dotnet/runtime was updated
cp -r $HELIX_CORRELATION_PAYLOAD/MauiTesting.app $(ScenarioDirectory)mauiios/MauiTesting.app | ||
cp -f embedded.mobileprovision $(ScenarioDirectory)mauiios/MauiTesting.app | ||
cd $(ScenarioDirectory)mauiios | ||
sign MauiTesting.app |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should move the signing logic into pre.py
, right now that invocation doesn't do anything for this scenario and it would be better encapsulated there.
@@ -127,7 +131,12 @@ jobs: | |||
displayName: Build Startup tool (Linux) | |||
env: | |||
PERFLAB_TARGET_FRAMEWORKS: net6.0 | |||
condition: and(succeeded(), ne(variables['Agent.Os'], 'Windows_NT')) | |||
condition: and(succeeded(), and(ne(variables['Agent.Os'], 'Windows_NT'), ne(variables['Agent.Os'], 'Darwin'))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't fully remember if the Agent.Os
string is consistent for Linux, but if it is we should look into changing these conditions from negative to positive, since we now have a third OS to choose from.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per https://docs.microsoft.com/en-us/azure/devops/pipelines/build/variables?view=azure-devops&tabs=yaml#agent-variables-devops-services, it looks like the three Agent.OSs are Windows_NT, Darwin, and Linux. Updated this where there is a difference between each of the 3.
- IsInternal: -Internal | ||
- ${{ if and(ne(parameters.osGroup, 'windows'), ne(parameters.osSubGroup, '_musl')) }}: | ||
- IsInternal: -Internal | ||
- ${{ if and(ne(parameters.osGroup, 'windows'), ne(parameters.osGroup, 'OSX'), ne(parameters.osSubGroup, '_musl')) }}: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It probably also makes sense to do the same thing here as below with Agent.Os
, and move this to more positive conditions. Should make the blocks easier to read and modify going forward as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this is set in the YAML and not necessarily only windows/linux/osx. I will make another PR for going through and checking these.
@@ -178,11 +178,12 @@ steps: | |||
cp MauiTesting.csproj MauiTesting.csproj.bak | |||
sed -i'' -e 's/net6.0-ios;net6.0-maccatalyst/net6.0-ios/g' MauiTesting.csproj | |||
|
|||
../dotnet publish -bl:MauiiOS.binlog -f net6.0-ios --self-contained -r ios-arm64 -c Release /p:_RequireCodeSigning=false | |||
../dotnet publish -bl:MauiiOS.binlog -f net6.0-ios --self-contained -r ios-arm64 -c Release /p:_RequireCodeSigning=false /p:ApplicationId=net.dot.mauitesting | |||
ls -aR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these going to stay around, or were they just for testing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Testing the above mentioned ApplicationId and to add the podcast app
c3e5ffd
to
55cbac5
Compare
Added the xharnessappbundletotest item for first pass test. Remove iosmono build dependency temporarily. Add missing directory in scenarios.proj include. Don't use --user for pip installs in venv. Added back in linux extra private job settings, and fixed added and() yaml to use a single and() as they can take more than 2 inputs. Fix .proj -- in comment issue. Try cding into the HELIX_CORRELATION_PAYLOAD directory. Add back in SOD testing to ensure proper working, and try a zip directory for inclusion. Set shared helix precommand to original since specific zipping style results in what we want, added some more to the CustomCommands for the Xharness running, added the app file to the payload directory. Fix incorrect unzip directory. Remove SOD condition so it actually runs. Check directory after signing and copy app to work item root for another run attempt. Fix the MauiiOSDefault.ipa copy. Add echos of the target and the command to see if there is something obvious going wrong. Fix(?) MauiiOSDefault.ipa file path. Test the iosstartup performance branch. Ensure that XHarnessCLI is included and print the directories. Add XharnessPath as an Env variable for the test.py runs, and remove middle testing stuff. Hardcode the Xharness version in the .proj file, remove some testing ls/pwd stuff, update perf scenario name, and add a command line xharness test. Set the xharness path manually like done for android. Turn on echo, export instead of set XHARNESSPATH. Try setting XHarnessPath to XHARNESS_CLI_PATH for the perf repo to catch, and remove the recursive file print as it shouldn't be needed at least for a while. Test other env variables to see if they are being passed in at all. Use Env variables properly(?). try using the pub dir and the proper folder name for testing. Try setting ios app id to net.dot.mauitesting and fix a spacing issue. Add another ls on the starting folder, and try renaming the workitem name. copy the mobileprovision and sign in the starting directory and copy the .app file to the scenario dir. Also cleaned up some of the printing. Update xharness version. Add chmod for the startup directory and print post pre.py files for testing. Reenable runtime build. Added maui version and iosllvmbuild specifiers for the perf-setup.sh file. Reenabled the mono iOS steps for full integration testing with both. Also setup some testing for precommands. Missed adding the ios app build. Fix additionalSetupParameters setup. Cleanup, minor perf setup updates to hopefully fix long running setup, and added print lines to the setup to further help with testing. Missing space for bash if statement. Add parenths around a mauiVersion as it doesn't seem to be set properly at runtime. linux Variable replacement needs parenths. Change casing of iosLlvmBuild to match .sh assumptions, and print out all available files in the source_directory. Add condition to work items for llvm vs nollvm, hopefully fixed file copies. Fix iosLlvmBuild condition casing. Remove file listing and add target dir for the MauiiOSDefaultIPA unzip. Try initial iOSLlvmBuild casing. Remove printing from setup.sh, fix ios helloworldpath, and set iOSLlvmBuild variable. Fix(?) broken paths in actual tests. Add Maccatalyst and some cleanup. Removed large comment and shifted changed private job settings to clarify the OS commands. Enable all tests for final testing. Another iteration for final changes. Perf update and xharness update test prep. Update the xharness version for perf update testing. Fix net.dot.mauitesting comment. Iterate based on comments and fix MauiMacCatalyst pathing. Change sed and try using ApplicationId instead and removed the MicrosoftDotNetXharnessCLIVersion from the ios_scenarios version. Add podcast app part 1. Add the ios podcast app test itself since it was missed. Add back the incorrectly removed HelixPrecommand steps for Alpine. Change agent.os conditions to be more positive. A wish I would replace all the paths when I try to the first time. Remove file printing. Update xharness to latest version (dotnet#67667) Needed for some perf scenarios. Test using the bottom coded XHARNESSPATH. Activate full pipeline for testing.
55cbac5
to
9f1b88b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This adds tests and flows to run the iOS Startup scenario for Maui. The majority of the work completed revolves around moving the Maui iOS flow from running on Windows (we only had Size on Disk) to running on a MacOS device connected to an iPhone. It also included shifting the app run test to use the xHarnessAppBundleToTest Task. More specifically, the changes included:
TODO:
This is a paired PR with dotnet/performance#2355