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

Enable source link #1746

Merged
merged 9 commits into from Jun 24, 2018
Merged

Enable source link #1746

merged 9 commits into from Jun 24, 2018

Conversation

JamesNK
Copy link
Owner

@JamesNK JamesNK commented Jun 22, 2018

No description provided.

@JamesNK
Copy link
Owner Author

JamesNK commented Jun 22, 2018

C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\MSBuild\15.0\Bin\Roslyn\Microsoft.Managed.Core.targets(90,5): error : SourceRoot items must include at least one top-level (not nested) item when DeterministicSourcePaths is true [C:\projects\newtonsoft-json\Src\Newtonsoft.Json\Newtonsoft.Json.csproj]

@ctaggart What is the cause of this error and how do I fix it?

@ctaggart
Copy link

ctaggart commented Jun 22, 2018

I don't know. What is your environment? I know it requires .NET SDK 2.1 https://github.com/dotnet/sourcelink/ . Can you give me a msbuild /version and a dotnet --info for that environment?

@ctaggart
Copy link

Any just to make sure your git repo is normal, can you run git rev-parse HEAD & git config --get remote.origin.url?

@JamesNK
Copy link
Owner Author

JamesNK commented Jun 22, 2018

MSBuild is 15.7.179.6572

https://ci.appveyor.com/project/JamesNewtonKing/newtonsoft-json/build/1.0.1535#L231

I'm not actually building with dotnet cli.

@JamesNK
Copy link
Owner Author

JamesNK commented Jun 22, 2018

C:\Development\Source\Newtonsoft.Json [jamesnk/source-link ≡]> git rev-parse HEAD
33d60e09415a79ba9d397c436938f466858deccc
C:\Development\Source\Newtonsoft.Json [jamesnk/source-link ≡]> git config --get remote.origin.url
https://github.com/JamesNK/Newtonsoft.Json.git
C:\Development\Source\Newtonsoft.Json [jamesnk/source-link ≡]>

Normal looking?

@JamesNK
Copy link
Owner Author

JamesNK commented Jun 22, 2018

I updated dotnet CLI to latest just in case and still have the same error.

C:\Development\Source\Newtonsoft.Json [jamesnk/source-link ≡]> dotnet --version
2.1.301

@ctaggart
Copy link

I'll check out the branch and see if I can reproduce it.

@ctaggart
Copy link

ctaggart commented Jun 22, 2018

I think you are hitting this bug: dotnet/sourcelink#62

May be try setting DeterministicSourcePaths to false.

@MagicAndre1981 MagicAndre1981 mentioned this pull request Jun 22, 2018
@ctaggart
Copy link

I opened up #1747 for that change to this branch. My build is still going, but I'm going to sleep:
https://ci.appveyor.com/project/ctaggart/newtonsoft-json/build/28

@JamesNK
Copy link
Owner Author

JamesNK commented Jun 22, 2018

It builds. How can I tell if source link is part of the NuGet package and it is working correctly?

@TylerBrinkley
Copy link
Contributor

When adding SourceLink support to my library I just used the generated NuGet Package in a local folder NuGet Package Source and tested it with a new console app. I also temporarily renamed my repo folder so it wouldn't find my local files.

@ctaggart
Copy link

You can use dotnet sourcelink test:
https://github.com/ctaggart/SourceLink#test

Unfortunately, it is not working yet. Both of these fail:

dotnet sourcelink test "C:\Users\taggac\Downloads\Newtonsoft.Json.11.0.3-beta1.nupkg"
dotnet sourcelink print-json "C:\Users\taggac\Downloads\Newtonsoft.Json.11.0.3-beta1\lib\netstandard2.0\Newtonsoft.
Json.pdb"

</PropertyGroup>
<ItemGroup>
<None Remove="**\*.orig" />
<None Include="..\..\LICENSE.md" Pack="true" PackagePath="LICENSE.md" />
</ItemGroup>
<ItemGroup Condition="'$(ContinuousIntegrationBuild)'=='true'">

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Microsoft.SourceLink.GitHub is not being installed. I removing this condition in my pull request and will try it out.

@ctaggart
Copy link

Rather than adding /p:DeterministicSourcePaths=false to everything like #1747, It is probably better to follow dotnet/sourcelink#91 by adding this to the WPF projects:

<DeterministicSourcePaths Condition="'$(EnableSourceLink)' == ''">false</DeterministicSourcePaths>

