Skip to content

Add build manifest#5210

Merged
chcosta merged 6 commits into
dotnet:masterfrom
chcosta:no-sign
Apr 10, 2020
Merged

Add build manifest#5210
chcosta merged 6 commits into
dotnet:masterfrom
chcosta:no-sign

Conversation

@chcosta
Copy link
Copy Markdown
Member

@chcosta chcosta commented Apr 6, 2020

  • Creates a build manifest during the build which later stages / jobs (or different pipelines) can consume.

  • Modifies signing validation to be an MSBuild task which can consume the manifest and is more extensible than the console tool previously used

  • Splits out many signing property defaults from the Arcade Sdk so they can be imported as needed

  • Replaces outdated Microsoft.Bcl.JsonSources with System.Text.Json

_UseBuildManifest in eng\common\variables.yml is currently set to False because we need to produce a version of the SDK with these changes before we can enable the build manifest.

Validated with https://dnceng.visualstudio.com/internal/_build/results?buildId=590601&view=results

@chcosta chcosta requested review from jcagme and mmitche April 6, 2020 20:54
@mmitche mmitche requested a review from JohnTortugo April 6, 2020 21:05
Comment thread eng/common/build.ps1 Outdated
Comment thread eng/common/sdk-task.ps1 Outdated
Comment thread src/Microsoft.DotNet.Arcade.Sdk/tools/Build.proj Outdated
Pack "true" to build NuGet packages and VS insertion manifests
Sign "true" to sign built binaries
Publish "true" to publish artifacts (e.g. symbols)
GenerateBuildManifest "true" to generate a build manifest during publish step. Requires "Sign" and "Publish" are also true so that signing metadata can be included in the build manifest.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

GenerateBuildManifest [](start = 4, length = 21)

I think we should generate the manifest when ContinuousIntegrationBuild == true

Comment thread src/Microsoft.DotNet.Arcade.Sdk/tools/Manifest.targets Outdated
Comment thread src/Microsoft.DotNet.Arcade.Sdk/tools/Publish.proj Outdated
Copy link
Copy Markdown
Member

@tmat tmat left a comment

Choose a reason for hiding this comment

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

🕐

Comment thread src/Microsoft.DotNet.Build.Tasks.Feed/src/GenerateBuildManifest.cs
@chcosta
Copy link
Copy Markdown
Member Author

chcosta commented Apr 8, 2020

Comment thread .gitignore Outdated
Comment thread src/Microsoft.DotNet.Arcade.Sdk/tools/Publish.proj Outdated
Comment thread src/Microsoft.DotNet.Arcade.Sdk/tools/Sign.proj Outdated
Copy link
Copy Markdown
Member

@tmat tmat left a comment

Choose a reason for hiding this comment

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

:shipit:

@tmat
Copy link
Copy Markdown
Member

tmat commented Apr 8, 2020

  AssetManifestPath="$(AssetManifestFilePath)" />

Is this going away in future? Seems like we have now two manifests. Do we need both?


Refers to: src/Microsoft.DotNet.Arcade.Sdk/tools/Publish.proj:145 in 67498d5. [](commit_id = 67498d5, deletion_comment = False)

@chcosta
Copy link
Copy Markdown
Member Author

chcosta commented Apr 8, 2020

Long term, that's a goal.

They're serving two slightly different purposes.

Build manifest - defines the artifacts produced by the build and metadata for how the build believes they should be used (signing info at the moment).

Asset manifest - defines the artifacts that were published externally and where they can be found.

There's some overlap here in the sense that the PushToAzureDevOpsArtifacts task could consume the build manifest which it would then use to produce the asset manifest.

I agree, it would be nice if there was one manifest (or they were named better), but I don't yet have an idea for how we could combine these.

@JohnTortugo @mmitche

@chcosta
Copy link
Copy Markdown
Member Author

chcosta commented Apr 8, 2020

Perhaps, if the build manifest contained the metadata and rather than using the asset manifest as the "here's what I actually did" document, we used the build manifest as the "here's what you should do" document and then later we trusted that it actually happened, then we could eliminate the asset manifest.

@mmitche
Copy link
Copy Markdown
Member

mmitche commented Apr 8, 2020

The asset manifest actually isn't quite that unfortunately. It doesn't define entirely what was done. It's a combo of "what should I do" and "what I did". The asset manifest is produced prior to publishing and contains a list of artifacts. But it also contains information for publishing that identifies where those artifacts should go if the build happens to be published. The big one there is for blobs. The azdo build artifacts have the files that got produced, but the asset manifest tells us what subpaths those artifacts should go live on if they happen to be finally published. Publishing defines the base endpoints (e.g. dotnetcli.blob.core.windows.net/dotnet) and then the relative paths are defined by the asset manifests

