Try using helix sdk support directly again #23585

HaoK · 2020-07-01T21:12:48Z

eng/helix/helix.proj

HaoK · 2020-07-06T16:35:07Z

@Tratcher @jkotalik looks like all of the IIS functional tests are failing on this PR, but everything else is working, can you guys take a look and see if its obvious what's going on? The main difference in this PR is we switched to using the build in helix sdk support, so its possible some of the paths and side effects are slightly different, not sure if IIS functionals are relying on anything there...

HaoK · 2020-07-06T16:35:53Z

Direct link to a build with the failing tests, since I'm going to fix a build warning in the PR:

https://dev.azure.com/dnceng/public/_build/results?buildId=715506&view=ms.vss-test-web.build-test-results-tab&runId=22171954&paneView=debug

eng/helix/helix.proj

eng/helix/content/runtests.cmd

HaoK · 2020-07-09T23:27:10Z

Status of this PR is waiting for the arcade changes to propogate, and then rebase to see if things go green (NetCoreSDK isn't set right now so runs are all broken)

JunTaoLuo · 2020-07-21T17:35:45Z

What arcade changes is this waiting on? Is this blocked on #23978?

dougbu · 2020-07-21T17:41:24Z

Is this blocked on #23978?

Yes

JunTaoLuo · 2020-07-22T06:15:50Z

FYI this should be unblocked.

HaoK · 2020-07-22T16:57:32Z

Nice, with the new arcade changes, looks like the built in helix sdk support works for us now, marking as ready for review

dougbu

Change looks fine. Just not positive whether it improves one area.

dougbu · 2020-07-22T17:28:58Z

eng/helix/content/runtests.cmd

-set DOTNET_HOME=%HELIX_CORRELATION_PAYLOAD%\sdk
-set DOTNET_ROOT=%DOTNET_HOME%\%$arch%
+set DOTNET_ROOT=%HELIX_CORRELATION_PAYLOAD%\dotnet
+set DOTNET_HOME=%DOTNET_ROOT%


In our previous conversations about this change and about incorrectly changing the payload folders, I got the impression we were getting out from under the payload folders with this PR. Won't we hit leftover files still❔

I'm hoping this is fine and that I misunderstood you and @MattGal 😈

If we want to install the runtime to a different directory we can still do that in a different PR, that's orthogonal to using the sdk support

MattGal · 2020-07-22T17:32:36Z

eng/targets/Helix.targets

@@ -117,8 +117,8 @@ Usage: dotnet msbuild /t:Helix src/MyTestProject.csproj
        <TestAssembly>$(TargetFileName)</TestAssembly>
        <PreCommands>@(HelixPreCommand)</PreCommands>
        <PostCommands>@(HelixPostCommand)</PostCommands>
-        <Command Condition="$(IsWindowsHelixQueue)">call runtests.cmd $(TargetFileName) $(NETCoreSdkVersion) $(MicrosoftNETCoreAppRuntimeVersion) $(_HelixFriendlyNameTargetQueue) $(TargetArchitecture) $(RunQuarantinedTests) $(DotnetEfPackageVersion) Microsoft.AspNetCore.App.Runtime.win-x64.$(AppRuntimeVersion).nupkg Microsoft.AspNetCore.App.Ref.$(AppRuntimeVersion).nupkg $(HelixTimeout)</Command>
-        <Command Condition="!$(IsWindowsHelixQueue)">./runtests.sh $(TargetFileName) $(NETCoreSdkVersion) $(MicrosoftNETCoreAppRuntimeVersion) $(_HelixFriendlyNameTargetQueue) $(TargetArchitecture) $(RunQuarantinedTests) $(DotnetEfPackageVersion) Microsoft.AspNetCore.App.Runtime.win-x64.$(AppRuntimeVersion).nupkg Microsoft.AspNetCore.App.Ref.$(AppRuntimeVersion).nupkg $(HelixTimeout)</Command>
+        <Command Condition="$(IsWindowsHelixQueue)">call runtests.cmd $(TargetFileName) $(MicrosoftNETCoreAppRuntimeVersion) $(_HelixFriendlyNameTargetQueue) $(TargetArchitecture) $(RunQuarantinedTests) $(DotnetEfPackageVersion) Microsoft.AspNetCore.App.Runtime.win-x64.$(AppRuntimeVersion).nupkg Microsoft.AspNetCore.App.Ref.$(AppRuntimeVersion).nupkg $(HelixTimeout)</Command>


Suggestion: Typically this would be expressed like:
Condition=" '$(IsWindowsHelixQueue)' == 'true' "> or Condition=" '$(IsWindowsHelixQueue)' != 'true' ">

@MattGal that's true when the property defaults to empty. But, when a property is always set to true or false, using it directly as a bool is easier to read and 💯% supported in msbuild and dotnet msbuild.

aspnetcore/eng/targets/Helix.props

Lines 17 to 18 in 324a900

<IsWindowsHelixQueue>false</IsWindowsHelixQueue>

<IsWindowsHelixQueue Condition="$(HelixTargetQueue.Contains('Windows')) or $(HelixTargetQueue.Contains('windows'))">true</IsWindowsHelixQueue>

Pilchie · 2020-07-22T17:46:21Z

👀

This reverts commit 059fe39.

…24250)" This reverts commit d9049ec.

