CSHARP-2088: Publish PDBs and sources on symbolsource. #694

DmitryLukyanov · 2021-11-25T02:08:07Z

Some notes:

Currently I temporally switched back to myget for tests reasons, probably we will know that it works with nuget only during next release.
In the scope of this ticket I also fixed nuget package warnings from here and here. Steps that I took more or less taken from here.
After this PR, the nuget package manager should look like this:

NOTE: it looks like myget doesn't work correctly with the above package validation, all nuget packages that I checked (including external) were red. Probably it's only myget issue. Will check it after release :)

I needed to update cake to 1.3.0, to be able to configure ContinuousIntegration field that required to make package deterministic. Some article about it here.
What should be checked after package creating:

where Url means the git repository that will be used to download source file and Commit a particular commit (make sure that it's pushed).
How it should be configured:
5.1. While we use myget in testing, this source https://www.myget.org/F/mongodb/api/v3/index.json should be added to nuget package manager.
5.2. Consume the test package in the consumer application
5.3. Make sure that VS has the same configuration:

5.4. Make sure that myget symbol server https://www.myget.org/F/mongodb/api/v2/symbolpackage is added to VS here:

NOTE: I think when we will use nuget.org symbol server, it should be enough just to enable default nuget symbol server

JamesKovacs

LGTM! Very excited to see this feature enabled.

dotnet new console
dotnet add package MongoDB.Driver --version 2.14.0-csharp2088-4-1 --source https://www.myget.org/F/mongodb/api/v3/index.json

I configured JetBrains Rider to use the following Symbol Server as documented by myget.org:

https://www.myget.org/F/mongodb/api/v2/symbolpackage/

I then created a simple MongoDB program and was able to successfully debug into db.GetCollection<BsonDocument>("coll")! I confirmed that I had debugged into our actual sources by verifying that source comments were present along with our copyright. When working with decompiled sources, source comments aren't present (for obvious reasons) and rather than the copyright there is a "decompiled by" comment at the top of the file.

I had some initial fumbling around with configuration. Without explicitly adding the Symbol Server, you still get decompiled sources. So we will have to add some documentation on how to enable source debugging with our driver.

JamesKovacs · 2021-11-25T21:15:14Z

build.cake

 RunTarget(target);

+public class BuildData


BuildConfig might be a better name for this class.

JamesKovacs · 2021-11-25T21:17:09Z

build.cake

+            string s when s.StartsWith("test") && s.EndsWith("netstandard20") => "netcoreapp2.1",
+            string s when s.StartsWith("test") && s.EndsWith("netstandard21") => "netcoreapp3.1",
+            _ => null
+        };


I really like the centralization of this logic! It removes a lot of code duplication from our Cake script. 👍

JamesKovacs · 2021-11-25T21:17:58Z

src/MongoDB.Bson/MongoDB.Bson.csproj

@@ -27,6 +27,10 @@
    <PackageTags>mongodb;mongo;nosql;bson</PackageTags>
    <PackageLanguage>en-US</PackageLanguage>
    <IncludeSymbols>true</IncludeSymbols>
+    <SymbolPackageFormat>snupkg</SymbolPackageFormat>
+    <PublishRepositoryUrl>true</PublishRepositoryUrl>
+    <!--obj-->


What is this line?

it says that EmbedUntrackedSources is responsible for obj folder

There are few reasons:

Without this option, the package description triggers the warning:

Based on this article and this issue it looks like at least it affects AssemblyInfo.cs which is placed in obj\Release\{Framework}\. I didn't check further, but in this file we may apply global assembly attributes that could be missed? Anyhow it looks like this option is always recommended.

Also some article that recommends setting it.

JamesKovacs · 2021-11-25T21:22:55Z

src/MongoDB.Bson/MongoDB.Bson.csproj

+    <SymbolPackageFormat>snupkg</SymbolPackageFormat>
+    <PublishRepositoryUrl>true</PublishRepositoryUrl>
+    <!--obj-->
+    <EmbedUntrackedSources>true</EmbedUntrackedSources>


Should we be using this option? If a source file isn't in GitHub then there is a reason. And source files excluded from GitHub might contain secrets that we don't want shared. While I am not aware of any passwords or private keys embedded in our source, it seems like a risky option to enable. What are the consequences of setting this option to false?

it's required to have files in obj folder available in debugging, I need to refresh my memory to recall what will happen if we remove it

Discussed in Slack and decided that safer to not embed untracked files by default. We can revisit this decision if users report debugging problems.

JamesKovacs

Meant to select "request changes" on my last review. Minor questions/suggestions only. Overall it works as expected.

JamesKovacs

LGTM

BorisDog

Look great overall.
Confirmed that it works, exited to see this great enhancement for our debugging experience!

BorisDog · 2021-12-06T16:00:55Z

build.cake

+            string s when s.StartsWith("test") && s.EndsWith("netstandard20") => "netcoreapp2.1",
+            string s when s.StartsWith("test") && s.EndsWith("netstandard21") => "netcoreapp3.1",
+            _ => null
+        };


BorisDog · 2021-12-06T16:01:47Z

build.cake

+            Console.WriteLine("Build continuousIntegration is enabled");
+            settings.MSBuildSettings = new DotNetCoreMSBuildSettings();
+            // needs to make nupkg package deterministic. Should be used only during package release
+            settings.MSBuildSettings.SetContinuousIntegrationBuild(continuousIntegrationBuild: true);


Is it possible to always use SetContinuousIntegrationBuild and skip having IsReleaseMode )as we don't use cake for debug/dev env.)?

as we don't use cake for debug/dev env.)?

it sounds like it's possible, at least I wasn't able to reproduce any issues in our tests (including for tests that work with system paths). However given that it's recommended to set it only for non local development:

While deterministic builds are enabled by default in .NET SDK projects, there is an extra property, ContinuousIntegrationBuild, to set on the build server to normalize stored file paths. These should not be enabled during local dev or the debugger won’t be able to find the local source files.

I would probably prefer to leave this check (to fully follow the guide), however I can rely on you and @JamesKovacs here.

We only publish release mode assemblies. If you're in debug mode for the driver, you're debugging through your local sources. You don't need to reach out to a source server to download a nuget package containing PDB files because you've got the PDB files on your local file system already. We should leave this check in place IMHO.

Hashed out in Slack. Our decision was to use @BorisDog's suggested comment:
configure deterministic build for better compatibility with debug symbols (used in Package/Build tasks)

And add @DmitryLukyanov's note to the comment about whether nupkg or snupkg is affected by the specific call to SetContinuousIntegrationBuild.

BorisDog · 2021-12-06T16:08:41Z

build.cake

@@ -65,7 +66,7 @@ Task("Restore")

 Task("Build")
    .IsDependentOn("Restore")
-    .Does(() =>
+    .Does<BuildConfig >((buildConfig) =>


nit: extra space: <BuildConfig >

BorisDog · 2021-12-06T16:09:32Z

build.cake

@@ -798,8 +783,36 @@ Task("TestsPackaging")
    })
    .DeferOnError();

+Setup<BuildConfig >(


nit: extra space

BorisDog

LGTM!

CSHARP-2088: Publish PDBs and sources on symbolsource.

5fe0f04

DmitryLukyanov force-pushed the csharp2088_4 branch from 47944a2 to 5fe0f04 Compare November 25, 2021 02:09

DmitryLukyanov requested a review from JamesKovacs November 25, 2021 02:10

DmitryLukyanov added 2 commits November 25, 2021 10:56

CSHARP-2088: Small fix.

455bcc1

CSHARP-2088: Cake refactoring.

a80b63e

JamesKovacs requested a review from BorisDog November 25, 2021 18:20

JamesKovacs approved these changes Nov 25, 2021

View reviewed changes

JamesKovacs self-requested a review November 25, 2021 22:24

JamesKovacs requested changes Nov 25, 2021

View reviewed changes

CSHARP-2088: Code review.

f6f3e0a

JamesKovacs approved these changes Dec 6, 2021

View reviewed changes

BorisDog requested changes Dec 6, 2021

View reviewed changes

DmitryLukyanov requested a review from BorisDog December 12, 2021 18:47

DmitryLukyanov added 5 commits December 12, 2021 21:48

CSHARP-2088: Code review.

496540c

CSHARP-2088: Update comments.

a102e39

CSHARP-2088: Reset myget changes.

df390f2

CSHARP-2088: Remove EmbedUntrackedSources.

53fd7a6

CSHARP-2088: Comments rewording.

db56b84

BorisDog approved these changes Dec 13, 2021

View reviewed changes

DmitryLukyanov merged commit b5e9da2 into mongodb:master Dec 13, 2021

CSHARP-2088: Publish PDBs and sources on symbolsource. #694

CSHARP-2088: Publish PDBs and sources on symbolsource. #694

Uh oh!

Conversation

DmitryLukyanov commented Nov 25, 2021

Uh oh!

JamesKovacs left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JamesKovacs left a comment

Choose a reason for hiding this comment

Uh oh!

JamesKovacs left a comment

Choose a reason for hiding this comment

Uh oh!

BorisDog left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DmitryLukyanov Dec 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BorisDog left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DmitryLukyanov Dec 12, 2021 •

edited

Loading