Helix Restore Issue Mitigation #41311
Conversation
msftbot
bot
added
the
area-infrastructure
If CI validation works, great. Would still prefer you remove the duplication around CA2007
|
|
@TanayParikh to avoid that, set |
|
I still approve |
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 |
dougbu
force-pushed
the
taparik/fixHelixRestoreIssue
branch
from
8ac42bf
to
04e7adf
Compare
|
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). |
| @@ -19,14 +19,18 @@ static async Task Main(string[] args) | |||
| { | |||
| keepGoing = await runner.InstallDotnetToolsAsync(); | |||
| } | |||
| #if INSTALLPLAYWRIGHT | |||
|
|
|||
There was a problem hiding this comment.
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
|
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 |
|
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
force-pushed
the
taparik/fixHelixRestoreIssue
branch
from
04e7adf
to
5b14ca6
Compare
Good suggestion, especially since we hook into an Arcade |
| @@ -15,10 +17,14 @@ | |||
| <_TemplateProperties> | |||
| AspNetCorePatchVersion=$(AspNetCorePatchVersion); | |||
| DefaultNetCoreTargetFramework=$(DefaultNetCoreTargetFramework); | |||
| DotnetDumpVersion=$(DotnetDumpVersion); | |||
| dotnetefVersion=$(dotnetefVersion); | |||
There was a problem hiding this comment.
minor nit: casing of dotnetefVersion here is wonky compared to everything else like DotnetDump
There was a problem hiding this comment.
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).
|
Found what I think is the last bug in this PR.
|
No description provided.