Improve build tasks packaging #299
Conversation
sharwell
force-pushed
the
cleaner-build
branch
from
04e60da
to
a646c2d
Compare
Codecov Report
@@ Coverage Diff @@
## master #299 +/- ##
==========================================
- Coverage 87.1% 87.07% -0.03%
==========================================
Files 31 31
Lines 3033 3034 +1
==========================================
Hits 2642 2642
- Misses 391 392 +1 |
sharwell
force-pushed
the
cleaner-build
branch
3 times, most recently
from
d0233d8
to
99c539c
Compare
sharwell
changed the title
sharwell
force-pushed
the
cleaner-build
branch
from
158c97e
to
5fadd27
Compare
sharwell
changed the title
|
Marked as WIP while I rebase to account for recent merges. |
sharwell
force-pushed
the
cleaner-build
branch
from
7235806
to
c35018a
Compare
sharwell
changed the title
|
What's the reason that coverlet.msbuild cross-compiles against net472 but not coverlet.console? |
|
Wouldn't it also make sense to merge coverlet.msbuild and coverlet.msbuild.tasks into one folder? |
I haven't done any work to get coverlet.console working differently. This pull request is about making MSBuild-based instrumentation work on .NET Framework builds.
|
sharwell
force-pushed
the
cleaner-build
branch
from
1f6c7ea
to
e4b29b4
Compare
|
Oh right. Coverlet.Console is a global tool, that's why it never supported .NET Framework. |
|
@tonerdo as we now cross-compile coverlet.core and coverlet.template here I propose to undo the unnecessary coverlet.template project and move the template back into core. |
I do have a remaining concern that's difficult to validate prior to some of the unmerged pull requests. This pull request separates the instrumentation task according to host runtime (msbuild vs. dotnet msbuild). However, the target framework of the individual projects might also come into play, and I haven't yet verified all the combinations there. |
|
ok fine. Let's keep it in mind. |
sharwell
force-pushed
the
cleaner-build
branch
from
8c1ca3f
to
e3a5191
Compare
|
Any timeframe when this will be merged and released? I really need that this. 😀 |
|
@sharwell can you please resolve the conflicts |
|
@tonerdo can we please get this into master after the conflicts are resolved? would be great. |
|
@ViktorHofer I'll need to get some time to take a closer look at this after the conflicts are fixed. Been quite busy with work lately |
|
@tonerdo I fixed the conflicts |
| <!-- Build tasks should not be added to the lib folder. --> | ||
| <IncludeBuildOutput>false</IncludeBuildOutput> | ||
| <IncludeSymbols>true</IncludeSymbols> | ||
| <IncludeSources>true</IncludeSources> |
There was a problem hiding this comment.
Any reason we're including symbols and sources?
There was a problem hiding this comment.
It can improve the debugging experience if needed. We can alter the way this is included in a future PR if desired.
|
|
||
| if (relativePath.Contains("..")) continue; | ||
| var relativePath = Path.GetDirectoryName(document).Substring(Path.GetDirectoryName(key).Length + 1); |
There was a problem hiding this comment.
If this is indeed a functional workaround to using Path.GetRelativePath then we can make coverlet.msbuild be fully compliant with netcoreapp2.0 and net472 frameworks by making it target netstandard2.0 instead
There was a problem hiding this comment.
To my knowledge Coverlet worked quite well with net472 when the target was netstandard2.0. Only got issues when I changed it to netcoreapp2.0
There was a problem hiding this comment.
It is even working with net35
There was a problem hiding this comment.
@sharwell are you still able to this? If not, I'll just merge it in and make the necessary changes if that's alright with you
There was a problem hiding this comment.
Thanks for your help so far
There was a problem hiding this comment.
I'd prefer if it gets done in this PR. #301 might take a bit to get through and I want the next release (which I want to happen this week) to fix the .NET Framework issue
There was a problem hiding this comment.
I'm not sure I'll have time to make and test this modification. I'll try to get it done tomorrow morning though (produce one netstandard2.0 assembly, which is included twice in the resulting package under two different paths).
There was a problem hiding this comment.
No worries. I'll go ahead and merge this and effect the necessary changes. Thank you so much for all your work on this
|
Just wanted to chime in and say that I am trying to get code coverage setup for https://github.com/dotnet/winforms, and I am blocked until this gets in |
(optional) Use Nerdbank.GitVersioning to dramatically simplify the versioning process for the whole project(submitted as Use Nerdbank.GitVersioning #301)This change will allow the build-time instrumentation to work with both
dotnet msbuild(works already using .NET Core as the host runtime) andmsbuild(currently fails using .NET Framework as the host runtime). The strategy will be similar to the one I use for ANTLR, except much simpler because I don't need to account for very old hosts like MSBuild 4.0.I verified that c35018a produces the same output assembly that shipped as 2.5.0 on NuGet except for the following changes: