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

Make engine easier to Debug #551

Closed
CharliePoole opened this issue Feb 21, 2019 · 39 comments · Fixed by #957
Closed

Make engine easier to Debug #551

CharliePoole opened this issue Feb 21, 2019 · 39 comments · Fixed by #957
Assignees
Milestone

Comments

@CharliePoole
Copy link
Contributor

I'm phrasing this as a set of questions. Depending on the answers, it might become a task.

When using the engine as a component of another runner via nuget, debugging is rather tricky.

  1. Is there a reason that the pdb files are not distributed with the nuget packages? That's done with the adapter, for example.

  2. If I build for debug, the version suffix changes in the assemblyinfo file. I'm wondering if use of "-dbg" at the end of the suffix is something folks are attached to or which seems like it would break anyone if we did it differently. I see a couple of alternatives
    2.1 Add -dbg or (Debug) to the name component name, making it a different package
    2.2 Don't change the name at all but just create a separate feed for debug builds.

  3. How do you feel about having separate build targets for the engine and console?

These questions could apply to the framework as well, but it's the engine that I'm trying to debug under the GUI right now.

@mikkelbu
Copy link
Member

I ran into the same issue as in 1. when I did some debugging at work. But I think that from 3.10 we will actually get PDBs in the packages (at least in the zip-files) - at least I did when executing the target with "package". I guess this is a sideeffect of #388, but have not examined it in any detail.

Regarding 2. I think we could do it differently without harming people, and I don't know if there is a standard way to do this (meaning having debug builds in nuget).

Regarding 3. I won't mind separate build targets, but I do not know much about the history in this area.

@CharliePoole
Copy link
Contributor Author

I'm using the current master, which includes #388 and I'm not seeing pdbs in the nuget packages.

@mikkelbu
Copy link
Member

You are right Charlie. I was sure that I checked that the output contained PDBs because I was about to create an issue for this a couple of weeks ago, but I did a final check and then I thought that it was already solved on master. But I must have made an mistake when I did the check.

@CharliePoole
Copy link
Contributor Author

Let me throw in another item that I think would make life easier:

  1. Allow specifying the productVersion as an argument to the build script. This would make it easier to create debug or release test builds for use with a third-party runner, without modifying the script or stepping on the pre-release prefixes used by NUnit.

@CharliePoole
Copy link
Contributor Author

So far, @mikkelbu has responded. I'd rather see some others comments before I put any effort into a PR. At the moment, based on Mikkel's feedback, I would be inclined to put in a PR that covered 1, 2 and 4.

@ChrisMaddock
Copy link
Member

I’d like to hear @jnm2’s thoughts on pdbs, after all the work he did in the framework here recently, I remember we discussed several options. We absolutely should have something!

4 is fine by me. 2, I have no strong opinions on - could you just clarify the problems this is currrently causing, for interests sake?

3 also, can you clarify what you’re suggestion? Doesn’t the cake script already cater for this?

@CharliePoole
Copy link
Contributor Author

@ChrisMaddock Enlarging on some of the points you mentioned.

  1. There are various more "modern" alternatives to distributing pdbs, including source packages and servers. People are continually reporting problems with the on SO and I've read several suggestions that providing the pdbs directly is currently the only bulletproof alternative. But @jnm2 may be more up to date on this than I am.

  2. When building locally, a debug build causes the AssemblyInfo.cs files to be rewritten. I have to then reset those three files.

  3. If I test or package anything, the cake script builds everything. Also, the cake script is broken down by type of package, not by what you are building. So, for example, I can run the target PackageNuget, which causes everything to be built and nuget packages to be built for everything.

It would be convenient to me to be able to just build the engine, just test the engine and just package the engine in a particular way. Of course, the overall build would still have to do everything but the individual commands would be broken down more than they now are. I could probably explain this better by just doing a PR. 😃

