Skip to content

Expand osx-arm64 test coverage #44008

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

Merged
merged 13 commits into from
Oct 25, 2024

Conversation

v-wuzhai
Copy link
Member

@v-wuzhai v-wuzhai commented Oct 9, 2024

fixes #43916

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Infrastructure untriaged Request triage from a team member labels Oct 9, 2024
@v-wuzhai v-wuzhai marked this pull request as ready for review October 9, 2024 08:18
@v-wuzhai v-wuzhai requested a review from a team as a code owner October 9, 2024 08:18
@v-wuzhai v-wuzhai requested a review from MiYanni October 9, 2024 08:19
@v-wuzhai v-wuzhai force-pushed the dev/Jason/Expand-osx-arm64-test-coverage branch from e9eb935 to 5598c42 Compare October 9, 2024 10:06
@v-wuzhai
Copy link
Member Author

v-wuzhai commented Oct 9, 2024

@MiYanni Is helixTargetContainer still needed here?

@marcpopMSFT
Copy link
Member

@MiYanni will be back on Friday. Looking at the logs, I see it ran the tests on osx.13.amd64.open so another change is needed somewhere in here to target the arm64 helix images. I can try to look tomorrow or we can wait for him or if you see where it should get updated, you can try that tonight.

@v-wuzhai v-wuzhai force-pushed the dev/Jason/Expand-osx-arm64-test-coverage branch from 4dfce23 to deb027f Compare October 10, 2024 09:02
@marcpopMSFT
Copy link
Member

It looks like the latest change got us to run the tests on arm64 successfully as far as i can tell. From the results, the most common failure is in R2R and illink tests when downlevel targeting (net5 and net3.1)
error NETSDK1095: Optimizing assemblies for performance is not supported for the selected target platform or architecture. Please verify you are using a supported runtime identifier, or set the PublishReadyToRun property to false.

A lot of these tests use EnvironmentInfo.GetCompatibleRid so I wonder if we should have some special logic there that returns an x64 rid when targeting <netN.0 and the default would be arm64.

@agocke @elinor-fung Would ya'll want to keep around the downlevel targeting tests and is my proposal to change GetCompatibleRid reasonable or would you like to just get rid of the 3.1/5.0 targeting tests entirely? Alternatively, we could just modify the tests to return when run on arm64.

@elinor-fung
Copy link
Member

cc @dotnet/illink for whether 3.1/5.0 targeting tests are still valuable

If we do keep them, I'd suggest skipping downlevel on arm64 by switching the tests from InlineData to MemberData and only returning those downlevel versions when running on platforms that support it. I don't know that special-casing GetCompatibleRid for <netN.0 on osx-arm64 provides much interesting coverage.

…he minimum version to net6.0
@v-wuzhai v-wuzhai requested review from AntonLapounov and a team as code owners October 12, 2024 06:11
@agocke
Copy link
Member

agocke commented Oct 13, 2024

We can get rid of 3/5 tests

@marcpopMSFT
Copy link
Member

@v-wuzhai see agocke's comment above. Can you go through the tests that are failing on 3.1 and 5.0 in this PR and simply remove that coverage? Thanks.

…dev/Jason/Expand-osx-arm64-test-coverage
…dev/Jason/Expand-osx-arm64-test-coverage
@v-wuzhai v-wuzhai requested a review from a team as a code owner October 16, 2024 02:15
@v-wuzhai
Copy link
Member Author

v-wuzhai commented Oct 17, 2024

It seems that the value of helixTargetQueue cannot be overridden.

[dotnet-sdk-public-ci](https://dev.azure.com/dnceng-public/cbb18261-c48f-4abb-8651-8cdcb5474649/_build/results?buildId=845800) / [[ 🚧 Report infrastructure issue]](https://helix.dot.net/ki/new/build=&pr=https%3A%2F%2Fgithub.com%2Fdotnet%2Fsdk%2Fpull%2F44008&build-leg=Pipeline%20Definition%20Validation&repository=dotnet%2Fdnceng&template=z-build-break-infrastructure-issue-template.yml&labels=Known%20Build%20Error%2CFirst%20Responder%2CDetected%20By%20-%20Customer) [[ 📄 Report repository issue]](https://helix.dot.net/ki/new/build=&pr=https%3A%2F%2Fgithub.com%2Fdotnet%2Fsdk%2Fpull%2F44008&build-leg=Pipeline%20Definition%20Validation&repository=dotnet%2Fsdk&labels=Known%20Build%20Error%2Cblocking-clean-ci) ❌/eng/pipelines/templates/jobs/sdk-job-matrix.yml (Line: 113, Col: 5): 'helixTargetQueue' is already defined

So I customized a variable helixQueue as a specific queue name, and added a script step to dynamically set the CustomHelixTargetQueue variable based on the value of runtimeIdentifier, so that we can see the returned queue value in the pipeline for debugging purposes.

image

@marcpopMSFT
Copy link
Member

Thanks for trying out my proposal and coming up with an alternative. Let's just wait until next week when @MiYanni is back as he's the real expert here and maybe he'll have a better suggestion. Your solution seems reasonable to me though.

Copy link
Member

@MiYanni MiYanni left a comment

Choose a reason for hiding this comment

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

Let me adjust this PR to use the design that is currently in place to accomplish this.

… Arm64 specific TestBuild run in both the PR and Official pipelines.
@@ -291,6 +291,19 @@ extends:
publishArgument: $(_publishArgument)
officialBuildProperties: $(_officialBuildProperties)
runTests: false
### ARM64 TESTBUILD ###
- ${{ if or(eq(parameters.runTestBuild, true), eq(variables['Build.Reason'], 'PullRequest')) }}:
Copy link
Member

Choose a reason for hiding this comment

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

This will only run on internal PRs or when runTestBuild checkbox is checked when running the build manually.

name: Azure Pipelines
vmImage: macOS-latest
os: macOS
helixTargetQueue: osx.13.arm64
Copy link
Member

Choose a reason for hiding this comment

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

helixTargetQueue is specified at the pipeline-level, because the pipelines run in different environments. The external (PR) pipeline uses Helix queues with .open on the end of the name. For the internal (Official) pipeline, we use the internal queues which do not have .open in the name.

@MiYanni
Copy link
Member

MiYanni commented Oct 24, 2024

The Arm64 leg for macOS is appropriately appearing after my changes:
image

Comment on lines +69 to +72
macOSJobParameterSets:
- categoryName: TestBuild
buildArchitecture: arm64
runtimeIdentifier: osx-arm64
Copy link
Member

Choose a reason for hiding this comment

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

Defining macOSJobParameterSets here overrides the default in sdk-job-matrix.yml. Since we only specify a single entry, it will only create a single job. This is also how the design of the Official pipeline works; it defines a bunch of specific job sets to do the official builds.

@v-wuzhai
Copy link
Member Author

@MiYanni Thank you for your additions and optimizations. These changes look good to me.

@v-wuzhai v-wuzhai merged commit 49c42bc into release/9.0.2xx Oct 25, 2024
32 checks passed
@v-wuzhai v-wuzhai deleted the dev/Jason/Expand-osx-arm64-test-coverage branch October 25, 2024 02:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Infrastructure untriaged Request triage from a team member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants