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

Add VMR leg that builds test projects #44843

Draft
wants to merge 13 commits into
base: main
Choose a base branch
from
Draft

Conversation

ViktorHofer
Copy link
Member

Fixes dotnet/source-build#4168

Running as part of the PR to get some initial results.

Fixes dotnet/source-build#4168

Running as part of the PR to get some initial results.
@ViktorHofer ViktorHofer requested review from a team as code owners November 13, 2024 13:08
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Infrastructure untriaged Request triage from a team member labels Nov 13, 2024
@ViktorHofer
Copy link
Member Author

ViktorHofer commented Nov 13, 2024

src\winforms\src\System.Windows.Forms.Design\tests\UnitTests\System\Windows\Forms\Design\ToolStripContentPanelDesignerTests.cs(43,67): error CS8620: (NETCORE_ENGINEERING_TELEMETRY=Build) Argument of type 'string[]' cannot be used for parameter 'span' of type 'Span<string?>' in 'bool MemoryExtensions.Contains<string?>(Span<string?> span, string? value)' due to differences in the nullability of reference types.

@am11 did we see this one when we updated the VMR to the .NET 10 SDK? Can't remember if we talked about it.

@am11
Copy link
Member

am11 commented Nov 13, 2024

Yup, it was #43015 (comment). It will probably need a fix like dotnet/winforms@main...am11:winforms:patch-1. I'll try building it on windows soon.

@ViktorHofer
Copy link
Member Author

    D:\a\_work\1\vmr\src\efcore\test\EFCore.Specification.Tests\CustomConvertersTestBase.cs(1187,49): error CS0023: Operator '.' cannot be applied to operand of type 'void' [D:\a\_work\1\vmr\src\efcore\test\EFCore.Specification.Tests\EFCore.Specification.Tests.csproj]
    D:\a\_work\1\vmr\src\efcore\test\EFCore.Specification.Tests\CustomConvertersTestBase.cs(1188,49): error CS0023: Operator '.' cannot be applied to operand of type 'void' [D:\a\_work\1\vmr\src\efcore\test\EFCore.Specification.Tests\EFCore.Specification.Tests.csproj]
    D:\a\_work\1\vmr\src\efcore\test\EFCore.Specification.Tests\Query\PrimitiveCollectionsQueryTestBase.cs(430,67): error CS8620: Argument of type 'string[]' cannot be used for parameter 'span' of type 'Span<string?>' in 'bool MemoryExtensions.Contains<string?>(Span<string?> span, string? value)' due to differences in the nullability of reference types. [D:\a\_work\1\vmr\src\efcore\test\EFCore.Specification.Tests\EFCore.Specification.Tests.csproj]
    D:\a\_work\1\vmr\src\efcore\test\EFCore.Specification.Tests\Query\PrimitiveCollectionsQueryTestBase.cs(433,68): error CS8620: Argument of type 'string[]' cannot be used for parameter 'span' of type 'Span<string?>' in 'bool MemoryExtensions.Contains<string?>(Span<string?> span, string? value)' due to differences in the nullability of reference types. [D:\a\_work\1\vmr\src\efcore\test\EFCore.Specification.Tests\EFCore.Specification.Tests.csproj]
    D:\a\_work\1\vmr\src\efcore\test\EFCore.Specification.Tests\Query\PrimitiveCollectionsQueryTestBase.cs(577,67): error CS8620: Argument of type 'string[]' cannot be used for parameter 'span' of type 'Span<string?>' in 'bool MemoryExtensions.Contains<string?>(Span<string?> span, string? value)' due to differences in the nullability of reference types. [D:\a\_work\1\vmr\src\efcore\test\EFCore.Specification.Tests\EFCore.Specification.Tests.csproj]

@am11 would you want to help me with driving this one in? I currently have some other obligations and won't be able to get back to this PR anytime soon. See the above efcore test failure. They need the same first class Span reaction update.

There's also a winforms test failure in the PR that I don't really understand yet.

@am11
Copy link
Member

am11 commented Nov 19, 2024

I think #44922 is bringing winforms change. I'll check efcore.

@ViktorHofer
Copy link
Member Author

The efcore issue is weird:

2024-11-20T20:05:12.6651643Z     FSC : error FS2014: A problem occurred writing the binary 'D:\a\_work\1\vmr\src\efcore\artifacts\obj\EFCore.FSharp.FunctionalTests\Release\net9.0\refint\Microsoft.EntityFrameworkCore.FSharp.FunctionalTests.dll': A call to StrongNameSignatureSize failed (Invalid Public Key blob) [D:\a\_work\1\vmr\src\efcore\test\EFCore.FSharp.FunctionalTests\EFCore.FSharp.FunctionalTests.fsproj]

The winforms failure is also still happening.

@am11
Copy link
Member

am11 commented Nov 20, 2024

Looks like a known bug dotnet/fsharp#17451. A solution (workaround?) is here dotnet/fsharp#11546 (comment).

Since it is a test assembly perhaps we should just disable strong name signing?

@ViktorHofer
Copy link
Member Author

It would be interesting to understand why the fsharp test build behaves differently with the VMR.

@NikolaMilosavljevic
Copy link
Member

efcore issue is caused by /p:FullAssemblySigningSupported=false which is set in VMR build. This causes PublicSign to not be set to false here: https://github.com/dotnet/arcade/blob/2fd9b394c6ef3ad22639d83b8da649c9350f88f6/src/Microsoft.DotNet.Arcade.Sdk/tools/StrongName.targets#L50

I'll try to set PublicSign to false explicitly in this test project.

@ViktorHofer
Copy link
Member Author

ViktorHofer commented Dec 4, 2024

cc @mmitche as you approved the Arcade PR: dotnet/arcade#12940 and might have context here.

@mmitche
Copy link
Member

mmitche commented Dec 4, 2024

I believe this should be conditioned on source-only or non-source only builds, rather than a blanket setting across all VMR builds. That said, I'm slightly uncomfortable with where the setting is put. I think it should actually be in StrongName.targets or generally within the arcade infra, rather than the source build control setup.

@mmitche
Copy link
Member

mmitche commented Dec 4, 2024

I'll file an issue and open a PR to fix this.

@mmitche
Copy link
Member

mmitche commented Dec 4, 2024

dotnet/arcade#15301

@ViktorHofer
Copy link
Member Author

ViktorHofer commented Jan 3, 2025

The winforms failures might be related to these sync exclusions:

"name": "winforms",
"defaultRemote": "https://github.com/dotnet/winforms",
"exclude": [
// Non-OSS license - https://github.com/dotnet/source-build/issues/3772
"src/System.Windows.Forms/tests/IntegrationTests/DesignSurface/**/*.cs",
"src/System.Windows.Forms/tests/IntegrationTests/DesignSurface/**/*.ico",
"src/System.Windows.Forms/tests/IntegrationTests/DesignSurface/**/*.resx",
"src/System.Windows.Forms/tests/IntegrationTests/DesignSurface/LICENSE.txt"
]

The aspnetcore failures happen because we build the repo with desktop msbuild.

@NikolaMilosavljevic
Copy link
Member

Merging with latest brought 4 compiler issues in arcade for an obsolete API - here's one instance of the error:

D:\a\_work\1\vmr\src\arcade\src\Microsoft.DotNet.Build.Tasks.Feed.Tests\LatestLinksManagerTests.cs(107,62): error SYSLIB0026: 'X509Certificate2.X509Certificate2()' is obsolete: 'X509Certificate and X509Certificate2 are immutable. Use X509CertificateLoader to create a new certificate.' (https://aka.ms/dotnet-warnings/SYSLIB0026)

I'll either suppress this or have a fix - it will go directly to arcade and wait for it to flow.

@ViktorHofer
Copy link
Member Author

This will be taken care of as part of dotnet/arcade#15404

@NikolaMilosavljevic
Copy link
Member

NikolaMilosavljevic commented Jan 14, 2025

This will be taken care of as part of dotnet/arcade#15404

In my local build with a fix for arcade, I hit the following WinForms errors - @ViktorHofer do you know if these will also be fixed with the SDK update? If not, I'll work on the fixes.

  C:\git\dotnet\src\winforms\src\System.Windows.Forms.Primitives\tests\UnitTests\Interop\Mocks\MockCursor.cs(14,29): error IDE0052: Private member 'MockCursor._resourceId' can be removed as the value assigned to it is never read (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/style-rules/ide0052) [C:\git\dotnet\src\winforms\src\System.Windows.Forms.Primitives\tests\UnitTests\System.Windows.Forms.Primitives.Tests.csproj]
  C:\git\dotnet\src\winforms\src\System.Drawing.Common\tests\System\Drawing\Drawing2D\GraphicsPathTests.cs(2069,25): error IDE0051: Private member 'GraphicsPathTests.AssertEqual' is unused (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/style-rules/ide0051) [C:\git\dotnet\src\winforms\src\System.Drawing.Common\tests\System.Drawing.Common.Tests.csproj]
  C:\git\dotnet\src\winforms\src\System.Windows.Forms.Design\tests\UnitTests\System\ComponentModel\Design\Serialization\CodeDomComponentSerializationServiceTests.cs(1697,25): error IDE0051: Private member 'CodeDomComponentSerializationServiceTests.DumpState' is unused (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/style-rules/ide0051) [C:\git\dotnet\src\winforms\src\System.Windows.Forms.Design\tests\UnitTests\System.Windows.Forms.Design.Tests.csproj]
  C:\git\dotnet\src\winforms\src\System.Windows.Forms\tests\IntegrationTests\WinformsControlsTest\RichTextBoxes.cs(62,45): error IDE0057: Remove can be simplified (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/style-rules/ide0057) [C:\git\dotnet\src\winforms\src\System.Windows.Forms\tests\IntegrationTests\WinformsControlsTest\WinformsControlsTest.csproj]
  C:\git\dotnet\src\winforms\src\System.Windows.Forms\tests\IntegrationTests\WinformsControlsTest\RichTextBoxes.cs(75,36): error IDE0057: Remove can be simplified (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/style-rules/ide0057) [C:\git\dotnet\src\winforms\src\System.Windows.Forms\tests\IntegrationTests\WinformsControlsTest\WinformsControlsTest.csproj]
  C:\git\dotnet\src\winforms\src\System.Windows.Forms\tests\UnitTests\System\Windows\Forms\ControlPaintTests.cs(500,18): error IDE0051: Private member 'ControlPaintTests.ControlPaint_Dark_InvokeColorFloat_ReturnsExpected' is unused (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/style-rules/ide0051) [C:\git\dotnet\src\winforms\src\System.Windows.Forms\tests\UnitTests\System.Windows.Forms.Tests.csproj]
  C:\git\dotnet\src\winforms\src\System.Windows.Forms\tests\UnitTests\System\Windows\Forms\ControlPaintTests.cs(533,18): error IDE0051: Private member 'ControlPaintTests.ControlPaint_DarkDark_InvokeColorFloat_ReturnsExpected' is unused (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/style-rules/ide0051) [C:\git\dotnet\src\winforms\src\System.Windows.Forms\tests\UnitTests\System.Windows.Forms.Tests.csproj]
  C:\git\dotnet\src\winforms\src\System.Windows.Forms\tests\UnitTests\System\Windows\Forms\ControlPaintTests.cs(2264,18): error IDE0051: Private member 'ControlPaintTests.ControlPaint_Light_InvokeColorFloat_ReturnsExpected' is unused (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/style-rules/ide0051) [C:\git\dotnet\src\winforms\src\System.Windows.Forms\tests\UnitTests\System.Windows.Forms.Tests.csproj]
  C:\git\dotnet\src\winforms\src\System.Windows.Forms\tests\UnitTests\System\Windows\Forms\DataObjectTests.ClipboardTests.cs(107,22): error IDE0051: Private member 'DataObjectTests.ClipboardTests.DataObject_SetData_InvokeStringObject_GetReturnsExpected' is unused (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/style-rules/ide0051) [C:\git\dotnet\src\winforms\src\System.Windows.Forms\tests\UnitTests\System.Windows.Forms.Tests.csproj]
  C:\git\dotnet\src\winforms\src\System.Windows.Forms\tests\UnitTests\System\Windows\Forms\DataObjectTests.ClipboardTests.cs(169,22): error IDE0051: Private member 'DataObjectTests.ClipboardTests.DataObject_SetData_InvokeStringBoolObject_GetReturnsExpected' is unused (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/style-rules/ide0051) [C:\git\dotnet\src\winforms\src\System.Windows.Forms\tests\UnitTests\System.Windows.Forms.Tests.csproj]

@ViktorHofer
Copy link
Member Author

ViktorHofer commented Jan 14, 2025

No they won't. These are new analyzer warnings that got introduced with the recent SDK bump. Those would need to get addressed or suppressed.

@NikolaMilosavljevic
Copy link
Member

arcade fix is flowing, but the PR is failing: #46021 In the meantime, I've made progress in local build. I have fixes for WinForms - PR following.

Now, aspnetcore is failing due to missing test certificates, required by various tests. We are skipping the creation of those certificates due to this false condition: https://github.com/dotnet/aspnetcore/blob/7340678143d9ade4b772bad5e3ffd0b86718706e/src/ProjectTemplates/TestInfrastructure/PrepareForTest.targets#L34-L38

    <!-- This task only tries to generate a certificate if there is none existing at the location provided as path. -->
    <GenerateTestDevCert
        CertificatePath="$(_DevCertPath)"
        Condition="'$(MSBuildRuntimeType)' == 'core'">
    </GenerateTestDevCert>

MSBuildRuntimeType is set to Full due to: https://github.com/dotnet/dotnet/blob/81f0f65d674c3aebd3c1ce78f2e5d831d32980f8/repo-projects/aspnetcore.proj#L19-L21

    <!-- aspnetcore must be built with desktop msbuild but defaults to dotnet build. -->
    <BuildArgs Condition="'$(BuildOS)' == 'windows'">$(BuildArgs) -msbuildEngine vs</BuildArgs>
    <ForceDotNetMSBuildEngine>false</ForceDotNetMSBuildEngine>

@wtgodbe @ViktorHofer is there a way around this? Excluding these tests might be one option but I'm unsure of the impact of that change.

@ViktorHofer
Copy link
Member Author

@wtgodbe @ViktorHofer is there a way around this? Excluding these tests might be one option but I'm unsure of the impact of that change.

@wtgodbe this code path looks problematic as we are building aspnetcore with desktop msbuild inside the VMR. Any idea why this is conditioned to only run with core msbuild?

@NikolaMilosavljevic
Copy link
Member

@wtgodbe @ViktorHofer is there a way around this? Excluding these tests might be one option but I'm unsure of the impact of that change.

@wtgodbe this code path looks problematic as we are building aspnetcore with desktop msbuild inside the VMR. Any idea why this is conditioned to only run with core msbuild?

The task isn't compiled for desktop msbuild: https://github.com/dotnet/aspnetcore/blob/7340678143d9ade4b772bad5e3ffd0b86718706e/eng/tools/RepoTasks/RepoTasks.csproj#L29-L30

I'll try to build it and remove task condition, to see what we get.

@wtgodbe
Copy link
Member

wtgodbe commented Jan 22, 2025

That task/behavior is new as of dotnet/aspnetcore#57431, @javiercn may know more

@javiercn
Copy link
Member

@wtgodbe I think it was because it uses some APIs that are only available in .NET Core.

@NikolaMilosavljevic
Copy link
Member

@wtgodbe I think it was because it uses some APIs that are only available in .NET Core.

@ViktorHofer @wtgodbe do you know the reason for using desktop msbuild for aspnetcore in VMR (Windows)? Other repos are building with Core msbuild.

Apparently, Windows build leg in aspnetcore repo is also using Core msbuild.

@wtgodbe
Copy link
Member

wtgodbe commented Jan 23, 2025

It's because we build our native components (e.g. ANCM, aspnetcorev2_inprocess.dll) during each vertical, and don't have the logic dotnet/runtime has to shell out to desktop msbuild just for those portions of the build. Skipping those tests in the VMR might be easiest for now, about how many are failing?

@ViktorHofer
Copy link
Member Author

ViktorHofer commented Jan 23, 2025

Building aspnetcore with desktop in the VMR is currently a strong dependency. As Will mentioned, it's needed for both the native vcxprojs part but also for the wixprojs. The latter will go away when we upgrade to a newer version of the Wix toolset.

FWIW I think the only right solution here is to not exclude the task compile item when building with desktop msbuild. By any means of polyfilling or using different APIs. @NikolaMilosavljevic I assume you already tried that? What are the challenges with those files?

@NikolaMilosavljevic
Copy link
Member

Building aspnetcore with desktop in the VMR is currently a strong dependency. As Will mentioned, it's needed for both the native vcxprojs part but also for the wixprojs. The latter will go away when we upgrade to a newer version of the Wix toolset.

FWIW I think the only right solution here is to not exclude the task compile item when building with desktop msbuild. By any means of polyfilling or using different APIs. @NikolaMilosavljevic I assume you already tried that? What are the challenges with those files?

I am working on enabling the task on desktop - it doesn't appear to be trivial as there are dependencies that also need to be enabled. I'll have more data soon.

@NikolaMilosavljevic
Copy link
Member

As expected, enabling GenerateTestDevCert task won't be easy. First, the task requires https://github.com/dotnet/aspnetcore/blob/main/eng/tools/RepoTasks/shared/CertificateGeneration/DevelopmentCertificate.cs - enabling this to build for .NET FX was simple.

However, that class, has a dependency on CertificateManager: https://github.com/dotnet/aspnetcore/blob/febd7e8bdf05f17fb4e0e4dd3123e9538fbf7e7b/src/Shared/CertificateGeneration/CertificateManager.cs#L646

Library that implements CertificateManager builds for Core only, and the code has many use cases of modern C# language features that won't compile on .NET FX. I don't think this would be a good use of anyone's time to convert.

Two ideas:

  • Disable the affected tests
  • Use a different code for certificate generation on .NET FX

@javiercn
Copy link
Member

javiercn commented Jan 24, 2025

Use a different code for certificate generation on .NET FX

Given that this only runs on windows, could you shell out to New-SelfSignedCertificate or MakeCert in that case? I believe the main "thing" that the dev cert has is a custom extension, which I believe you could add.

I would caution against disabling the tests, since we would be loosing important coverage.

@NikolaMilosavljevic
Copy link
Member

NikolaMilosavljevic commented Jan 24, 2025

Use a different code for certificate generation on .NET FX

Given that this only runs on windows, could you shell out to New-SelfSignedCertificate or MakeCert in that case? I believe the main "thing" that the dev cert has is a custom extension, which I believe you could add.

I would caution against disabling the tests, since we would be loosing important coverage.

I am testing some code for proper certificate generation on Windows - some of this was borrowed from CertificateManager class. If this does work, I'll think of further optimization.

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.

Rolling CI builds should build the test projects, to validate that things still build on the merged content
6 participants