Fix RunHelix script #34601
Fix RunHelix script #34601
Conversation
ghost
added
the
area-infrastructure
dougbu
left a comment
dougbu
left a comment
There was a problem hiding this comment.
This is a bit scary because the shared Fx is required for a number of projects in the repo. I'd feel better about this if the change
- really were specific to the RunHelix.ps1 script (
IncludeAspNetRuntime) is run for every Helix submission - was controlled by a property that's set using a new RunHelix.ps1 option to opt out of including our shared Fx in the correlation payload
This change effectively brings the script back to it's former state where it worked for tests that didn't need the shared Fx. The behavior regressed with the shared Fx work. If you need the shared fx there is a nice warning when running the script: If we add some opt-in or opt-out switch, that is extra work someone has to know to do. Whereas the current state it will just work for someone who wants the shared-fx or not (assuming they build the shared-fx if they need it, which is always the case if you need it) |
|
🆗 I buy what you said @BrennanConroy but I think you ignored
My concern here is somewhat separate. The RunHelix.ps1 script outputs the "nice warning" you mention but that won't happen if anything else hits the |
eng/helix/helix.proj
Outdated
| </ItemGroup> | ||
|
|
||
| <Target Name="IncludeAspNetRuntime" BeforeTargets="Gather"> | ||
| <Target Name="IncludeAspNetRuntime" BeforeTargets="Gather" Condition="'$(DoNotIncludeSharedFxHelix)' != 'true'"> |
There was a problem hiding this comment.
It's up to you but I was thinking of something more like
| <Target Name="IncludeAspNetRuntime" BeforeTargets="Gather" Condition="'$(DoNotIncludeSharedFxHelix)' != 'true'"> | |
| <Target Name="IncludeAspNetRuntime" BeforeTargets="Gather" | |
| Condition="'$(DoNotIRequireSharedFxHelix)' != 'true' OR | |
| EXISTS('$(RepoRoot)artifacts\packages\$(Configuration)\Shipping\Microsoft.AspNetCore.App.Runtime.$(TargetRuntimeIdentifier).$(SharedFxVersion).nupkg')"> |
Then RunHelix.ps1 wouldn't need a new [switch] and could pass /p:DoNotIRequireSharedFxHelix=true unconditionally.
There was a problem hiding this comment.
Separately, @HaoK could you remind me why the EXISTS('...') exists on line 90 of this file❔ We should avoid ordering issues and ensure the shared Fx packages exist before creating the correlation payload because it's created once for all projects under test. If the order isn't working right, tests that require the shared Fx e.g. ProjectTemplates.Tests.csproj will fail annoyingly late.
There was a problem hiding this comment.
My approach was sort of the backwards of what you are implying, I wanted to add targets that no-op or didn't fail since I believe things have morphed/transformed from how the targets were originally named, before we had a single option which included the runtime and did a lot of things, so lots of tests set the flag but they might not be building the shared Fx since only templates needed them. If we have more granular options now, its likely the exists is redundant now if this is only for the shared fx
There was a problem hiding this comment.
The EXISTS isn't redundant for Microsoft.AspNetCore.App.Ref because we hope not to service that package but @BrennanConroy please remove the one for Microsoft.AspNetCore.App.Runtime if you choose to update this PR again.
Don't require building the sharedfx in order to use RunHelix.ps1