@ctaggart
Copy link

ctaggart commented Jun 22, 2018

The dotnet sourcelink tests pass with #1747 changes.

@ctaggart
Copy link

@JamesNK
Copy link
Owner Author

JamesNK commented Jun 23, 2018

It is probably better to follow dotnet/sourcelink#91 by adding this to the WPF projects:

There are no WPF projects.

What exactly is ContinuousIntegrationBuild and DeterministicSourcePaths? Why doesn't DeterministicSourcePaths work and what does setting it to false mean?

I don't like setting values without knowing what the cause and effect is.

I think the issue is the build script is building the entire solution. Setting DeterministicSourcePaths to false on the test projects appears to fix the errors.

@JamesNK JamesNK changed the title [WIP] Enable source link Enable source link Jun 23, 2018
@JamesNK JamesNK changed the base branch from jamesnk/build-directory to master June 23, 2018 23:34
@JamesNK JamesNK merged commit 7217c48 into master Jun 24, 2018
@JamesNK
Copy link
Owner Author

JamesNK commented Jun 24, 2018

Merged. Thanks for your help @ctaggart

@MagicAndre1981
Copy link

do you have a preview/nightly NuGet Package to test this? On myget I also only see the 11.0.2.

@JamesNK
Copy link
Owner Author

JamesNK commented Jun 25, 2018

https://www.myget.org/feed/json-net/package/nuget/Newtonsoft.Json

@MagicAndre1981
Copy link

MagicAndre1981 commented Jun 25, 2018

thanks. Works, I see the SourceLink entry in the PDB (with dotPeek from Jetbrains)

<SourceLink>
    <Conversion FilePath="/_/*" Uri="https://raw.githubusercontent.com/JamesNK/Newtonsoft.Json/7217c484e9705b5e76585c8b7fcd489c8e021c23/*" />
</SourceLink>

and stepping through the code works fine in VS2017 15.7.4

@tmat
Copy link

tmat commented Jun 25, 2018

What exactly is ContinuousIntegrationBuild and DeterministicSourcePaths? Why doesn't DeterministicSourcePaths work and what does setting it to false mean?

ContinuousIntegrationBuild is an indicator that the build is a CI build as opposed to a dev build. Targets may decide to behave differently when running on CI vs. local dev build. For example, when both Deterministic is true and ContinuousIntegrationBuild is true we enable DeterministicSourcePaths.

DeterministicSourcePaths causes the build to normalize all paths generated to the DLL and PDB to be deterministic - that is do not depend on the directory the source code was built from and the OS it was built on. Without this feature on the PDBs would include full paths like C:\MyRepos\Newtonsoft.Json\Src\Newtonsoft.Json\ConstructorHandling.cs. With DeterministicSourcePaths on the paths will be normalized to /_/Src/Newtonsoft.Json/ConstructorHandling.cs. You can see this happening in the SourceLink entry - since SourceLink needs to map the directory of the repository to the raw github URL it maps /_/*.

The reason why DeterministicSourcePaths is only switched on for CI builds is local debugging. If it was enabled when you e.g. F5 in Visual Studio the debugger wouldn't be able to find the sources as they do not exist at /_/* location on your dev machine and they were not pushed to the github repo yet, assuming you don't push every time you debug.

@ctaggart
Copy link

Looking forward to 11.0.3 being published :-)

@ctaggart
Copy link

I blogged about it:
http://blog.ctaggart.com/2018/06/newtonsoftjson-enabling-source-link.html

@dezsiszabi
Copy link

This PR had a possibly unintentional side effect:

https://stackoverflow.com/a/53983050/4620101

Maybe it's worth mentioning it in documentation or somewhere (migration doc?).

https://docs.microsoft.com/en-us/visualstudio/test/troubleshooting-code-coverage?view=vs-2017#pdb-symbol-files-are-unavailable

@tmat
Copy link

tmat commented Jan 9, 2019

Since nuget.org server now supports uploading symbol packages, I'd recommend to remove the PDB from the package (remove AllowedOutputExtensionsInPackageBuildOutputFolder property update) and instead produce .snupkg by setting:

<IncludeSymbols>true</IncludeSymbols>
<SymbolPackageFormat>snupkg</SymbolPackageFormat>

.snupkg file can then be published to nuget.org along with the main 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.

None yet

6 participants