-
Notifications
You must be signed in to change notification settings - Fork 347
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add DeltaBuild into Arcade #12885
base: main
Are you sure you want to change the base?
Add DeltaBuild into Arcade #12885
Conversation
If @riarenas , @chcosta , @AlitzelMendez could take a look that'd be super helpful. Jan and I have been talking, and basically he'd like to add this builddelta tool to Arcade. It's not intended to be in the primary tool chain path, but available for those who want it. |
I lack the necessary context, so I'll ask:
|
src/Microsoft.DotNet.DeltaBuild/Microsoft.DotNet.DeltaBuild.csproj
Outdated
Show resolved
Hide resolved
I plan to start reviewing this early next week. |
We'd like to reuse the tool between multiple .NET repositories. Today, the tool is not distributed in any way. Source of the tool is built as a prerequisite before source of unrelated project is built. It's a new tool.
That's going to be myself, @mobratil, @RussKie and more broadly the .NET teams using it.
Absolutely. The tool works as it is now, but we wouldn't be able to catch any regressions if we decide to add or change the behavior. After we have tool published as a .NET tool, I'm going to add a test suite for it. |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
717a056
to
93f9394
Compare
I'd like to see an official test build with these changes, especially due to the workarounds for symbol publishing errors you are introducing that will only be exercised for real during official builds and not during PR builds. @RussKie has done this before, and the steps are basically to follow the validation steps in this document: https://github.com/dotnet/arcade/blob/main/Documentation/CorePackages/Publishing.md#validating-the-changes I'm not familiar with the vs script you are introducing into the repo. My sense is that it shouldn't be added in the PR as it's not related to this tool. Ideally we won't check this in without unit tests either, but that's not blocking as you mentioned you planned followup changes. |
src/Microsoft.DotNet.DeltaBuild/Microsoft.DotNet.DeltaBuild.csproj
Outdated
Show resolved
Hide resolved
|
||
## Results | ||
|
||
Depending on size of the solution and relative change, measured speedup can vary from 25% to 85%. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
93f9394
to
9747cf5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Few suggestions to the readme.
This comment was marked as resolved.
This comment was marked as resolved.
Also, more importantly, what else we need to do to get this merged? |
Co-authored-by: Igor Velikorossov <[email protected]>
Co-authored-by: Igor Velikorossov <[email protected]>
Co-authored-by: Igor Velikorossov <[email protected]>
Co-authored-by: Igor Velikorossov <[email protected]>
It's because the shas match between the public and internal repos, so Maestro will preference the public repo URI. In that case the default channel doesn't apply. I very rarely recommend using default channels for one-off testing like this. Instead, just do the following after the build is complete:
and wait for it to complete. |
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need a way to reference and invoke the tool.
I'm not exactly clear on how to do the first step - i.e., reference. The tool has to run (if opted in) before the build because it defines the subset of projects that get built.
One way could be via global.json msbuild-sdks
node, e.g.:
"msbuild-sdks": {
"Microsoft.DotNet.Arcade.Sdk": "8.0.0-beta.23322.2",
"Microsoft.DotNet.Helix.Sdk": "8.0.0-beta.23322.2",
+ "Microsoft.DotNet.DeltaBuild": "8.0.0-beta.23325.1"
}
...but it means we need to create DeltaBuildSdk. I don't know how this works though...
The other option is to add an unconditional reference to https://github.com/dotnet/arcade/blob/main/src/Microsoft.DotNet.Arcade.Sdk/tools/Tools.proj, e.g.:
<ItemGroup>
<PackageReference Include="Microsoft.DotNet.NuGetRepack.Tasks" Version="$(MicrosoftDotnetNuGetRepackTasksVersion)" Condition="'$(UsingToolNuGetRepack)' == 'true'" IsImplicitlyDefined="true" />
+ <PackageReference Include="Microsoft.DotNet.DeltaBuild" Version="8.0.0-beta.23325.1" IsImplicitlyDefined="true">
+ <PrivateAssets>all</PrivateAssets>
+ </PackageReference>
</ItemGroup>
...and then create DeltaBuild.props and DeltaBuild.targets that contain some targets that run before Build
(or Restore
?) target. Again this requires further thoughts and iterations.
|
||
<PropertyGroup> | ||
<OutputType>Exe</OutputType> | ||
<TargetFramework>$(NetCurrent)</TargetFramework> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By targeting the current TFM only we make the tool only usable by projects targeting .NET 8. Which isn't good. We need to be targeting something like net472.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had .NET Framework TFM but there was a build warning/error that a .NET Tool can only be .NET Core. Should we add .NET 7? Ideas?
@markwilkie @mmitche @riarenas is there someone we could we talk to learn how to best plug this tool into the Arcade SDK and the whole build process? |
I think the package ref in tools.proj, conditionalized based on whether the feature is enabled (this is important since it shouldn't be enabled by default) is a good way to start. |
I tried to do this locally by defining |
The internal vs. public channel isn't really about the repo being public or internal. It's about whether the artifacts can be public. For instance, we publish wpf-int to public channels, even though the source is internal. It's simply that Maestro has some logic in it that preferences the repo URI to be public, if the commit built is public. So it takes the AzDO URI + sha that was built officially, e.g. dotnet-extensions @ foosha, translates the URI to what the github equivalent would be, i.e. dotnet/extensions @ foosha, then attempts to do an anonymous get. If the URI 404's, then it says that the URI of the repo built was the original URI, otherwise it adds github. Default channels always preference a build's github uri over the azdo one rather than letting both go through so that we cannot mistakenly publish internal bits to public endpoints. A good example of how this could happen is this:
Instead in this case, the default channel for public repos always pulls from GitHub, but the commit doesn't show up on github. So the AzDO URI does not match the default channel and no sensitive bits are accidentally published. TLDR: It's really just about what the purpose of your bits are. You may assign any build to a channel based on whether you want the bits to be public or not. You need to take into account whether commit being built is public or not when you set up a default channel. However, it is simpler to just manually assign in these one off situations, because you may be pushing the commit you build to both github and azdo, or only azdo. I think we should document Option 3 |
This comment was marked as resolved.
This comment was marked as resolved.
<Project Sdk="Microsoft.NET.Sdk"> | ||
|
||
<PropertyGroup> | ||
<TargetFrameworks>net7.0;$(NetCurrent)</TargetFrameworks> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Arcade usually only target NetCurrent. Why do you need to support net7.0?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a dotnet tool, which needs to work for all kinds of solutions that may target the whole plethora of TFMs. The Arcade SDK isn't exclusive to .NET 8 projects and, for example, can be used to build .NET Framework projects.
https://github.com/microsoft/slngen tool is another example, it targets the whole specter of TFMs:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now, an attempt to install this tool fails:
C:\>dotnet tool install -g Microsoft.DotNet.DeltaBuild --add-source https://pkgs.dev.azure.com/dnceng/public/_packaging/general-testing/nuget/v3/index.json --prerelease
C:\Users\jagutt\AppData\Local\Temp\79bb3973-33dd-43b6-b8e8-78303e953957\restore.csproj : error NU1202: Package Microsoft.DotNet.DeltaBuild 8.0.0-beta.23325.1 is not compatible with net7.0 (.NETCoreApp,Version=v7.0) / any. Package Microsoft.DotNet.DeltaBuild 8.0.0-b
eta.23325.1 supports: net8.0 (.NETCoreApp,Version=v8.0) / any
The tool package could not be restored.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But why do you need only net7.0 then? net6.0 is currently the minimum supported TFM and is represented as an msbuild property: $(NetMinimum)
. net7.0 should probably also be replaced with $(NetPrevious)
.
To double check: