Build TraceEvent NuGet package #229

sharwell · 2017-05-21T02:02:45Z

Build the Microsoft.Diagnostics.Tracing.TraceEvent NuGet package with the TraceEvent project.
Attach the NuGet package to the set of AppVeyor artifacts.

sharwell · 2017-05-21T02:03:34Z

src/TraceEvent/Microsoft.Diagnostics.Tracing.TraceEvent.nuspec

+      Also see the TraceEvent Samples NuGet package for example uses of this library.
+    </description>
+    <summary>TraceEvent is a .NET Framework library for capturing and analyzing ETW events.</summary>
+    <releaseNotes>https://github.com/Microsoft/perfview/releases/tag/V$version$</releaseNotes>


💡 This link is much more maintainable than the previous approach.

sharwell · 2017-05-21T02:04:00Z

src/TraceEvent/Microsoft.Diagnostics.Tracing.TraceEvent.nuspec

+    <!-- User information -->
+    <!--<file src="License-Stable.rtf" target="License-Stable.rtf" />-->
+    <file src="ReadMe.txt" target="ReadMe.txt" />
+    <file src="ReleaseNotes.txt" target="ReleaseNotes.txt" />


💡 I would recommend removing this from the repository now that the project is on GitHub. It's an unnecessary maintenance burden.

Yes, you can remove these as well. In general we want only one place for information to be updated. Putting it in the releases page on GitHub is very reasonable.

sharwell · 2017-05-21T02:05:13Z

src/TraceEvent/Microsoft.Diagnostics.Tracing.TraceEvent.nuspec

+    <tags>TraceEvent EvenSource Microsoft ETW Event Tracing for Windows</tags>
+  </metadata>
+
+  <files>


📝 I intentionally left out the content files which are part of the current NuGet package. This is not a standard convention for NuGet packages and despite the intention makes it harder to adopt the package in a project.

I'm confused by this statement. content is a NuGet standard: https://docs.microsoft.com/en-us/nuget/schema/nuspec#including-content-files

It's a confusing standard that has changed behavior over time, which is probably enough to avoid using it though.

@rainersigwald What I mean is, the TraceEvent NuGet package currently adds its readme and getting started guide to the project whenever you install it into a project. This is allowed, but makes installing the package more burdensome when you have to go delete these files every time.

I agree we should not put docs in the package. All of that predated GitHub and easy web links. We have wanted to get rid of it for a while.

sharwell · 2017-05-21T02:05:54Z

src/TraceEvent/Microsoft.Diagnostics.Tracing.TraceEvent.nuspec

+<package>
+  <metadata minClientVersion="2.5">
+    <id>Microsoft.Diagnostics.Tracing.TraceEvent</id>
+    <version>0.0.0</version>


📝 This along with $version$ are automatically populated from the [AssemblyInformationalVersion] attribute value.

sharwell · 2017-05-21T02:06:46Z

src/TraceEvent/Microsoft.Diagnostics.Tracing.TraceEvent.targets

+***********************************************************************************************
+-->
+<Project xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
+  <Target Name="TraceEventInjectReference" BeforeTargets="ResolveAssemblyReferences" Condition="('$(DesignTimeBuild)' != 'true')">


❓ Why is this a build target? Can't we just place the <ItemGroup> directly in the file?

/cc @rainersigwald

Yes: since there's no glob expansion in the output directories of the project, these should be outside of a target.

Again, I would recommend starting with a completely vanilla file and build up from there.
I do not know the purpose of TraceEventInjectReference (I think it was there a bug fix for a scenario that we ultimately dropped). I think it is OK to re-learn why things were done by starting with something simple and understandable, and work up from there.

sharwell · 2017-05-21T02:08:46Z

src/TraceEvent/Properties/AssemblyInfo.cs

@@ -17,3 +17,6 @@
 [assembly: NeutralResourcesLanguage("en")]

 // [assembly: InternalsVisibleTo("PerfView")]
+[assembly: AssemblyVersion("1.0.42.0")]


📝 There are multiple approaches available which either eliminate the need to manually update these, or dramatically reduce the frequency of changes. If you describe your desired versioning behavior either @AArnott or myself will implement that approach as described or will propose changes in order to make things even simpler. If you are unsure of what you want, we can make recommendations.

This applies to the TraceEvent library along with each of the other assemblies build by this solution for distribution.

I would like to follow the simplest/most commonly used approach (to NOT be innovative). Ideally humans don't deal with version numbers much at all (only on major transitions).

Agreed. Which is exactly why a couple groups of people developed a NuGet package to establish a simple pattern you can follow and then basically forget about versioning except when you really want to bump the major (or minor) versions.
Check out Nerdbank.GitVersioning. Disclosure: I wrote it.

@vancem From what you have said, my versioning approaches target a different audience and are not likely what you want to deal with. My recommendation is to let @AArnott file a pull request later after this one is completed and merged which incorporates his versioning tools. They are as low maintenance as you can get.

codecov-io · 2017-05-21T02:46:01Z

Codecov Report

Merging #229 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #229   +/-   ##
=======================================
  Coverage   24.31%   24.31%           
=======================================
  Files         197      197           
  Lines      112419   112419           
  Branches    11211    11211           
=======================================
  Hits        27338    27338           
  Misses      85054    85054           
  Partials       27       27

Flag	Coverage Δ
#2015	`24.31% <ø> (ø)`	⬆️
#2017	`24.31% <ø> (ø)`	⬆️
#Debug	`15.78% <ø> (ø)`	⬆️
#Release	`14.85% <ø> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c94f22d...8907740. Read the comment docs.

rainersigwald · 2017-05-22T16:54:15Z

src/TraceEvent/Microsoft.Diagnostics.Tracing.TraceEvent.targets

+***********************************************************************************************
+-->
+<Project xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
+  <Target Name="TraceEventInjectReference" BeforeTargets="ResolveAssemblyReferences" Condition="('$(DesignTimeBuild)' != 'true')">


Yes: since there's no glob expansion in the output directories of the project, these should be outside of a target.

rainersigwald · 2017-05-22T16:54:39Z

src/TraceEvent/Microsoft.Diagnostics.Tracing.TraceEvent.targets

+***********************************************************************************************
+Microsoft.Bcl.Compression.targets
+
+WARNING:  DO NOT MODIFY this file unless you are knowledgeable about MSBuild and have


I find this header overly chatty, plus it doesn't make sense in a NuGet package since they aren't installed machine state. Plus the filename is wrong.

➡️ I copied this from the existing TraceEvent NuGet package without modification.

I can simplify the header when I simplify the content.

Note that the previous Nuget package should be viewed as experimental. It should probably not be used as a template if there are better templates available. Ideally we start with a completely vanilla nuspec and tweek it just enough to get what we want. The goals are

Simple and straightforward.

A resulting nuget package that current users can pick up very easily. (Ideally it is compatible, but even that is negotiable if we can get significant improvements by a modest breaking change).

We wire into existing infrastructure to get the package signed and or released.

The thing that makes this package a bit harder than normal is that it has native components. The solution in the past was to include all architectures for the native components in the one package, but we may wish to change to the mechanism used by other packages where you refer to one of several architecture-specific packages.

Our goal is to NOT be innovative, but simply follow a standard for doing what we need (access native components).

We may wish to split the package (symbol resolution, collection and processing are the obvious groupings). However I think we can defer that issue for now (for better or worse the are currently together and there is no reason to force this transition now).

Note that the previous Nuget package should be viewed as experimental. It should probably not be used as a template if there are better templates available. Ideally we start with a completely vanilla nuspec and tweek it just enough to get what we want.

I'm fine with that. I started with what existed to be less likely to "rock the boat". I'll work with @AArnott and/or @rainersigwald to recommend specific changes for a great long-term solution (dependable yet low maintenance).

The thing that makes this package a bit harder than normal is that it has native components.

My work was the foundation for libgit2sharp's native binaries distribution (libgit2/libgit2sharp#821), which has since been modified for improved cross-platform support (libgit2/libgit2sharp.nativebinaries). I plan to build on that work to the extent it relates to this project, but simplify it where it makes sense in cases that don't apply to us.

Our goal is to NOT be innovative, but simply follow a standard for doing what we need (access native components).

There's a reason I suggested the two people I suggested for helping. 😄

We may wish to split the package (symbol resolution, collection and processing are the obvious groupings).

This tends to cause more headaches than it solves. We should approach that if/when it becomes a clear requirement.

rainersigwald · 2017-05-22T16:58:41Z

src/TraceEvent/Microsoft.Diagnostics.Tracing.TraceEvent.nuspec

+    <tags>TraceEvent EvenSource Microsoft ETW Event Tracing for Windows</tags>
+  </metadata>
+
+  <files>


I'm confused by this statement. content is a NuGet standard: https://docs.microsoft.com/en-us/nuget/schema/nuspec#including-content-files

It's a confusing standard that has changed behavior over time, which is probably enough to avoid using it though.

rainersigwald · 2017-05-22T17:00:56Z

src/TraceEvent/packages.config

@@ -1,4 +1,6 @@
 <?xml version="1.0" encoding="utf-8"?>
 <packages>
  <package id="Microsoft.Diagnostics.Tracing.TraceEvent.SupportFiles" version="1.0.2" targetFramework="net45" />
+  <package id="NuGet.CommandLine" version="2.8.3" targetFramework="net45" />
+  <package id="Tvl.NuGet.BuildTasks" version="1.0.0-alpha002" targetFramework="net45" developmentDependency="true" />


Could also use NuGetizer-3000 for this for a "more supported" approach: https://github.com/NuGet/NuGet.Build.Packaging.

I don't have particular experience with that one, but I've been quite happy with the performance of these build tasks for cases where one NuGet package is created for the assembly contained in one C# project.

vancem · 2017-05-24T17:25:03Z

src/TraceEvent/TraceEvent.csproj

@@ -1,5 +1,6 @@
 <?xml version="1.0" encoding="utf-8"?>
 <Project ToolsVersion="14.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
+  <Import Project="..\..\packages\Tvl.NuGet.BuildTasks.1.0.0-alpha002\build\Tvl.NuGet.BuildTasks.props" Condition="Exists('..\..\packages\Tvl.NuGet.BuildTasks.1.0.0-alpha002\build\Tvl.NuGet.BuildTasks.props')" />


What is this for? Presumably it is a Nuget building thing?

It's added by a package that tells MSBuild how to convert the .nuspec into a .nupkg as part of the build. The source is here:

https://github.com/tunnelvisionlabs/NuGetBuildTasks

vancem · 2017-05-26T14:04:30Z

I want to look at the versioning scheme suggested in the conversation and take a look at the nuget package building utility over the weekend. We are setting precedent here.

sharwell · 2017-05-26T14:09:46Z

I want to look at the versioning scheme suggested in the conversation and take a look at the nuget package building utility over the weekend. We are setting precedent here.

No problem. Keep in mind that I made no changes to versioning as part of this PR, at least none that I was aware of. I was planning to address that separately.

The NuGet package building tool is the one we've been using for StyleCop Analyzers for a couple years and thousands of builds without hitting problems. It's lightweight and reliable from what I can tell. 😄

vancem · 2017-05-29T21:06:16Z

I have looked this over more careful. I do see that this change has nothing to do with version numbering.

My largest concern is that I want to be as 'boring' as possible, using the most standard way of creating a nuget package. This update uses the Tvl.NuGet.BuildTasks to do this building, which I do see is a very thin wrapper but it is still non-standard. I notice that there seems to be built in MSBuild support now with the target (see https://docs.microsoft.com/en-us/nuget/schema/msbuild-targets). I don't know what limitations it has, but it seems like the most 'boring' way, so I want to at least understand what drove us elsewhere.

If that built-in support is too simplistic, the https://github.com/NuGet/NuGet.Build.Packaging. also seems worthy of consideration because it comes from the Nuget team presumably is recommended by them (I would be interested in their official guidance on the matter).

Since most of the delta here has nothing to do with the Tvl.Nuget.BuildTasks and switching to something else should be very straightforward, I am not averse to merging this request as is (since we are making forward progress), but my goal would be to replace Tvl.Nuget.BuildTasks rather aggressively once we determine the 'boring' guidance we should be following.

vancem · 2017-05-29T22:04:17Z

The only thing blocking this is the merge conflict.

…downstream project

sharwell · 2017-05-29T23:39:04Z

The only thing blocking this is the merge conflict.

Fixed

This update uses the Tvl.NuGet.BuildTasks to do this building, which I do see is a very thin wrapper but it is still non-standard.

On the plus side, I went with one I know works. The reason the package is still "alpha" is because in the two years it's been available, despite thousands of users using it for various projects I never had a single negative report come back after my initial publish.

Build TraceEvent NuGet package

msftclas added the cla-already-signed label May 21, 2017

sharwell commented May 21, 2017

View reviewed changes

sharwell requested a review from vancem May 21, 2017 02:09

sharwell force-pushed the traceevent-nuget branch from 0e0bfd4 to 407e8ec Compare May 21, 2017 02:25

sharwell added the ReviewOnly Do Not Merge label May 21, 2017

rainersigwald reviewed May 22, 2017

View reviewed changes

vancem reviewed May 24, 2017

View reviewed changes

sharwell removed the ReviewOnly Do Not Merge label May 25, 2017

sharwell added 5 commits May 29, 2017 18:35

Build TraceEvent NuGet package

2128f78

Remove user information from NuGet package contents

b8e18d2

Use a .props instead of .targets for including native binaries

528b6c5

Avoid build errors if some binaries are intentionally omitted from a …

6f29818

…downstream project

Add missing Windows 7 binary

8907740

sharwell force-pushed the traceevent-nuget branch from 8a5e271 to 8907740 Compare May 29, 2017 23:35

vancem merged commit 88c3332 into microsoft:master May 30, 2017

sharwell deleted the traceevent-nuget branch May 30, 2017 02:09

sharwell modified the milestone: 1.9.55 May 31, 2017

vancem added a commit to vancem/perfview that referenced this pull request Dec 20, 2017

Merge pull request microsoft#229 from sharwell/traceevent-nuget

02abb89

Build TraceEvent NuGet package

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Build TraceEvent NuGet package #229

Build TraceEvent NuGet package #229

sharwell commented May 21, 2017

sharwell May 21, 2017

sharwell May 21, 2017

vancem May 24, 2017

sharwell May 21, 2017

rainersigwald May 22, 2017

sharwell May 23, 2017

vancem May 24, 2017

sharwell May 21, 2017

sharwell May 21, 2017

rainersigwald May 22, 2017

vancem May 24, 2017

sharwell May 21, 2017 •

edited

Loading

vancem May 24, 2017

AArnott May 25, 2017

sharwell May 25, 2017

codecov-io commented May 21, 2017 •

edited

Loading

rainersigwald May 22, 2017

rainersigwald May 22, 2017

sharwell May 23, 2017 •

edited

Loading

vancem May 24, 2017

sharwell May 25, 2017

rainersigwald May 22, 2017

rainersigwald May 22, 2017

sharwell May 24, 2017

vancem May 24, 2017

sharwell May 25, 2017

vancem commented May 26, 2017

sharwell commented May 26, 2017 •

edited

Loading

vancem commented May 29, 2017

vancem commented May 29, 2017

sharwell commented May 29, 2017

Build TraceEvent NuGet package #229

Build TraceEvent NuGet package #229

Conversation

sharwell commented May 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharwell May 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented May 21, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharwell May 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vancem commented May 26, 2017

sharwell commented May 26, 2017 • edited Loading

vancem commented May 29, 2017

vancem commented May 29, 2017

sharwell commented May 29, 2017

sharwell May 21, 2017 •

edited

Loading

codecov-io commented May 21, 2017 •

edited

Loading

sharwell May 23, 2017 •

edited

Loading

sharwell commented May 26, 2017 •

edited

Loading