-
Notifications
You must be signed in to change notification settings - Fork 279
Update the iOS scenarios build flow to match android #4834
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
Update the iOS scenarios build flow to match android #4834
Conversation
…em up to running net10.
…sts to the correct one.
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.
Thanks a lot for porting the changes to iOS as well. I would propose to make a few more changes if possible.
- Unifying the scenario naming between
maui_scenarios_ios.proj
andmaui_scenarios_android.proj
so that they follow the same naming conventions so the subsequent PowerBI queries are easier to implement. - Add
codeGenType: FullAOT
to Mono scenarios insdk-perf-jobs.yml
. (I'm unsure if we log somewhere that iOS jobs use LLVM backend. I think we log it only for dotnet/runtime perf measurements not scenarios. It would be nice to log this as well here.) - For CoreCLR, I wonder if we should use
runtimeFlavor: coreclr
+codeGenType: NativeAOT
or use justruntimeFlavor: nativeaot
. @ivanpovazan do you have insight into what is the proper naming here? I'm more in favor of the 1st one, but I'm unsure whether NativeAOT is still considered as part of CoreCLR runtime (it lives in the same subfolder in dotnet/runtime but...)
channels: | ||
- 8.0 | ||
- main |
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.
would it make sense to get channels from a (default) value of a new template parameter? So it's kept uniform across all template
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.
That probably makes sense for most cases, although mobile is currently a special case as we only run main and not each main, 9.0, and 8.0.
versions_write_json(version_dict, rf"{output_dir}/versions.json") | ||
print(f"Versions: {version_dict} from location " + rf"./{const.APPDIR}/obj/Release/{precommands.framework}/ios-arm64/linked/Microsoft.iOS.dll") | ||
print(f"Versions: {version_dict} from location " + rf"./{const.APPDIR}/obj/Release/{precommands.framework}/ios-arm64/linked") |
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.
probably out of scope for this PR but would it be possible and would it make sense to unify all the pre.py scripts to avoid code/configs duplications to avoid possible misconfigurations/bugs in the future?
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 in general, few comments left though
For 1, I have a commit ready locally but also need the configuration updates so we can use it. For 2 and partially 3, I am looking for confirmation of the correct configuration values to use.
Once we have figured out what we want for the configurations, I think making those changes will be pretty fast. |
In fact, all three use different setup:
Each configuration differs in the amount of code that is AOTed before the app execution.
I would prefer the
LLVM is only relevant to Mono. In current setup, only affects iOS builds (we do allow LLVM with Android builds, but it is not a recommended scenario, and we don't measure it at the moment). |
@LoopedBard3 Are you referring to I agree it is confusing and is probably misused in some places, like using I am more inclined towards having We can then revisit this again once we tackle dotnet/runtime#111923 |
Yes. We end up passing the value through these scripts like that due to hitting an issue when trying to use the RuntimeType parameter directly in one of the yamls somewhere (I think it was in the runtime mobile yamls). RuntimeType was already a defined parameter that influenced the templates and I had issues passing it through all the way to our scripts so instead used RuntimeFlavor as the parameter in it's place. |
…pe to be saved as configurations.
Latest successful test run: https://dev.azure.com/dnceng/internal/_build/results?buildId=2689882&view=results |
…not currently running the startup tests for blazor.
As I am not 100% sure on the correct value for the llvm configuration value for iOS, I will confirm it and add it in another PR. |
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. Thanks for the changes.
Update the iOS scenarios to use the same flow as android, bringing them up to running net10. This copies the build flow update from #4770 to the iOS scenarios.
Successful test run: https://dev.azure.com/dnceng/internal/_build/results?buildId=2689882&view=results