Helix Restore Issue Mitigation #41311

TanayParikh · 2022-04-21T22:06:35Z

No description provided.

eng/Build.props

eng/helix/content/runtests.cmd

eng/helix/content/runtests.sh

eng/helix/content/RunTests/RunTests.csproj

dougbu

If CI validation works, great. Would still prefer you remove the duplication around CA2007

TanayParikh · 2022-04-21T23:40:15Z

/mnt/vss/_work/1/s/eng/helix/content/RunTests/RunTests.csproj : error MSB4057: The target "Test" does not exist in the project.
##[error]eng/helix/content/RunTests/RunTests.csproj(0,0): error MSB4057: (NETCORE_ENGINEERING_TELEMETRY=Build) The target "Test" does not exist in the project.

https://dev.azure.com/dnceng/public/_build/results?buildId=1731724&view=logs&jobId=5ac7b393-e840-5549-7fb4-a4479af8e7e3&j=5ac7b393-e840-5549-7fb4-a4479af8e7e3&t=29df2fa2-0d20-51bd-e85a-8b546e86c529

dougbu · 2022-04-21T23:42:00Z

@TanayParikh to avoid that, set $(IsUnitTestProject) and $(IsTestProject) to false in the project. Wasn't an issue before but this project name matches our usual test project names

eng/helix/content/RunTests/ProcessUtil.cs

dougbu

🥇

dougbu · 2022-04-22T00:26:14Z

I still approve 😃

TanayParikh · 2022-04-22T02:29:09Z

@TanayParikh to avoid that, set $(IsUnitTestProject) and $(IsTestProject) to false in the project. Wasn't an issue before but this project name matches our usual test project names

Hmm that didn't seem to do it, the issue is persisting: https://dev.azure.com/dnceng/public/_build/results?buildId=1731849&view=logs&jobId=5ac7b393-e840-5549-7fb4-a4479af8e7e3&j=5ac7b393-e840-5549-7fb4-a4479af8e7e3&t=29df2fa2-0d20-51bd-e85a-8b546e86c529

5a96d33 set IsUnitTestProject and IsTestProject to false

eng/helix/content/RunTests/RunTests.csproj

dougbu · 2022-04-28T20:30:51Z

I rewrote the history of this PR to remove reverted commits, rebased on 'main', and added the workarounds discussed above and in Teams. The result seems simpler than I expected though the 11 brand new commits were a bit of work 😉

Some of the commits are strictly cleanup as I went along (hitting issues).

eng/tools/RunTests/ProcessResult.cs

eng/helix/helix.proj

HaoK · 2022-04-28T21:41:40Z

eng/helix/content/RunTests/Program.cs

@@ -19,14 +19,18 @@ static async Task Main(string[] args)
                {
                    keepGoing = await runner.InstallDotnetToolsAsync();
                }
-#if INSTALLPLAYWRIGHT
+


I believe the reason I did this was because some queues don't support playwright but maybe now that we are building this on CI rather than on the actual machines, this doesn't matter anymore, but just something to be aware of

HaoK · 2022-04-28T21:47:15Z

Looks great overall, I'd suggest we take this opportunity now that we are moving it to rename this RunHelixTests or something similar since its not longer in the helix content directory

HaoK · 2022-04-28T21:49:13Z

HelixTestRunner for the project is my vote

- add generation to GenerateFiles.csproj - include all required versions in eng/Versions.props - nit: bump tool versions slightly - `dotnet-dump` move from `5.0.0-*` to `6.0.322601` is largest version bump - have `git` ignore generated file - nit: put `*.svclog` together w/ other extension exclusions

- get tool packages from Helix correlation root - to do this, save and restore NuGet.config file - this removes `--version` from `dotnet-dump` and `dotnet-ef` installations - will only have a single package for each tool in the correlation payload

- mostly cleanup; no longer needed

- mostly cleanup - `dotnet-ef` tool now restored by Arcade's Tools.proj much earlier in our build because most configured tool packages are needed in `RunTests` on Helix agents

- remove `INSTALLPLAYWRIGHT` define and `$env:INSTALLPLAYWRIGHT` - always reference Microsoft.Playwright in `RunTests` - nit: `InstallPlaywrightAsync()` wasn't `async`; fix it and rename to `InstallPlaywright()`

- match most other projects in this repo - remove empty Directory.Build.props and .targets files preventing Arcade imports - exclude project build if `$(SkipTestBuild)` (though not a test project)

- don't need the project on Helix agents - restore Directory.Build.* files removed when switching to Arcade

- less confusing given `RunTests` target and runtests.sh et cetera

dougbu · 2022-04-29T01:23:28Z

HelixTestRunner for the project is my vote

Good suggestion, especially since we hook into an Arcade RunTests target and have scripts named runtests.*

HaoK

Nice!

HaoK · 2022-04-29T17:43:08Z

eng/tools/GenerateFiles/GenerateFiles.csproj

@@ -15,10 +17,14 @@
      <_TemplateProperties>
        AspNetCorePatchVersion=$(AspNetCorePatchVersion);
        DefaultNetCoreTargetFramework=$(DefaultNetCoreTargetFramework);
+        DotnetDumpVersion=$(DotnetDumpVersion);
+        dotnetefVersion=$(dotnetefVersion);


minor nit: casing of dotnetefVersion here is wonky compared to everything else like DotnetDump

I agree but it has to match the casing of the dependency in Version.Details.xml for strange reasons related to the wild logic in the https://github.com/dotnet/aspnetcore/blob/main/eng/Dependencies.props#L218 item group.

I may see if property name case-insensitivity makes a difference these days but will leave that for another PR (even though this one is failing validation).

eng/helix/helix.proj

dougbu · 2022-04-30T00:06:37Z

Found what I think is the last bug in this PR. %(HelixCorrelationPayload.Destination) metadata shouldn't have a trailing slash of any type. Adding a backslash causes problems on Unix-based Helix agents.

ghost added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Apr 21, 2022

dougbu reviewed Apr 21, 2022

View reviewed changes

eng/Build.props Outdated Show resolved Hide resolved

eng/helix/content/runtests.cmd Outdated Show resolved Hide resolved

eng/helix/content/runtests.sh Outdated Show resolved Hide resolved

eng/helix/content/RunTests/RunTests.csproj Outdated Show resolved Hide resolved

dougbu approved these changes Apr 21, 2022

View reviewed changes

dougbu reviewed Apr 21, 2022

View reviewed changes

eng/helix/content/RunTests/ProcessUtil.cs Outdated Show resolved Hide resolved

dougbu approved these changes Apr 22, 2022

View reviewed changes

TanayParikh marked this pull request as ready for review April 22, 2022 00:21

TanayParikh requested review from a team and wtgodbe as code owners April 22, 2022 00:21

dougbu reviewed Apr 22, 2022

View reviewed changes

eng/helix/content/RunTests/RunTests.csproj Outdated Show resolved Hide resolved

dougbu reviewed Apr 22, 2022

View reviewed changes

eng/helix/content/RunTests/RunTests.csproj Outdated Show resolved Hide resolved

dougbu force-pushed the taparik/fixHelixRestoreIssue branch from 8ac42bf to 04e7adf Compare April 28, 2022 20:26

TanayParikh requested review from captainsafia, mkArtakMSFT and Pilchie as code owners April 28, 2022 20:26

dougbu reviewed Apr 28, 2022

View reviewed changes

eng/tools/RunTests/ProcessResult.cs Outdated Show resolved Hide resolved

dougbu requested a review from HaoK April 28, 2022 20:36

wtgodbe reviewed Apr 28, 2022

View reviewed changes

eng/helix/helix.proj Outdated Show resolved Hide resolved

HaoK reviewed Apr 28, 2022

View reviewed changes

TanayParikh added 5 commits April 28, 2022 18:05

RunTests dotnet run --no-build

021516b

Remove duplicate run

ee6202c

Add to Build.props

674d5b9

Treat CA2007 as a warning in RunTests

85c3d69

NoWarn CA2007

133ce0b

dougbu added 14 commits April 28, 2022 18:05

Add tool packages to Helix correlation payloads

56bacbe

Do not restore tools from network

95a90e5

- get tool packages from Helix correlation root - to do this, save and restore NuGet.config file - this removes `--version` from `dotnet-dump` and `dotnet-ef` installations - will only have a single package for each tool in the correlation payload

