Skip to content
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

Open
wants to merge 18 commits into
base: main
Choose a base branch
from
Open

Add DeltaBuild into Arcade #12885

wants to merge 18 commits into from

Conversation

tekian
Copy link

@tekian tekian commented Mar 16, 2023

To double check:

Arcade.sln Outdated Show resolved Hide resolved
@markwilkie
Copy link
Member

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.

@tekian tekian requested review from RussKie and mobratil March 16, 2023 16:22
@riarenas
Copy link
Member

riarenas commented Mar 16, 2023

I lack the necessary context, so I'll ask:

  • What is the business goal of having this tool available through arcade instead of however it's distributed now?
  • Who is going to own the tool, including paying attention to issues related to it? Especially after the recent reorg I don't think we should pick up anything new for now, even if we would just be directing any issues to another team.
  • Are tests coming in future iterations?

@RussKie
Copy link
Member

RussKie commented Mar 17, 2023

I plan to start reviewing this early next week.

@tekian
Copy link
Author

tekian commented Mar 17, 2023

What is the business goal of having this tool available through arcade instead of however it's distributed now?

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.

Who is going to own the tool, including paying attention to issues related to it?

That's going to be myself, @mobratil, @RussKie and more broadly the .NET teams using it.

Are tests coming in future iterations?

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.

@tekian tekian marked this pull request as ready for review March 17, 2023 15:12
@RussKie

This comment was marked as resolved.

@RussKie

This comment was marked as resolved.

@RussKie RussKie force-pushed the main branch 3 times, most recently from 717a056 to 93f9394 Compare March 20, 2023 06:26
@tekian
Copy link
Author

tekian commented Mar 20, 2023

@riarenas With @RussKie 's help, we now have all 15 checks green. What should be our next step? 🙂

start-vs.cmd Outdated Show resolved Hide resolved
@riarenas
Copy link
Member

riarenas commented Mar 21, 2023

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.


## Results

Depending on size of the solution and relative change, measured speedup can vary from 25% to 85%.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tekian tekian closed this Jun 23, 2023
@tekian tekian force-pushed the main branch 2 times, most recently from 93f9394 to 9747cf5 Compare June 23, 2023 11:49
@tekian tekian reopened this Jun 25, 2023
RussKie
RussKie previously approved these changes Jun 26, 2023
Copy link
Member

@RussKie RussKie left a 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.

src/Microsoft.DotNet.DeltaBuild/README.md Outdated Show resolved Hide resolved
src/Microsoft.DotNet.DeltaBuild/README.md Outdated Show resolved Hide resolved
src/Microsoft.DotNet.DeltaBuild/README.md Outdated Show resolved Hide resolved
src/Microsoft.DotNet.DeltaBuild/README.md Outdated Show resolved Hide resolved
@RussKie

This comment was marked as resolved.

@RussKie
Copy link
Member

RussKie commented Jun 26, 2023

Also, more importantly, what else we need to do to get this merged?

@mmitche
Copy link
Member

mmitche commented Jun 26, 2023

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:

darc add-build-to-channel --id <BAR ID> --channel "General Testing"

and wait for it to complete.

@RussKie

This comment was marked as resolved.

@RussKie
Copy link
Member

RussKie commented Jun 27, 2023

Copy link
Member

@RussKie RussKie left a 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>
Copy link
Member

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.

Copy link
Author

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?

@RussKie
Copy link
Member

RussKie commented Jun 27, 2023

@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?

@mmitche
Copy link
Member

mmitche commented Jun 27, 2023

@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.

@RussKie
Copy link
Member

RussKie commented Jun 27, 2023

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 UseDeltaBuild=true property in a test project's Directory.Build.props and conditionalising the import in the Arcade's Tools.proj (in my local nuget cache), and it didn't work. The binlog was showing the property is set, however, that was too late for the Tools.proj eval, or it had no knowledge of that...

@mmitche
Copy link
Member

mmitche commented Jun 27, 2023

Right, thanks. I was kind of wondering whether the docs should have said "General Testing Internal". I think the docs should be updated, though there are three possibilities:

  • Option 1: Instead of "Create a branch on the Azure DevOps internal mirror of the repo that includes the pipeline changes." use the public repo. Then "General Testing" would work.
  • Option 2: Instead of "General Testing" specify "General Testing Internal".
  • Option 3: Remove default channel creation steps and use darc add-build-to-channel --id <BAR ID> --channel "General Testing" instead.

Which version do you prefer? I'm happy to update the docs.

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:

  • A dev has a security fix that they open in an AzDO PR, mistakenly pointing the target branch to release/5.0.
  • It merges and officially builds.
  • The default channel picks up the release/5.0 branch + AzDO URI and publishes public bits.

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

@RussKie

This comment was marked as resolved.

@RussKie RussKie mentioned this pull request Jun 27, 2023
<Project Sdk="Microsoft.NET.Sdk">

<PropertyGroup>
<TargetFrameworks>net7.0;$(NetCurrent)</TargetFrameworks>
Copy link
Member

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?

Copy link
Member

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:
image

Copy link
Member

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.

Copy link
Member

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants