Add support for additional DotNetRuntime #7158
Conversation
| <ItemGroup> | ||
| <HelixCorrelationPayload Include="dotnet-cli"> | ||
| <Uri>$(DotNetCliPackageUri)</Uri> | ||
| <Destination>$(DotNetCliDestination)</Destination> | ||
| </HelixCorrelationPayload> | ||
| <HelixCorrelationPayload Include="dotnet-runtime" Condition="$(IncludeDotNetRuntime) == 'true'"> |
There was a problem hiding this comment.
Not sure what Include=dotnet-runtime means here, it still ends up installing the runtime bits to the same dotnet-cli which is what I want so...
|
@MattGal starting point which seems to do the right thing, I end up with both versions in the payload/correlation directory as I want: |
src/Microsoft.DotNet.Helix/Sdk/tools/dotnet-cli/DotNetCli.props
Outdated
Show resolved
Hide resolved
|
Could use some msbuild help here with batching, I'm trying to get the outputs of the task to go into an item list and then add it to the correlation payload in a batch, its not happy with my mangling of msbuild C:\Github\arcade\src\Microsoft.DotNet.Helix\Sdk\tools\dotnet-cli\DotNetCli.targets(15,5): error MSB4096: The item "C:\Github\arcade\tests\archivepayload.zip" in item list "HelixCorrelationPayload" does not define a value for metadata "DotNetRuntimePackageUri". In order to use this metadata, either qualify it by specifying %(HelixCorrelationPayload.DotNetRuntimePackageUri), or ensure that all items in this list define a value for this metadata. [C:\Github\arcade\tests\UnitTests.proj] |
|
@HaoK taking a peek. (also: "_additiona -> _additional") |
|
I think the issue is that I'm not properly aggregating the output DotNetRuntimePackageUris, for the correlation payloads to batch. In the binlog it looks like each of those is getting stored into its own Item as opposed to building up a list of packageuris like I want |
|
Updated msbuild stuff
|
|
You've already updated again after I wandered off to make a self-contained sample. I am thinking something like this: |
|
Cool that makes sense, I'll try that, but assuming I don't refactor the FindDotNetCliPackage, how do I call it repeatedly and combine the Output from each call into a single list? That's the msbuild piece I'm stuck on. each call gives me a single uri, which i can attach to a property name or item, but how do I tell it to append to something? This call, I'd like to build up DotNetRuntimePackageUri to contain "url to version1;url to version 2" |
|
You don't need to append it to something, just add additional correlation payloads. |
|
It should be possible to add items in batches as well, but agree with @alexperovich . Note for batching in targets like my sample you are required to have outputs, but can totally ignore them. |
|
One question @MattGal what is the "SomeRuntime1" a friendly moniker? Why wouldn't that just be the version? Maybe? |
|
Also does it really make sense to mix runtimes like this? When would you want to have an arm64 runtime mixed with an osx for example? I would assume the rid should be shared. But i'll defer to you guys to determine if that makes sense |
Sure, you can use any string you want as an ITaskItem's identity, but for anything beyond the first string you use metadata as shown (or you could do something funky with string splitting... please don't). Identity is just a string and should strive to but doesn't have to be unique. |
|
I'm still struggling with how to add payloads given the task structure, the message thing works since its just printing, how do I add a payload though... Here's what I have where I tried to do the actual payload generation instead of the message which works fine, I am unsure how to batch against the list of DotNetCliPackageUri. My instinct is to just write a new task that takes a list of AddAdditionalRuntimes and does the right thing in C# to avoid this msbuild stuff, but I'm guessing there's an easy way to do this in msbuild? |
|
This binlog demonstrates the current issue I'm seeing, each invocation of the FindDotNetCli task overwrites the last output url, so it seems to be using the last package url instead of adding 3 payloads with different package uris
|
|
@HaoK that is happening because you aren't batching the target, you are using task batching because you put "%(AdditionalDotNetPackage.Identity)" in the FindDotNetCliPackage task invocation. You need to stick that in the Outputs of the target too for it to batch at the target level. What you have right now is equivalent to this pseudo code: If you stick the %(AdditionalDotNetPackage.Identity) into the Outputs of the target, then msbuild will switch to target batching and run the entire target once for each unique value of %(AdditionalDotNetPackage.Identity) |
|
Also, the target shouldn't have any Inputs, otherwise msbuild will try to do "up-to-date" checks and possibly not run parts of the target if files of the right names exist. |
|
Thanks @alexperovich that output seems to have done the trick, I tried the following additional payloads: |
|
Test run looks good to me, Input: Resulted in the expected versions on disk in the sdk.tests.dll: |
| <_package Condition=" '$(_package)' == '' ">runtime</_package> | ||
| </PropertyGroup> | ||
|
|
||
| <Message Text = "FindDotNetCliPackage '%(AdditionalDotNetPackage.Identity)' '$(DotNetCliRuntime)' '$(_package)' '$(_channel)' "/> |
There was a problem hiding this comment.
I think this is cruft from my example? If not, you could improve the text like "Adding package ..."
There was a problem hiding this comment.
Up to you, I found it nice for debugging, I can remove it or update the text, what do you prefer?
There was a problem hiding this comment.
If it's up to me I definitely favor logging it with normal verbosity, something like
<Message Text = "Adding correlation payload for additional .NET Core package: Version: '%(AdditionalDotNetPackage.Identity)' Runtime: '$(DotNetCliRuntime)' Type: '$(_package)' Channel: '$(_channel)' "/>
|
Can you add some content to https://github.com/dotnet/arcade/blob/main/src/Microsoft.DotNet.Helix/Sdk/Readme.md#all-possible-options about how this works / some examples? |
| @@ -152,6 +152,23 @@ Given a local folder `$(TestFolder)` containing `runtests.cmd`, this will run `r | |||
| <HelixPostCommands>$(HelixPostCommands);echo 'One Pepperoni Pizza'</HelixPostCommands> | |||
| </PropertyGroup> | |||
|
|
|||
| <!-- | |||
There was a problem hiding this comment.
@MattGal want to review the doc update too?
There was a problem hiding this comment.
Looks good, some suggestions:
- Including additional dotnet correlation payloads -> could be more verbose, e.g. "Optional additional dotnet runtimes or SDKs for correlation payloads"
- Where your examples say "the specified" it couldn't hurt to have (version that's actually in the example) for clarity.
- I would put parenthesis around defaults (e.g. "PackageType (defaults to runtime)")
| @@ -152,6 +152,23 @@ Given a local folder `$(TestFolder)` containing `runtests.cmd`, this will run `r | |||
| <HelixPostCommands>$(HelixPostCommands);echo 'One Pepperoni Pizza'</HelixPostCommands> | |||
| </PropertyGroup> | |||
|
|
|||
| <!-- | |||
There was a problem hiding this comment.
Looks good, some suggestions:
- Including additional dotnet correlation payloads -> could be more verbose, e.g. "Optional additional dotnet runtimes or SDKs for correlation payloads"
- Where your examples say "the specified" it couldn't hurt to have (version that's actually in the example) for clarity.
- I would put parenthesis around defaults (e.g. "PackageType (defaults to runtime)")
|
Hello @HaoK! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
|
Apologies, while this PR appears ready to be merged, I've been configured to only merge when all checks have explicitly passed. The following integrations have not reported any progress on their checks and are blocking auto-merge:
These integrations are possibly never going to report a check, and unblocking auto-merge likely requires a human being to update my configuration to exempt these integrations from requiring a passing check. Give feedback on thisFrom the bot dev teamWe've tried to tune the bot such that it posts a comment like this only when auto-merge is blocked for exceptional, non-intuitive reasons. When the bot's auto-merge capability is properly configured, auto-merge should operate as you would intuitively expect and you should not see any spurious comments. Please reach out to us at [email protected] to provide feedback if you believe you're seeing this comment appear spuriously. Please note that we usually are unable to update your bot configuration on your team's behalf, but we're happy to help you identify your bot admin. |
|
What's the best way for me to track when this will be available for us in dotnet/aspnetcore? |
|
@riarenas can correct what I get wrong here but I'll take a stab:
This will eventually flow via regular Arcade dependency flow automation, as well. |
|
@HaoK another way to do this would be a bit more inline with the regular promotion process
Main difference between that list and @MattGal's suggestion is it includes the promotion checks. Downside is it'll take longer. |
|
@dougbu since I'm OOF next week, will this likely have flowed into our repo in 2 weeks? |
|
This fix absolutely should flow into dotnet/aspnetcore by next Tuesday at the latest. We took an update in dotnet/aspnetcore#31349 yesterday and that's the only reason I am not confident the PR won't be created "naturally" on Sunday. |
|
Either way you do it is fine by me, but I was thinking about how once the package version is up on a feed, you can start experimenting with fitting its usage into the ASP.NET's Helix tests in a private branch faster, then land it once it promotes naturally or otherwise. |
|
Yeah I probably won't have time to play with it before I get back, hopefully it will be trivial (set the new versions in our helix proj), remove our invocations of dotnet-install from our helix bootstrappers and just trust that the correlation payload paths have the right bits... It 'should' just work, since we are basically doing the same thing today (installing sdk/runtime into a workitem directory) |
|
If it gets sent to Helix and then breaks, you can pretty much always expect me to help investigate regardless of what my boss wants. I will be OOF next week though. |
|
Whoa, how will the rest of us deal❕ Thinking about taking a few Wellness days to avoid the fallout. |
|
My changes are opt in, if someone else is brave enough to try it first, they are far braver than I would be :) |
|
/fyi we're at step 3 in my list. Promotion build is https://dev.azure.com/dnceng/internal/_build/results?buildId=1066875 |
|
/fyi I manually triggered dotnet/aspnetcore#31427 That brings in the Arcade build 20210331.6 and so contains your fix @HaoK |
- changes may help figure out dotnet/core-eng#11424 - will likely go away in the next Arcade update PR - if not, we should revert this change after dotnet/core-eng#11424 is done for realz FYI @MattGal I didn't find a way to add similar logging in tools.sh. Tried - adding `chmod +x "$install_script"` in `GetDotNetInstallScript` - changing commands to `bash "verbose=true \"$install_script\" ..."` in `InstallDotNet` but that didn't get the `say_verbose` or `eval $invocation` bits in the script going.
Cherry-pick of b61eaae Co-authored-by: Hao Kung <[email protected]>
Enables
Which will install both the sdk and the runtime into the same directory