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

Include Microsoft.Diagnostics.FastSerialization in the TraceEvent package #273

Merged
merged 3 commits into from
Jul 13, 2017

Conversation

sharwell
Copy link
Member

@sharwell sharwell commented Jun 5, 2017

Currently the Microsoft.Diagnostics.Tracing.TraceEvent assembly depends on Microsoft.Diagnostics.FastSerialization, but no dependency was added to the NuGet package. Rather than deal with separate NuGet packages at this time, this change simply includes the dependency in the original NuGet package.

📝 If we ever need to split these in the future, we can trivially do so and add a dependency from the current NuGet package to the new one without breaking upgrade scenarios. Since this makes distribution a little bit more complex (managing multiple NuGet releases from one repository) and didn't seem essential at this time, I chose the simplest route to packaging for my initial proposed approach.

@codecov-io
Copy link

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #273   +/-   ##
=======================================
  Coverage   24.35%   24.35%           
=======================================
  Files         197      197           
  Lines      112394   112394           
  Branches    11210    11210           
=======================================
  Hits        27373    27373           
  Misses      84994    84994           
  Partials       27       27
Flag Coverage Δ
#2015 24.35% <ø> (ø) ⬆️
#2017 24.35% <ø> (ø) ⬆️
#Debug 15.81% <ø> (ø) ⬆️
#Release 14.9% <ø> (ø) ⬆️

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 3844164...78487b6. Read the comment docs.

@vancem
Copy link
Contributor

vancem commented Jun 12, 2017

This is reasonable, but I am mulling over whether we event want a separate DLL for the FastSerialization code (what makes this different from the other utility code that happens to be in TraceEvent.dll (e.g. PEFile ...). This was originally spit out to allow reuse by Visual Studio, but in retrospect, that was not a clear win. Hmm...

@sharwell
Copy link
Member Author

@vancem If you aren't sure, we could file that as a follow-up issue to take on separately.

@vancem
Copy link
Contributor

vancem commented Jul 13, 2017

I have been mulling this too long. It is now blocking someone, and so we will do it this way and we can change it later as a separate issue.

@vancem vancem merged commit 3dde977 into microsoft:master Jul 13, 2017
@sharwell sharwell deleted the package-fastserialization branch July 13, 2017 16:32
vancem added a commit to vancem/perfview that referenced this pull request Dec 20, 2017
Include Microsoft.Diagnostics.FastSerialization in the TraceEvent package
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.

4 participants