ghost added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Jul 1, 2020

HaoK commented Jul 2, 2020

View reviewed changes

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

MattGal mentioned this pull request Jul 6, 2020

Prevent double-import warning for Microsoft.Common.props dotnet/arcade#5747

Closed

HaoK mentioned this pull request Jul 6, 2020

Try using helix sdk payload support instead #20876

Closed

dougbu reviewed Jul 6, 2020

View reviewed changes

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

jkotalik reviewed Jul 6, 2020

View reviewed changes

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

HaoK linked an issue Jul 7, 2020 that may be closed by this pull request

[Helix] Need to stop permanently modifying path on windows #23575

Closed

dougbu reviewed Jul 8, 2020

View reviewed changes

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

HaoK force-pushed the haok/helix-cli branch from 9ecb260 to 47379c4 Compare July 8, 2020 04:21

dougbu mentioned this pull request Jul 10, 2020

Prepare for strict coherency #23227

Merged

HaoK added 14 commits July 22, 2020 00:18

Use cli support

99eec40

Update runtests.cmd

89f4e78

Update InstallDotNet.ps1

7fe916a

Update runtests.sh

8a17c83

Update helix.proj

bd61788

Update runtests.sh

8069366

Update ci.yml

e672b5b

Update helix.proj

f8803f4

Update helix.proj

32c8bd6

Update ci.yml

9afddd5

Update runtests.cmd

47e3e55

Update runtests.sh

234dd7e

Update InstallDotNet.ps1

91bcabf

Update InstallDotNet.ps1

89ca365

HaoK added 7 commits July 22, 2020 00:18

Set DOTNET_HOME to dotnet_root

f23e13a

Temp no warn to make sure things are good

b67e31e

Remove sdk version arg

c30bcf6

Remove sdkVersion

a228673

Remove sdk version

923927f

Remove sdk version

6c5dd32

Update helix.proj

324a900

HaoK force-pushed the haok/helix-cli branch from 0ecd20a to 324a900 Compare July 22, 2020 07:18

HaoK marked this pull request as ready for review July 22, 2020 16:57

dougbu approved these changes Jul 22, 2020

View reviewed changes

MattGal approved these changes Jul 22, 2020

View reviewed changes

HaoK merged commit 059fe39 into master Jul 22, 2020

HaoK deleted the haok/helix-cli branch July 22, 2020 17:50

HaoK added a commit that referenced this pull request Jul 23, 2020

Revert "Try using helix sdk support directly again (#23585)"

a086173

This reverts commit 059fe39.

ghost pushed a commit that referenced this pull request Jul 23, 2020

Revert "Try using helix sdk support directly again (#23585)" (#24250)

d9049ec

This reverts commit 059fe39.

HaoK added a commit that referenced this pull request Jul 24, 2020

Revert "Revert "Try using helix sdk support directly again (#23585)" (#…

b5f6df0

…24250)" This reverts commit d9049ec.

HaoK added a commit that referenced this pull request Jul 27, 2020

Revert "Revert "Try using helix sdk support directly again (#23585)" (#…

f6f51a1

…24250)" This reverts commit d9049ec.

HaoK added a commit that referenced this pull request Aug 11, 2020

Revert "Revert "Try using helix sdk support directly again (#23585)" (#…

f905c18

…24250)" This reverts commit d9049ec.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Try using helix sdk support directly again #23585

Try using helix sdk support directly again #23585

HaoK commented Jul 1, 2020

HaoK commented Jul 6, 2020

HaoK commented Jul 6, 2020

HaoK commented Jul 9, 2020

JunTaoLuo commented Jul 21, 2020

dougbu commented Jul 21, 2020

JunTaoLuo commented Jul 22, 2020

HaoK commented Jul 22, 2020

dougbu left a comment

dougbu Jul 22, 2020

HaoK Jul 22, 2020

MattGal Jul 22, 2020

dougbu Jul 22, 2020

dougbu Jul 22, 2020

Pilchie commented Jul 22, 2020

	<IsWindowsHelixQueue>false</IsWindowsHelixQueue>
	<IsWindowsHelixQueue Condition="$(HelixTargetQueue.Contains('Windows')) or $(HelixTargetQueue.Contains('windows'))">true</IsWindowsHelixQueue>

Try using helix sdk support directly again #23585

Try using helix sdk support directly again #23585

Conversation

HaoK commented Jul 1, 2020

HaoK commented Jul 6, 2020

HaoK commented Jul 6, 2020

HaoK commented Jul 9, 2020

JunTaoLuo commented Jul 21, 2020

dougbu commented Jul 21, 2020

JunTaoLuo commented Jul 22, 2020

HaoK commented Jul 22, 2020

dougbu left a comment

Choose a reason for hiding this comment

dougbu Jul 22, 2020

Choose a reason for hiding this comment

HaoK Jul 22, 2020

Choose a reason for hiding this comment

MattGal Jul 22, 2020

Choose a reason for hiding this comment

dougbu Jul 22, 2020

Choose a reason for hiding this comment

dougbu Jul 22, 2020

Choose a reason for hiding this comment

Pilchie commented Jul 22, 2020