@JohnTortugo and I were talking about the possibility of getting rid of the asset manifest the other day. Much of what the asset manifest contains is duplicated in the BAR build info. But the asset manifest also contains info that is not duplicated, and potentially could change over time as we change publishing. In order to get rid of the manifest, you need to encapsulate everything in there today in BAR, and make it extensible enough such that changes to that info do not involve a database change every time. The xml based manifest is pretty perfect for that.

There's some overlap here in the sense that the PushToAzureDevOpsArtifacts task could consume the build manifest which it would then use to produce the asset manifest.

This actually could work really well. What if we called the build manifest in this case a "signing manifest" (since that's what it's primarily targeted at) and then that info gets added to the asset manifest like you say. We also add to the asset manifest, a version number. Let's call it version 2, where version one of the asset manifest is the current 3.x and 5.x builds.

This version of the asset manifest can be interpreted differently by the publishing infrastructure. For the most part, everything is identical. But in order to sign after the build, we'll need the signing info available, so we also need to archive the manifest itself (since builds will go away in AzDO after N days),

To recap:

So version 1 of publishing, the current state, knows nothing about adding signing info to the asset manifest, and version 2, which the new PushToAzureDevOps task will generate, will include the signing info from the provided signing manifest. Publishing will be altered to know about both asset versions. For version 1, it does what it does today. For version 2, it knows that the artifacts are not signed, will need to be signed later, and thus archives the manifest in blob storage or wherever for use by the post-build prep stages where signing is being moved to.

Comment thread eng/common-variables.yml Outdated
- task: PublishBuildArtifacts@1
displayName: Publish Build Manifest
inputs:
PathToPublish: '$(Build.SourcesDirectory)/artifacts/log/$(_BuildConfig)/manifest.props'
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

IMHO we really need a better name than just "manifest.props"

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

what do you think it should be?

Copy link
Copy Markdown
Contributor

@JohnTortugo JohnTortugo Apr 9, 2020

Choose a reason for hiding this comment

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

I'll post a more general observation in the PR.

Comment thread eng/common/templates/job/job.yml
Comment thread eng/common/templates/post-build/post-build.yml
Comment thread src/Microsoft.DotNet.Arcade.Sdk/Microsoft.DotNet.Arcade.Sdk.csproj
Comment thread src/Microsoft.DotNet.Arcade.Sdk/tools/SdkTasks/SigningValidation.proj Outdated
Comment thread src/Microsoft.DotNet.AzureDevOps.Build/src/GenerateBuildManifest.cs
Comment thread src/Microsoft.DotNet.AzureDevOps.Build/src/GenerateBuildManifest.cs Outdated
Comment thread src/Microsoft.DotNet.AzureDevOps.Build/src/GenerateBuildManifest.cs
Comment thread eng/common/templates/post-build/post-build.yml
@chcosta
Copy link
Copy Markdown
Member Author

chcosta commented Apr 8, 2020

Comment thread src/SignCheck/SignCheck/SignCheckTask.cs
Comment thread src/SignCheck/SignCheck/SignCheckTask.cs
options.InputFiles = inputFiles.ToArray();

LogVerbosity verbosity;
if(Enum.TryParse<LogVerbosity>(Verbosity, out verbosity))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

NIT: can inline the verbosity var declaration

Comment thread src/SignCheck/SignCheck/SignCheckTask.cs Outdated
Comment thread src/SignCheck/SignCheck/SignCheckTask.cs Outdated
@mmitche
Copy link
Copy Markdown
Member

mmitche commented Apr 9, 2020

@chcosta I'd like to get this merged into the asset manifest during the publish to BAR stage as outlined above (with a BAR manifest verison), rather than carrying along a separate manifest. What would it take to get that done?

@chcosta
Copy link
Copy Markdown
Member Author

chcosta commented Apr 9, 2020

Shouldn't be too difficult. I'm hoping to have this merged today but I think I need to make one more change to this pr before merging

@JohnTortugo
Copy link
Copy Markdown
Contributor

I fully agree with what @mmitche said. IMHO having two manifests isn't ideal.

I agree, it would be nice if there was one manifest (or they were named better), but I don't yet have an idea for how we could combine these.

I think we need to have a plan for this. What other information will this manifest contains in the future? Right now I see that it only contains signing information.

@chcosta
Copy link
Copy Markdown
Member Author

chcosta commented Apr 9, 2020

I think we all agree that two manifests isn't ideal.

@chcosta chcosta merged commit eead20d into dotnet:master Apr 10, 2020
@mmitche
Copy link
Copy Markdown
Member

mmitche commented Apr 28, 2020

@chcosta Did you open an issue to merge the manifests?

@chcosta
Copy link
Copy Markdown
Member Author

chcosta commented Apr 28, 2020

I asked @jonfortescue to open one, as he's working on that right now

@jonfortescue
Copy link
Copy Markdown
Contributor

@chcosta I am indeed working on it and I also forgot to open the issue to show that so i'll go do that now!

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.

5 participants