I would also find it convenient to be able to build for a particular target only. That may take some playing around to get it to work.

  1. In order to build, for example, nunit.engine.dll, version 3.10.0-tc-00002, for use with TestCentric while waiting for a release of NUnit that has the same fix, I now have to edit the config file, submit the build, wait till it passes and then remove the change from the config file. In my private copy, the productVersion is an argument, with the same default as currently, so I only have to run with the correct argument.

@ChrisMaddock
Copy link
Member

Thanks Charlie, all sound good to me.

I'd mis-remembered the Cake script, I thought we already had separate engine/console targets.

On pdb's - I think for the framework we include both pdbs and some SourceLink as well. Would be nice to get the engine to the same end point - but adding the pdb's would be a good incremantal step towards that!

@CharliePoole
Copy link
Contributor Author

The Cake targets are separate for test, but the build is unified and the packaging is broken down by package type rather than engine versus console.

I think I'm comfortable enough to do a PR at this point, which will probably drive out further comments.

@CharliePoole
Copy link
Contributor Author

I'm going to do each item as a PR, which will make them pretty small. That's how I'm currently working in the GUI and I really like it. WRT item 3, I'm not sure it possible without major changes to how the build is done, which I feel is out of scope for my role. I'll look more closely before deciding whether to drop it.

@rprouse
Copy link
Member

rprouse commented Mar 1, 2019

I think including the PDBs is a good idea. I'm not sure how useful they are if they aren't SourceLinked though.

For 2, I think it is fine to change although should we ever be publishing debug builds? For that, I usually publish the NuGet packages to a local folder that I have set up in Visual Studio as a package source.

For 3, I don't think there is any reason not to have multiple targets. I'm actually surprised we don't 😄

@CharliePoole
Copy link
Contributor Author

WRT 2, I think Rob has the right idea. If people want to test using a debug build, it shouldn't require a change in version, just in the feed from which they download. Of course, without package source it's only useful for those who have a copy of the source locally, but since that includes me, I'm happy.

@jnm2
Copy link
Collaborator

jnm2 commented Mar 10, 2019

There are various more "modern" alternatives to distributing pdbs, including source packages and servers. People are continually reporting problems with the on SO and I've read several suggestions that providing the pdbs directly is currently the only bulletproof alternative. But @jnm2 may be more up to date on this than I am.

The current planning (summarized at https://github.com/NuGet/Home/wiki/NuGet-Package-Debugging-&-Symbols-Improvements) is to introduce a new .snupkg which contains only symbols and is uploaded to nuget.org in parallel with the .nupkg, and nuget.org will run a new symbol server which will be added by default in future Visual Studio versions. However, the most reliable solution at the moment continues to be exactly what Charlie says, putting the .pdb files in with the main package.

Wherever the PDBs end up going, there are generally three options:

  1. The most foolproof is embedding the source files directly in the PDBs. This is what I have opted to do in all personal and work projects. In this day and age, disk space is no object IMO. This can be done in recent SDK versions simply by adding <EmbedAllSources>true<EmbedAllSources/> to the project file. VS2017 or later is required for embedded sources to be available when debugging. The only configuration the user has to do is (of course) temporarily turn off Just My Code.

  2. SourceLink is a little less foolproof, requiring an internet connection to a website, githubusercontent.com in our case, which is capable of serving raw source files for a given commit over HTTPS. SourceLink works by putting a tiny sourcelink.json file in the PDB describing the pattern to use to build each URL. (https://github.com/dotnet/designs/blob/master/accepted/diagnostics/source-link.md#examples). VS2017 or later is required for SourceLink sources to be available when debugging. The user is prompted by Visual Studio before the first download. One of the options is to always download without prompting.
    SourceLink is what the adapter currently uses: https://github.com/nunit/docs/wiki/Adapter-Source-Stepping

  3. Source indexing, the least foolproof but the most compatible because VS2005 or later can take advantage of it. Source-indexing works by executing an arbitrary Windows cmd file embedded in the PDBs to obtain the file and point the debugger at it. There's a security risk here and users must opt in by turning on 'Enable source server support' in debug options without a prompt.
    Source indexing is what the framework currently uses: https://github.com/nunit/docs/wiki/Debugger-Source-Stepping

If you like, I can make sample .nupkgs to prepare a table of final sizes like I did for the other two: nunit/nunit#2421 (comment)

@jnm2
Copy link
Collaborator

jnm2 commented Mar 10, 2019

I do really want to get source-stepping enabled, by the way. Last week it would have saved me a number of minutes at least when I was investigating a reported issue with the adapter.

@CharliePoole
Copy link
Contributor Author

I'd like to get this issue closed one way or another! 😃

I handled items 2 and 4 in separate PRs, which leaves items 1 (PDBs) and 3 (separate builds of engine and console).

@jnm2 did a nice summary, with recommendations, about pdbs. The framework project initially used GitLink but switched to SourceLink early this year. It seems to me that this may be an area where the projects should not operate completely independently. Should we be using SourceLink for all projects? All key projects? @nunit/engine-team What do you think?

Several of us were surprised that the engine and console builds were not separate targets. That's because there's only a single solution and that's what we build. Question: Is it worth changing that? If so, we can take two approaches: either build a list of projects for the engine and another for the console or create separate solutions for the engine and console. Personally, I think it's too much trouble unless we intend to entirely separate the release of the engine from that of the console runner. Thoughts?

@mikkelbu
Copy link
Member

mikkelbu commented May 2, 2021

@jnm2 did a nice summary, with recommendations, about pdbs. The framework project initially used GitLink but switched to SourceLink early this year. It seems to me that this may be an area where the projects should not operate completely independently. Should we be using SourceLink for all projects? All key projects? @nunit/engine-team What do you think?

I think we should go for SourceLink and make it similar to how we do it for the framework project.

Several of us were surprised that the engine and console builds were not separate targets. That's because there's only a single solution and that's what we build. Question: Is it worth changing that? If so, we can take two approaches: either build a list of projects for the engine and another for the console or create separate solutions for the engine and console. Personally, I think it's too much trouble unless we intend to entirely separate the release of the engine from that of the console runner. Thoughts?

It also sounds like trouble for me - but I don't feel that strongly about this issue, as I cannot remember having noticed or been affected by this.

@ChrisMaddock
Copy link
Member

My thoughts:

  • Yes to SourceLink. But only because I understand that to be current best practice (based on Joseph's knowledge!) - I don't particularly think this is something that needs to be standardised across projects.

  • I personally have limited need for separate targets right now. I'd like to look in future at releasing the engine/console on different release cycles, but I'm not sure that's something we need to deal with right now.

@CharliePoole
Copy link
Contributor Author

So, based on that feedback, which happens to match my own opinion, I'll drop item 3 (separate builds) and take a look at using SourceLink. That way we can finish up the issue.

@ChrisMaddock I'm not generally a fan of standardizing across projects but it may be convenient here, since NUnit users may need to debug into both the engine and the framework. I'm going to do the same for the GUI runner.

Do you want to switch to SourceLink in the current master or for the 4.0 release?

@ChrisMaddock
Copy link
Member

I know relatively little about best practices here, but my understanding from Joseph’s explanation above is that snupkgs are the “new” way, and including in the nupkg is the old way.

If this is going to be a new feature for the engine, do we want to support old dev environments? I don’t feel strongly either way - but if we’re going to do one or the other, the current best practice feels preferable.

Charlie - do you know any more than I do about the pros/cons here? Like I say, packing symbols isn’t something I’ve ever done before.

@CharliePoole
Copy link
Contributor Author

I don't know any more than you... all I did was read the docs. Microsoft guidance says to not include the pdbs in the nupkg, which seems to me to make you dependent on a connection. Also, AFAIK snupkg files only work with nuget.org.

I know that @jnm2 has continued to work in this area with the Cake project and others. His comments above are three years old now, so things have likely changed. @jnm2 can you give some advice here?

One possibility is to commit what I have so far... pdbs in the distribution. That makes it easy for anyone who already has the source... contributors, extenders, ourselves... to debug a package. It doesn't help the general user community, but maybe that's another step.

@jnm2
Copy link
Collaborator

jnm2 commented May 11, 2021

I use snupkg with MyGet also. I would recommend using the snupkg format because the nuget.org symbol server is enabled in VS by default and therefore consumers have the least burden possible (assuming they are referencing a package on nuget.org). If they are referencing a package on MyGet, they'll need to paste in the url to the symbol server for your MyGet feed name. I have example directions for this at https://github.com/Techsola/InstantReplay#debugging-into-techsolainstantreplay-source.

The problem with placing .pdb files in the nupkg is that the .NET SDK no longer copies PDBs to build output, and so they are not automatically loaded when the consumer turns off Just My Code. To use them, the consumer must have foreseen this eventuality by putting an obscure line in her csproj to cause .pdb files to be copied from dependency packages to build output before running her app, or she must browse into %userprofile%/.nuget/packages/yourpackageid/matchingversion/lib/... for every package and version, and then deal with these old paths clogging up the symbol sources settings.

None of the above means you can't put .pdb files other places in addition to snupkg→symbol server, however you find useful for yourself and your users.

There is one other option which is to embed the PDB into your assemblies themselves with <DebugType>embedded</DebugType>. (Not to be confused with embedding source into the PDB content, which is an entirely independent option.) There's never any trouble loading the symbols then, but there are two downsides to this:

  • VS currently doesn't respect Just My Code for dependencies that have embedded PDBs. The user will break on exceptions and be taken deep into your code when she doesn't want that; she usually wants to be taken to the line in her code that called in to your library. This is an especially bad experience for assertion libraries where clicking on failing test results suddenly takes you deep in the bowels of the assertion library where the exception was thrown, with the usual way to opt out (Just My Code) not being respected. I consider this a VS bug and I hope it gets fixed.

  • You force everyone that depends on your library to redistribute your PDB content, usually increasing your library's contribution to the redistribution size by around 1/3. (And that's when you're not additionally choosing to embed source files in the PDB.)

Lastly, the decision between embedding all source or using SourceLink to download on demand from raw.githubusercontent.com is a setting entirely independent of all of the above.

@CharliePoole
Copy link
Contributor Author

@jnm2 I'm afraid I'm left even more confused! What about the approach that @rprouse took with the framework - using two different .nuspecs in order to include pdbs while using sourcelink at the same time. What does that accomplish?

@ChrisMaddock If we can't make an ultimate decision right now. I propose to merge what I have here so far. That will at least give some of us what we need. (I wrote the issue in the first place in order to help myself!)

@jnm2
Copy link
Collaborator

jnm2 commented May 12, 2021

@CharliePoole It can be confusing. There are two things to think about separately: symbols (PDBs), which allow debugging, and source, which allows seeing source code for what you are debugging. SourceLink is an optional addition to PDBs. It's a small JSON file inside the PDB that says "here's where to download source code from the internet." Even if you have the PDB loaded in Visual Studio, you need an internet connection each time you step into a new source file if you want to see the source code.

An alternative to SourceLink is embedding source files within the PDB. If you use embedded source, the .cs files are compressed and included inside the PDB content. Once the PDB file is loaded in Visual Studio, you don't need an internet connection each time you step into a new source file if you want to see the source code.

Both of these things are independent of how you decide to distribute the PDB content itself, which is what my previous comment talks about. My previous comment talks about "I have an assembly; now how do I get the PDB for it?" SourceLink doesn't come in to that question. SourceLink is one answer to a different question: "I have an assembly and I also have a PDB; now how do I get the source code for it?" Embedded source is another answer.

@jnm2
Copy link
Collaborator

jnm2 commented May 12, 2021

What about the approach that @rprouse took with the framework - using two different .nuspecs in order to include pdbs while using sourcelink at the same time. What does that accomplish?

The reason Rob used two .nuspec files is because he was creating a .snupkg file as well as a .nupkg file. It's rare and difficult to manage your own .nuspec these days compared to letting the .NET SDK generate the .nuspec based on your .csproj file, unless you're unlucky enough to be doing something the SDK doesn't support. If the NUnit framework was letting the SDK generate the nuspec, Rob would have put <SymbolPackageFormat>snupkg</SymbolPackageFormat> in the csproj instead of dealing with any .nuspec files, and then dotnet pack would have produced both a .nupkg file and a .snupkg file.

What having a .snupkg file accomplishes is to distribute your PDB content to symbols.nuget.org or MyGet symbol servers. When a .snupkg file is next to a .nupkg file, the same nuget push command sends them both.

None of this has to do with SourceLink though. Snupkg is a PDB distribution mechanism. SourceLink is not. SourceLink is a PDB addition that enables debuggers like VS to download source files on demand while debugging. You can use SourceLink without snupkg and snupkg without SourceLink, or both or neither. They aren't aware of each other.

@CharliePoole
Copy link
Contributor Author

@jnm2 Yes, I get the part about "two things." My confusion is that you need both, but when I pack the nuget file using -Sources true -SourcePackageFormat snupkg, only the snupkg includes pdbs.

If I understand correctly, restoring the nuget package gets the pdbs for the user, provided the snupkg is also pressent. What I don't get is why nunit.framework is forcing the nupkg to also contain pdbs, in spite of creating an snupkg. We talked about making the console consistent with the framework, but... that seems unnecessary... Am I missing something?

Also... what about other, non-nuget packages: chocolatey, the msi, the zip package? How would you recommend handling them?

You've experimented with this stuff, I know... if I want to run some experiments, do I need a clean VM or will a clean directory suffice?

Charlie

@CharliePoole
Copy link
Contributor Author

Ah... we crossed... i see the answer now about the framework approach.

I could see switching to use of the project properties. What do you think @ChrisMaddock . Of course we would still need to handle the non-nuget packages we distribute in some way but at least the nuget part would be simpler.

@jnm2
Copy link
Collaborator

jnm2 commented May 12, 2021

This might be a better summary than my first ones.

"How will people load my PDB?" Options:

  • Download it from a symbol server (easiest for consumers if you upload to symbols.nuget.org because that's searched by default in new versions of VS, and the easiest way to publish there if you don't manage your own nuspec is <SymbolPackageFormat>snupkg</SymbolPackageFormat>)
  • Embed it inside your .dll file (big gotcha for consumers with Just My Code, as mentioned; <DebugType>embedded</DebugType>)
  • Manually browse to .pdb files they downloaded from you

"How will people see my source code after they have loaded my PDB?" Options:

  • SourceLink (requires internet connection)
  • <EmbedAllSource>true</EmbedAllSource> (increases PDB size)

@jnm2
Copy link
Collaborator

jnm2 commented May 12, 2021

Oops, my messages crossed too. I'm just now seeing your two replies. 👍

If I understand correctly, restoring the nuget package gets the pdbs for the user, provided the snupkg is also present.

Not quite. With snupkg, the PDBs are not obtained until the debugger contacts a symbol server in an attempt to load symbols for your assembly while debugging. Restoring the package gets you only what is in the nupkg. The snupkg doesn't really come back; the PDBs inside it get indexed in a standard symbol server.

What I don't get is why nunit.framework is forcing the nupkg to also contain pdbs, in spite of creating an snupkg. We talked about making the console consistent with the framework, but... that seems unnecessary... Am I missing something?

Having PDBs in the main package is not normal if you are using a snupkg. This might come down to the fact that @rprouse is creating the nuspec by hand and starting from a nuspec that did include PDBs in the main package.

Also... what about other, non-nuget packages: chocolatey, the msi, the zip package? How would you recommend handling them?

I would put PDBs in the .zip download for sure or else have a separate .zip download just for them, just so they are easy to find if someone wants to grab them manually. I wouldn't distribute PDBs via chocolatey or MSI because 1) people who debug them will be able to use the symbol server and 2) I think it's a tiny minority of people who will want to debug things distributed via chocolatey/MSI, and adding the PDB to the download and install size isn't worth it to me subjectively.

You've experimented with this stuff, I know... if I want to run some experiments, do I need a clean VM or will a clean directory suffice?

A clean directory will probably work, but you might need to delete a PDB cache if you want to test resolving the same PDB more than once. Once resolved, VS will cache a PDB rather that trying symbol servers again.
image

Let me know if I missed answering something else.

@CharliePoole
Copy link
Contributor Author

Enough food for thought at least for now... 😃

Thanks!

@CharliePoole
Copy link
Contributor Author

@ChrisMaddock I'm pretty well convinced by @jnm2 's comments.

  1. I don't think we need PDBs in the MSI or Chocolatey distributions.
  2. I do think we need them for the console runners and the engine packages.

Of course, that means I'll have to undo some work for (1), but it's cool... a learning experience.

For the nuget packages, I'm inclined to stick with what we are already doing and look into switching to project properties as a future step. Personally, I'd rather try it with a simpler project first.

What do you think?

@rprouse
Copy link
Member

rprouse commented May 16, 2021

I was debugging a project referencing the latest release of the framework and stepping into the framework with the new snupkg was a great experience. We'll need to test how it works with the engine, but I highly recommend it.

@rprouse
Copy link
Member

rprouse commented May 16, 2021

What I don't get is why nunit.framework is forcing the nupkg to also contain pdbs, in spite of creating an snupkg. We talked about making the console consistent with the framework, but... that seems unnecessary... Am I missing something?

It took me a lot of trial and error because most of the docs are based on creating the packages using the project file. With the nuspec file, when you compile it, the PDBs end up in the snupkg and the DLLs in the nupkg. I tried two nuspec files, but it didn't work. Everything had to be in one and the compiler puts the correct files in the correct package.

@CharliePoole
Copy link
Contributor Author

Ah! Now I see why I was confused. I was looking at a clone of the framework, which was out of date and apparently had one of your experiments in it. That's why I kept asking about "two nuspecs." In fact, I've done similar experiments as you must have done and learned how nuget creates two packages from one file. That's barely documented at all.

@CharliePoole
Copy link
Contributor Author

@ChrisMaddock I think we have enough info to decide. I have this blocked because I don't feel it's my decision to make. Per @jnm2 and @robs input and my own comment of four days ago, I think we should...

  1. Remove the pdbs from the msi.
  2. Remove the pdbs from the chocolatey package.
  3. Leave the pdbs in the nuget.org packages (engine and console runner).
  4. Create snupkgs for the engine and console runner nuget packages.

However, I don't have strong feelings about it and I'll do as much or as little of it as you like.

@ChrisMaddock
Copy link
Member

This sounds good to me 👍 Did you have any thoughts on SourceLink vs embedding sources? SourceLink would be my preference from the info Joseph has provided I think, but I don't feel strongly.

Re: switching from nuspecs to project files, I agree we should do that separately. It would be good to do at some point though - another one for the "making maintenance easier" list!

@CharliePoole
Copy link
Contributor Author

CharliePoole commented May 16, 2021

OK... I'll move ahead then. I'll leave conversion to the project file for somebody else to pick up at a future point. I may have to leave this until the end of the week as I'll be away.

@CharliePoole
Copy link
Contributor Author

CharliePoole commented May 17, 2021

PR is ready to review. This is the last piece needed to complete the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants