-
Notifications
You must be signed in to change notification settings - Fork 342
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 EOL versions in release/6.0
#14621
Changes from 10 commits
8b8bf20
75c504c
c5831a7
b75d737
7b3f47b
689d328
95c8d33
3f74e1f
ed3f74a
f418906
2efdb92
f8d5c80
424e5de
4d4be3f
dfd730c
8a0f7a4
d274d6f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,6 @@ | ||
parameters: | ||
runAsPublic: false | ||
sourceIndexPackageVersion: 1.0.1-20210614.1 | ||
sourceIndexPackageVersion: 1.0.1-20240320.1 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why was this changed? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Great question. The earlier version of the package was a 3.1 tool. New package is 8.0. Enabled these changes: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Apologies, I thought I am the only who can merge this so I did There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think you did anything wrong. Michael asked a question and I believe I provided a satisfactory answer. I would say that the source index package version is odd and approximately zero people are paying attention to that. It should be on a checklist for stuff we watch for. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the explanation Rich. FYI - I reverted these change in the main PR. 832b3fc before merging as they appeared unrelated and I didn't get a response. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Got it. I did that (for We're missing a policy on when this package should be upgraded. The versioning scheme for the package doesn't help much. The forcing function of upgrading the package for this branch was more obvious. @hoyosjs -- can you offer insight here. |
||
sourceIndexPackageSource: https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-tools/nuget/v3/index.json | ||
sourceIndexBuildCommand: powershell -NoLogo -NoProfile -ExecutionPolicy Bypass -Command "eng/common/build.ps1 -restore -build -binarylog -ci" | ||
preSteps: [] | ||
|
@@ -30,10 +30,10 @@ jobs: | |
- ${{ preStep }} | ||
|
||
- task: UseDotNet@2 | ||
displayName: Use .NET Core sdk 3.1 | ||
displayName: Use .NET 8 SDK | ||
inputs: | ||
packageType: sdk | ||
version: 3.1.x | ||
version: 8.0.x | ||
installationPath: $(Agent.TempDirectory)/dotnet | ||
workingDirectory: $(Agent.TempDirectory) | ||
|
||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -11,7 +11,7 @@ | |||||
<XHarnessAndroidProject Include="$(MSBuildThisFileDirectory)XHarness/XHarness.TestApks.proj"> | ||||||
<AdditionalProperties>XHarnessTestX86=true;XHarnessTestX86_64=true</AdditionalProperties> | ||||||
</XHarnessAndroidProject> | ||||||
<HelixTargetQueue Include="ubuntu.1804.amd64.android.29.open"/> | ||||||
<HelixTargetQueue Include="ubuntu.2204.amd64.android.29.open"/> | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The build failures you're seeing are connected to this change - these are Android emulator tests. I am honestly not sure what's the story with 22.04 and emulators but you could technically leave this at There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suspect the 32 bit Android emulators are not working well in 22.04. I believe we are not testing on those anymore in runtime anyway. So alternatively, you can keep 22.04 but remove Also we should probably use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for that. It looks like the build is now succeeding but I made this proposed change, too. It aligns with others I made. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jonathanpeppers @Redth -- Do you have insight on Android requirements with ,NET 6? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can give some context on why we have this in Arcade. Arcade contains Helix SDK which help .NET repos run tests. A subset of Helix SDK is the XHarness SDK - this allows (mostly runtime) repos to point to a test project and the XHarness SDK builds an apk/app bundle, sends it to Helix, runs the tests on devices, collects results, increases reliability, cleans up after, etc. Basically XHarness SDK is a layer on top of our Apple/Android infrastructure so that as a whole, we provide quite reliable "testing on phones-as-a-service". The tests that were failing here give us coverage for this functionality as runtime depends on this heavily. MAUI repos do not use Helix. They use the XHarness tool directly so they don't have this layer. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I propose we just switch back to 1804 for .NET 6. It isn't worth the trouble. We should make sure main and net8 are good. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I switched the XHarness test back to 1804. It seems like XHarness is still failing. Anyone got a guess on that? This seems unrelated? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe this is just some inherent flakiness of the 6.0 world where the real world phone infra just swept from under the 6.0 branches as literally all of the parts there got updated. So I think re-running the legs a couple of times should get you a green check eventually. I realize this is not ideal but there's just not enough funding to keep the 6.0 world up to date with the everchanging MacOS and Linux versions. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can also merge over this, I don't think it's a big deal honestly. |
||||||
</ItemGroup> | ||||||
|
||||||
<Target Name="Pack"/> | ||||||
|
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.
Any reason the 6.0 sdk wouldn't work here? ie, it simplifies things if all the places are using the same sdk where possible...
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's because (I believe) the global tool has been updated to .NET 8. it was using .NET Core 3.1 so I upgraded the tool to latest, which seemed to make the most sense.