Skip to content
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

Merged
merged 17 commits into from
Apr 16, 2024
Merged

Update EOL versions in release/6.0 #14621

merged 17 commits into from
Apr 16, 2024

Conversation

richlander
Copy link
Member

@richlander richlander commented Mar 19, 2024

No description provided.

@AlitzelMendez
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@richlander richlander changed the title Switch to mariner image Update EOL versions in release/6.0 Mar 20, 2024
@richlander
Copy link
Member Author

richlander commented Mar 20, 2024

Related to #8643

https://github.com/dotnet/source-indexer/blob/main/src/SourceBrowser/src/BinLogToSln/BinLogToSln.csproj

Attemping to remove .NET Core 3.1 usage

- task: UseDotNet@2
displayName: Use .NET Core sdk 3.1
inputs:
packageType: sdk
version: 3.1.x
installationPath: $(Agent.TempDirectory)/dotnet
workingDirectory: $(Agent.TempDirectory)

- task: UseDotNet@2
displayName: Install .NET 3.1 runtime
inputs:
packageType: runtime
version: 3.1.x

@richlander
Copy link
Member Author

We need to update this Ubuntu 14.04 reference.

image: mcr.microsoft.com/dotnet-buildtools/prereqs:ubuntu-14.04-cross-0cd4667-20170319080304

richlander and others added 2 commits March 21, 2024 18:14
Co-authored-by: Michael Simons <msimons@microsoft.com>
Co-authored-by: Michael Simons <msimons@microsoft.com>
@richlander
Copy link
Member Author

Getting close now. Seems like we have one issue that is this:

                 Std. Error: 
[09:06:40] dbug: Executing command: 'D:\h\w\AC09092B\p\microsoft.dotnet.xharness.cli\6.0.0-prerelease.24154.4\runtimes\any\native\adb\windows\adb.exe  devices -l'
[09:06:40] fail: Error: listing attached devices / emulators: . Check that any emulators have been started, and attached device(s) are connected via USB, powered-on, unlocked and authorized.
[09:06:40] fail: Exception thrown while trying to find compatible device with architecture 'arm64-v8a'
                 System.Exception: One or more attached Android devices are offline or without USB debug permission
                    at Microsoft.DotNet.XHarness.Android.AdbRunner.GetAttachedDevicesWithProperties(String property) in /_/src/Microsoft.DotNet.XHarness.Android/AdbRunner.cs:line 426
                    at Microsoft.DotNet.XHarness.Android.AdbRunner.GetAllDevicesToUse(ILogger logger, IEnumerable`1 apkRequiredProperties, String propertyName) in /_/src/Microsoft.DotNet.XHarness.Android/AdbRunner.cs:line 467
[09:06:40] crit: Failed to find compatible device: arm64-v8a
                 Microsoft.DotNet.XHarness.Common.CLI.NoDeviceFoundException: Failed to find compatible device: arm64-v8a
                    at Microsoft.DotNet.XHarness.CLI.Commands.Android.AndroidInstallCommand.InvokeHelper(ILogger logger, String apkPackageName, String appPackagePath, IEnumerable`1 apkRequiredArchitecture, String deviceId, TimeSpan bootTimeoutSeconds, AdbRunner runner) in /_/src/Microsoft.DotNet.XHarness.CLI/Commands/Android/AndroidInstallCommand.cs:line 99
                    at Microsoft.DotNet.XHarness.CLI.Commands.Android.AndroidTestCommand.InvokeInternal(ILogger logger) in /_/src/Microsoft.DotNet.XHarness.CLI/Commands/Android/AndroidTestCommand.cs:line 73
XHarness exit code: 85 (ADB_DEVICE_ENUMERATION_FAILURE)
User command ended with 85

@@ -18,7 +18,7 @@ variables:
resources:
containers:
- container: LinuxContainer
image: mcr.microsoft.com/dotnet-buildtools/prereqs:ubuntu-14.04-cross-0cd4667-20170319080304
image: mcr.microsoft.com/dotnet-buildtools/prereqs:centos-7
Copy link
Member

Choose a reason for hiding this comment

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

Why centos-7 versus something newer?

Copy link
Member Author

@richlander richlander Apr 2, 2024

Choose a reason for hiding this comment

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

Because we need to avoid breaking compat for .NET 6. CentOS 7 is part of our support contract.

https://github.com/dotnet/core/blob/main/release-notes/6.0/supported-os.md#libc-compatibility

Note that we're continuing to use CentOS 7 for the product build for .NET 6.

Copy link
Member

Choose a reason for hiding this comment

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

Then why change the source-build legs off of centos-7? Additionally CentOS 7 is EOL in June isn't it? What is the plan for the last 5-6 months?

Copy link
Member Author

Choose a reason for hiding this comment

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

I updated the versions to make them more conservative/consistent.

Plan: status-quo.

MichaelSimons
MichaelSimons previously approved these changes Apr 2, 2024
Copy link
Member

@MichaelSimons MichaelSimons left a comment

Choose a reason for hiding this comment

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

SB changes LGTM

inputs:
packageType: sdk
version: 3.1.x
version: 8.0.x
Copy link
Member

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...

      - task: UseDotNet@2
        displayName: Install Correct .NET Version
        inputs:
          useGlobalJson: true

Copy link
Member Author

@richlander richlander Apr 2, 2024

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.

@@ -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"/>
Copy link
Member

Choose a reason for hiding this comment

The 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 1804 for now. These are just tests that have more coverage in dotnet/runtime anyway.

Copy link
Member

Choose a reason for hiding this comment

The 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 XHarnessTestX86=true above.

Also we should probably use .svc version of the queues.

Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
<HelixTargetQueue Include="ubuntu.2204.amd64.android.29.open"/>
<HelixTargetQueue Include="ubuntu.1804.amd64.android.29.open"/>

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

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

@jonathanpeppers @Redth -- Do you have insight on Android requirements with ,NET 6?

Copy link
Member

@premun premun Apr 9, 2024

Choose a reason for hiding this comment

The 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.
The set of target platforms in these tests probably diverged from what we run in the end repos and is not in any way binding but it covers the basic matrix of simulator/device/OS.

MAUI repos do not use Helix. They use the XHarness tool directly so they don't have this layer.

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

@richlander richlander Apr 15, 2024

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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.

@richlander
Copy link
Member Author

Looks like the PR is now green. The approvals have expired.

@MichaelSimons @premun

@premun premun merged commit 532d156 into release/6.0 Apr 16, 2024
15 checks passed
@premun premun deleted the eol-os-6.0 branch April 16, 2024 15:00
@@ -1,6 +1,6 @@
parameters:
runAsPublic: false
sourceIndexPackageVersion: 1.0.1-20210614.1
sourceIndexPackageVersion: 1.0.1-20240320.1
Copy link
Member

Choose a reason for hiding this comment

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

Why was this changed?

Copy link
Member Author

@richlander richlander Apr 16, 2024

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

The 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

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

@richlander richlander Apr 16, 2024

Choose a reason for hiding this comment

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

Got it. I did that (for main) so that the branches would match. It doesn't make much sense for the 6.0 branch to have a newer package than main.

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.

Copy link
Member

@MichaelSimons MichaelSimons left a comment

Choose a reason for hiding this comment

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

Source-build changes LGTM. One question on a non-source-build change.

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.

6 participants