Do not pass dotnet-ef version to RunTests

0f97390

- mostly cleanup; no longer needed

Remove DotNetToolsInstaller.csproj

9a2900d

- mostly cleanup - `dotnet-ef` tool now restored by Arcade's Tools.proj much earlier in our build because most configured tool packages are needed in `RunTests` on Helix agents

Add --playwright option in Runtests

787b743

- remove `INSTALLPLAYWRIGHT` define and `$env:INSTALLPLAYWRIGHT` - always reference Microsoft.Playwright in `RunTests` - nit: `InstallPlaywrightAsync()` wasn't `async`; fix it and rename to `InstallPlaywright()`

Use Arcade SDK in RunTests and Publish project

3664794

- match most other projects in this repo - remove empty Directory.Build.props and .targets files preventing Arcade imports - exclude project build if `$(SkipTestBuild)` (though not a test project)

Correct minor problems in RunTests source

f550234

Use file-scoped namespaces in RunTests source

ac9b2f8

Use VS code fixes in RunTests

d9ef4d8

Use published RunTests output on Helix agents

c12501c

Move RunTests under eng/tools/

ddc91e0

- don't need the project on Helix agents - restore Directory.Build.* files removed when switching to Arcade

Add RunTests to tools.slnf

9961ccb

Rename RunTests project to HelixTestRunner

5b14ca6

- less confusing given `RunTests` target and runtests.sh et cetera

dougbu force-pushed the taparik/fixHelixRestoreIssue branch from 04e7adf to 5b14ca6 Compare April 29, 2022 01:22

dougbu added 2 commits April 28, 2022 21:29

!fixup! Define $(ArtifactsBinDir) in helix.proj

b3e15e0

!fixup! Use $(Configuration) in helix.proj

a788fd6

HaoK approved these changes Apr 29, 2022

View reviewed changes

HaoK reviewed Apr 29, 2022

View reviewed changes

eng/helix/helix.proj Show resolved Hide resolved

dougbu added 2 commits April 29, 2022 12:30

!fixup! Fix typo in property function

9e45752

!fixup! Fix dotnet syntax in runtests.sh

a22be2b

!fixup! Correct %(HelixCorrelationPayload.Destination)

9ffdffb

dougbu enabled auto-merge (squash) April 30, 2022 00:10

dougbu merged commit 0be2c29 into main Apr 30, 2022

dougbu deleted the taparik/fixHelixRestoreIssue branch April 30, 2022 01:31

ghost added this to the 7.0-preview5 milestone Apr 30, 2022

BrennanConroy mentioned this pull request May 10, 2022

Run tests on Windows again #41599

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Helix Restore Issue Mitigation #41311

Helix Restore Issue Mitigation #41311

TanayParikh commented Apr 21, 2022

dougbu left a comment

TanayParikh commented Apr 21, 2022

dougbu commented Apr 21, 2022

dougbu left a comment

dougbu commented Apr 22, 2022

TanayParikh commented Apr 22, 2022

dougbu commented Apr 28, 2022

HaoK Apr 28, 2022

HaoK commented Apr 28, 2022

HaoK commented Apr 28, 2022

dougbu commented Apr 29, 2022

HaoK left a comment

HaoK Apr 29, 2022

dougbu Apr 29, 2022

dougbu commented Apr 30, 2022

Helix Restore Issue Mitigation #41311

Helix Restore Issue Mitigation #41311

Conversation

TanayParikh commented Apr 21, 2022

dougbu left a comment

Choose a reason for hiding this comment

TanayParikh commented Apr 21, 2022

dougbu commented Apr 21, 2022

dougbu left a comment

Choose a reason for hiding this comment

dougbu commented Apr 22, 2022

TanayParikh commented Apr 22, 2022

dougbu commented Apr 28, 2022

HaoK Apr 28, 2022

Choose a reason for hiding this comment

HaoK commented Apr 28, 2022

HaoK commented Apr 28, 2022

dougbu commented Apr 29, 2022

HaoK left a comment

Choose a reason for hiding this comment

HaoK Apr 29, 2022

Choose a reason for hiding this comment

dougbu Apr 29, 2022

Choose a reason for hiding this comment

dougbu commented Apr 30, 2022