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

Optimize MicroBuild Signing #165

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

Conversation

thsparks
Copy link

@thsparks thsparks commented Apr 25, 2022

After moving code signing inside the build, we've noticed a significant increase in build time and network issues causing build failures. This is happening because:

  1. Signing a file requires a network call, and can take some time and is subject to possible networking issues
  2. If a project gets rebuilt, we re-sign dlls even if CoreCompile was skipped because it's already been built before.
  3. Signing NuGet files after CoreCompile is seen by MSBuild as an "update" on the project inputs, which triggers re-compilation on projects when it isn't necessary. Then, once that re-compile happens, the output dlls also need to be resigned.

These all compound upon each other and lead to a significant increase in build time and a decrease in reliability.

We can combine a few workarounds to fix this:

  1. Ensure SignNugetFiles runs before CoreCompile, so build output still appears more recent than signed nugets.
  2. When signing files, drop a temp file to indicate that it has already been signed. Do not sign files if the temp file exists.

Workaround 2 is implemented slightly differently for NuGet files and Output files. For build output, we simply drop a consistently named file in the OutDir, but for nuget files, we use the path of the dll to create the temp file, because we don't necessarily know the package path at the time of creating the file.

Given the "creative" nature of this workaround, we need to have a conversation about whether it's worth actually checking this in, or if we want to push for changes in the MicroBuild tooling instead and wait for those.

…output files (signed by SignFiles) will still look "more recent" than the nuget files (signed by SignNugetFiles), which are considered inputs to the project. If we don't do this and SignNugetFiles runs first, the inputs look more recent than the outputs and the project will be rebuilt unnecessarily.

In order for this to work completely, we must also sign the file in the intermediate output path, since that is what is used for the comparison.
…revent duplicate signing. We cannot use the previous technique that waited for CoreCompile because Nuget signing now happens before then. We need nuget signing to happen first so the build output will still appear more recent than the nuget dlls (inputs to the project) after the build and signing is complete.
Copy link
Member

@jonathanpeppers jonathanpeppers left a comment

Choose a reason for hiding this comment

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

@pjcollins should the xamarin-android repo be the thing that signs these files?

xamarin-android-tools doesn't publish artifacts anywhere, so how would you consume the signed files in other repos?

@pjcollins
Copy link
Member

should the xamarin-android repo be the thing that signs these files?

XA does sign these files during post build signing, but there are other repos that need to sign these files as well. Most of those repos were set up to sign during the build, not after.

xamarin-android-tools doesn't publish artifacts anywhere, so how would you consume the signed files in other repos?

Some FilesToSign and NuGetFilesToSign item groups were added here and in androidtools to allow other products that consume these submodules to sign everything they ship during their build. Looking back on this, we maybe could have better handled this directly in the shipping repos with extra wildcards as needed, for instance - https://github.com/xamarin/debugger-vs/blob/ddb4231999227a3421624e3d25db97005dc0f245/src/Debugging.VisualStudio/Debugging.VisualStudio.csproj#L71-L90.

<Message Text="Removing signing complete indicator file because CoreCompile ran..." />
<Delete Files="$(SigningCompleteFile)" />
</Target>
</Project>
Copy link
Member

@pjcollins pjcollins Apr 26, 2022

Choose a reason for hiding this comment

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

I think this may be a bit complicated as far as this repo is concerned at least. There are no @(NuGetFilesToSign) items in this repo and the broken incremental build scenario you've described doesn't seem like it would happen here?

As far as I can tell the only file in this repo that declares content for signing is Xamarin.Android.Tools.AndroidSdk.csproj, I think we should start here instead:

<Target Name="GetFilesToSign" BeforeTargets="SignFiles" Condition=" '$(Configuration)' == 'Release' Or '$(Configuration)' == 'ReleaseWindows'">
<ItemGroup>
<FilesToSign Include="$(OutDir)\$(AssemblyName).dll">
<Authenticode>Microsoft400</Authenticode>
</FilesToSign>
<FilesToSign Include="$(OutDir)\**\$(AssemblyName).resources.dll">
<Authenticode>Microsoft400</Authenticode>
</FilesToSign>
</ItemGroup>
</Target>

Maybe we should remove this ItemGroup entirely, and upstream shipping repos should add new <FilesToSign Include="/path/to/*Xamarin.Android.Tools.AndroidSdk*.dll> items?

Alternatively, we could maybe rename the target to something like GetXATFilesToSign, remove BeforeTargets="SignFiles" and the Microsoft.VisualStudioEng.MicroBuild.Core package reference, and then have upstream shipping repos call or depend on this GetXATFilesToSign target in one of their projects that does signing?

Do either of those approaches seem reasonable?

Copy link
Author

Choose a reason for hiding this comment

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

You're right - we actually don't need the NuGet stuff here, at least not at the moment... (Here I was, thinking I'd just pick a submodule to send the PR as an example, and I picked the one that doesn't have any nugets :) Whoops!). I'll remove that.

We do still need a workaround to ensure the output assemblies don't get re-signed if compile isn't run, so those parts will have to stay.

Regarding the suggestions, I believe that would go against the current guidance from VSEng. At the moment, the recommendation is to sign in the specific project that produces the assembly, and if it's in a submodule, to sign in the submodule itself rather than in the repo that includes it (https://devdiv.visualstudio.com/DevDiv/_wiki/wikis/DevDiv.wiki/27099/Guidance-for-Xamarin-MicroBuild-Signing-Migration?anchor=handling-submodules).

We can have a conversation w/ VSEng about whether that's the right approach (there's already a thread in the VSEng teams channel), but so far that has not amounted to much...

Copy link
Member

@pjcollins pjcollins Apr 26, 2022

Choose a reason for hiding this comment

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

Thanks I missed this conversation in Teams, lets continue the discussion on how to best move forward there or on https://github.com/xamarin/androidtools/issues/331

/>
Condition=" Exists('$([System.IO.Path]::GetDirectoryName($(MSBuildThisFileDirectory))).override.targets') "/>

<Import Project="RedundantSigningWorkarounds.targets" />
Copy link
Member

@pjcollins pjcollins Apr 26, 2022

Choose a reason for hiding this comment

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

If we do choose to go this route I think we should only import this in the projects that declare @(FilesToSign) items, we don't need every project in the repo to import this.

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.

None yet

3 participants