-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Conversation
@ctaggart, |
@@ -8,6 +8,7 @@ | |||
using Microsoft.AspNetCore.Razor.Evolution.Intermediate; | |||
using Microsoft.AspNetCore.Razor.Evolution.Legacy; | |||
using Xunit; | |||
using Microsoft.AspNetCore.Mvc.TestCommon; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not needed. I can remove.
@@ -38,7 +38,7 @@ public class TagHelpersTest : | |||
[InlineData("About")] | |||
[InlineData("Help")] | |||
[InlineData("UnboundDynamicAttributes")] | |||
[InlineData("ViewComponentTagHelpers")] | |||
// [InlineData("ViewComponentTagHelpers")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only test I couldn't get to pass on Windows when the file is lf
line endings.
Travis CI failed 1 of 2 tests. It looks like dev branch has been failing for 4 days. |
.travis.yml
Outdated
@@ -29,4 +29,4 @@ branches: | |||
before_install: | |||
- if test "$TRAVIS_OS_NAME" == "osx"; then brew update; brew install openssl; ln -s /usr/local/opt/openssl/lib/libcrypto.1.0.0.dylib /usr/local/lib/; ln -s /usr/local/opt/openssl/lib/libssl.1.0.0.dylib /usr/local/lib/; fi | |||
script: | |||
- ./build.sh | |||
- ./build.sh |tee /dev/null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a change merged in from #5906 which appears to fix the Travis CI build.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah Travis has limits on how much log output it'll take... and MVC's build produces waaaaaay too much output 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mvc is feeling a bit defensive here 😺. It's msbuild
and the problem (dotnet/msbuild#1792) doesn't just affect Mvc.
tee
ing is a viable workaround. But, we shouldn't check this in to a single .travis.yml file. My aspnet/KoreBuild#192 will do this everywhere. Yeah, kind-of like @Eilon suggested we might want to enable source link support.
Builds are passing using SourceLink 2.0.0. 👍 |
Very cool stuff! I'm glad the file sizes don't get too big, either! I'll need to discuss this with a few folks on the team to figure out the best way to apply this. E.g. do we just update the Also FYI due to a few people being on vacation who I'll need to consult regarding this, it'll be ~2-3 weeks until we can commit something like this. Thanks so much! |
@Eilon I'm not on vacation, I give this 4 thumbs up 😜 |
@davidfowl glad to hear it, but you're not the droid I'm looking for 😄 |
That AppVeyor build failure looks bogus. Just have it retry. It built fine here: This pull request is extremely simple now. This is how we just did it for a couple of other projects. Rx.NET was one. @dougbu, your concerns should be addressed. It should pass review for you now. |
@elion the suspense is killing me. 🌋 |
@ctaggart sorry for the delay! We're having a big discussion on what kind of PDBs to ship and how to ship them. Apparently the world of PDBs is far more complex than any of us had thought because different debugging tools have different requirements. We'd love to be able to get all the right PDBs to everyone, but we're not sure the best way to do that. |
@Eilon Is there anything beyond what is already documented? |
On top of that there are various tools that only consume some of the above pdbs. For example windbg doesn't support portable pdbs (AFAIK), but we need them for cross platform debugging. Other tools don't support embeddd portable pdbs yet even though they support portable pdbs. It's all actually pretty crazy. Somebody is now tasked with figuring out how we ship these for .NET Core all up. IMHO we need a good default experience (which I would say is embedded portable pdbs) and ways to get the other pdbs via other channels. |
I just released SourceLink 2.1.0. I added the ability to test the nupkg. Trying out an artifact from the latest commit, it passes.
|
Two more weeks have gone by. Is there a plan that can be shared? I see source link support is coming to Windows PDB files. Does that fit in a bigger picture? I like the idea of the default experience being embedded pdbs. |
/cc @tmat |
We are still having discussions on this topic. The current thinking, which is by no means final:
Makes sense? |
@tmat, I think that is a wonderful plan! Thank you for sharing. @Eilon, when is the appropriate time to move forward with enabling source link for aspnet projects? What parts of that plan need to be put in place before this can proceed? Enabling source link will benefit aspnet developers as soon as you enable it. Should aspnet use embedded pdb files? If yes, this pull request is ready to go. If not, is there an issue open so that by default dotnet pack includes Portable PDBs in the nupkg? NuGet/Home#4142 is related. cc @rohit21agrawal |
Hi @ctaggart we're still discussing the PDB story for .NET Core, ASP.NET Core, and related projects. We probably won't use embedded PDBs at this time, but a lot of the details are still a bit fuzzy. |
@Eilon, so the next steps should probably be helping get NuGet/Home#4142 pushed along, so that it is easy to add the portable pdb files to the nupkg's using the |
@ctaggart for the Preview1 release that we're finishing up, the PDBs won't be in the NUPKGs either, but we will upload them to the Microsoft symbol server instead. So I don't think NuGet/Home#4142 is immediately urgent at least for the NUPKGs built here. |
SourceLink 2.2 enables support for Windows PDB files too. There shouldn't be any blockers left with VS 15.3 out. You can try it out with https://www.nuget.org/packages/SourceLink.Create.CommandLine/2.2.0-b502. Closing this PR after 5 months. If you want help getting it to work, let me know. |
To those following along, I logged aspnet/Universe#553 to get this done once and for all for all of ASP.NET/EF Core. I apologize for the long delays in this; we're now at a good point where this has bubbled up to be a high priority item so we'll get this done ASAP. |
It is finally working! To try it out, add this package feed:
https://ci.appveyor.com/nuget/ctaggart-aspnet-mvc/
I simply did the web api template and updated the Microsoft.AspNetCore.Mvc package.
I was then able to step into the actual source code!
Here are my notes on files sizes:
Microsoft.AspNetCore.Mvc.Core.1.2.0-preview1-t00416d6cb\lib\netstandard1.6\Microsoft.AspNetCore.Mvc.Core.dll
475 KB without pdb embedded
Microsoft.AspNetCore.Mvc.Core.1.2.0-preview1-t00416e8a6\lib\netstandard1.6\Microsoft.AspNetCore.Mvc.Core.dll
576 KB with pdb embedded
21% increase
580 KB with source link and embedded source (several content files not in git).
You can see a list of all the nupkg files here:
https://ci.appveyor.com/project/ctaggart/mvc/build/12/artifacts
Totally worth it. It also eliminates building the symbols packages completely.
aspnet/Universe#131