Generate prebuilt report and include it in the intermediate nupkg (arcade-powered source-build) - #6121
Conversation
| Version="[%(ExactVersion)]"/> | ||
| Version="[%(SourceBuildIntermediateNupkgReference.ExactVersion)]"/> |
There was a problem hiding this comment.
I had to make this explicit to avoid MSB4096, which seems to me to be an MSBuild bug: dotnet/msbuild#5707. I don't think it's worth adding a comment (when you're just reading through the file, it doesn't seem distracting to me), but it looks odd in this diff.
|
@dagood, a heads up. :) we have removed $ git remote add dotnet https://github.com/dotnet/arcade
# delete entire line containing the unwanted license third line in C# code, which are part of your PR branch (HEAD to dotnet/master)
$ git show HEAD...dotnet/master --name-only --pretty="" :/*.cs |\
xargs grep -l "See the LICENSE file in the project root for more information." |\
xargs sed -i '' '/See the LICENSE file in the project root for more information/d'
# delete part of line containing license third line in rest of the files, which are part of your PR (HEAD to dotnet/master)
$ git show HEAD...dotnet/master --name-only --pretty="" |\
xargs grep -l "See the LICENSE file in the project root for more information." |\
xargs sed -i '' 's/ See the LICENSE file in the project root for more information.//g'
# middle grep is there to avoid autofixing of abnormal file-endings (and errors line `sed: RE error: illegal byte sequence`)after that it can be verified that there are no files containing the unwanted third line: $ git grep -l 'See the LICENSE file in the project root for more information'
# or without -l to see the file contents |
|
Interesting, thanks. Should probably put that info in #6111, since I'd expect this to happen a lot with that PR going in so quietly (as far as I can tell), and it would be good to have a common resource. |
89e3335 to
b3dd0a1
Compare
|
Sure, added a comment here: #6111 (comment). It was a bit different script than for the on-going PRs ( |
I'm going to remove the new test project, because tests are currently failing for me locally, even without my changes, so I can't efficiently look into the problems yet: #6126. No need to sort through this now. |
|
@markwilkie @mmitche PTAL, in particular around the new step I'm adding to |
| } | ||
|
|
||
| public override int GetHashCode() => ( | ||
| PackageIdentity, |
There was a problem hiding this comment.
Does PackageIdentity have an appropriate GetHashCode for this?
There was a problem hiding this comment.
This is the ref type over at https://github.com/NuGet/NuGet.Client/blob/dev/src/NuGet.Core/NuGet.Packaging/Core/PackageIdentity.cs, and it has some nondefault implementation, but I can't say for sure. Seems to have worked fine for us so far though.
| public const string CreationTimePropertyName = "BuildOutputPropsCreationTime"; | ||
|
|
||
| [Required] | ||
| public ITaskItem[] NuGetPackages { get; set; } |
There was a problem hiding this comment.
Can you include the expected metadata for this task item here?
| Directory.CreateDirectory(Path.GetDirectoryName(OutputPath)); | ||
|
|
||
| using (var outStream = File.Open(OutputPath, FileMode.Create)) | ||
| using (var sw = new StreamWriter(outStream, new UTF8Encoding(false))) |
There was a problem hiding this comment.
Is there a reason we're writing the xml text explicitly this rather using an XmlDocument or other such method?
|
@mmitche I intentionally didn't review the content of the CS files, they are just copied from source-build. Feel free to leave comments but in the interest of moving forward I think it's best if I don't address them now. (Edit: I filed dotnet/source-build#1730 to track this.) |
Add Microsoft.DotNet.SourceBuild.Tasks. Restore it in Tools.proj and use it from a build step proj. Move the intermediate nupkg generation to the build step so that the prebuilt report is available to insert into the int nupkg.
4335e91 to
8f1de11
Compare
|
Addressed comments. I also rebased to split out the changes I made to the files post-copy into a separate commit: b9a8d2c. There were also some ternary statements doing some redundant |
…cade-powered source-build) (dotnet#6121) # Conflicts: # Arcade.sln # src/Microsoft.DotNet.Arcade.Sdk/tools/Build.proj # src/Microsoft.DotNet.Arcade.Sdk/tools/DefaultVersions.props
…cade-powered source-build) (dotnet#6121) # Conflicts: # Arcade.sln # src/Microsoft.DotNet.Arcade.Sdk/tools/Build.proj # src/Microsoft.DotNet.Arcade.Sdk/tools/DefaultVersions.props
4 participants
Add
Microsoft.DotNet.SourceBuild.Tasks. This is separate fromMicrosoft.DotNet.Arcade.Sdkbecause the dependencies it takes on NuGet libraries are in danger of conflicting with what's loaded by Arcade already. See #6014. The tasks in this new package let us track prebuilts, for dotnet/source-build#1715.To use the tasks in
Microsoft.DotNet.SourceBuild.Tasks, we restore that package inTools.projif we're running source-build, and then use it from a new build stepAfterSourceBuild.proj. (Since we're relying onExecuterestoringTools.proj, we now need to add a build step rather than doing everything inBeforeTargets=Execute.)AfterSourceBuild.projonly runs if'$(ArcadeBuildFromSource)' == 'true', so I believe these changes will have no effect on existing infra.I also moved the intermediate nupkg generation from a
BeforeTargets="Execute"target toAfterSourceBuild.proj, so I could include the prebuilt report generated inAfterSourceBuild.projin the intermediate nupkg. This is for dotnet/source-build#1725.I included a test project for the new tasks. Currently there's only a tiny test to justify the project's existence. The tasks are simply copied from dotnet/source-build, so there isn't any significant test coverage yet. I want to make sure we have a test project ready so we don't have a barrier to entry to adding regression tests and such in the future.