InstrumentationHelper.HasPdb always returns false for deterministic source paths

[sharwell]

When /p:DeterministicSourcePaths=true is set, InstrumentationHelper.HasPdb looks for PDB files in the wrong location. These files are never found, so instrumentation is not performed.

https://github.com/tonerdo/coverlet/blob/062b907735901bdf3ad9d4d40953f53a4fe3f364/src/coverlet.core/Helpers/InstrumentationHelper.cs#L74

[viceice]

Additionally, it should have a try ... catch block, because peReader.ReadDebugDirectory() can fail.

Eg Oracle.ManagedDataAccess 18.3.0 is failing.

[MarcoRossignoli]

Additionally, it should have a try ... catch block, because peReader.ReadDebugDirectory() can fail.

Part of this fixed in https://github.com/tonerdo/coverlet/pull/367/files

[MarcoRossignoli]

@sharwell is there a "simple" way to repro this(maybe some simple example on some project ie winform etc...)?

[sharwell]

@MarcoRossignoli If you have WinForms, you should be able to simply set an environment variable DeterministicSourcePaths=true and the build will pick it up.

[MarcoRossignoli]

.net 3.0?I'll give it a try thanks!

[MarcoRossignoli]

@sharwell can you share a command to repro? I cloned windowsform and printed which modules is skipped

C:\git\winforms\src\System.Windows.Forms.Design\tests\UnitTests (master -> origin)
λ dotnet test /p:CollectCoverage=true

But with or without set DeterministicSourcePaths=true I get same list

[sharwell]

@MarcoRossignoli Here's a repro case:

1. Check out Enable code coverage dotnet/roslyn-analyzers#3028
2. Build with .\Build.cmd -rebuild /p:DeterministicSourcePaths=true
3. Test with .\Test.cmd /p:Coverage=True

Expected results:

Coverage files in artifacts\coverage have coverage information.

Actual results:

Coverage files are empty.

[MarcoRossignoli]

great thank's I'll take a look asap some busy weeks!

[MarcoRossignoli]

Hi @tmat I'm working on this issue and found dotnet/sourcelink#91 (comment)

Btw I don't undestand why it doesn't work on current repo, I've build with

D:\git\coverlet\test\coverlet.core.tests (master -> origin)
λ dotnet build ..\..\ /p:DeterministicSourcePaths=true

When I debug and try read debug directory for a referenced lib(from test) there is an "incomplete" path for source file inside pdb

coverlet.tests.projectsample.empty.dll is referenced by test project and imports Microsoft.SourceLink.GitHub package from https://github.com/MarcoRossignoli/coverlet/blob/issue_363/Directory.Build.props#L16

If I understood well Microsoft.SourceLink.GitHub should load SourceRoot during build with correct path.

@sharwell the problem is here...if compiled with /p:DeterministicSourcePaths=true the paths reported inside pdbs are all relative to / _/... and we fail to find local source.

[sharwell]

@MarcoRossignoli yes, the feature improves build determinism by hiding the path D:\git\coverlet\ that can vary across build machines.

[MarcoRossignoli]

Is there a standard "start with" prefix in case of deterministic build?I mean can we check / _/?

[sharwell]

Yes, the prefix is /_/ (a unix-style rooted path).

[MarcoRossignoli]

ok thank's Sam!

[tmat]

It's not always /_/ though. Each SourceRoot has a unique prefix /_/, /_1/, /_2/, etc. For example, when source files are linked from source packages each source package dir gets a distinct SourceRoot.

[tmat]

In any case, you should not rely on a certain prefix or prefixes. Instead, read the $(PathMap) msbuild property that's being passed to the compiler (Csc task). This map defines mapping that the compiler applies to all paths used in the compilation (source paths, pdb path, etc.).

This is an example of a PathMap value for dotnet/symreader-converter repo, which is using source packages:

F:\workspace\_work\1\s\=/_/,F:\workspace\_work\1\s\.packages\microsoft.codeanalysis.pooledobjects\3.3.0-beta3-19407-01\contentFiles\cs\netstandard1.3\=/_1/,F:\workspace\_work\1\s\.packages\microsoft.codeanalysis.debugging\3.3.0-beta3-19407-01\contentFiles\cs\netstandard1.3\=/_2/

[sharwell]

@tmat Interesting. I never saw any path but /_/. It should be easy enough to use the path map though 👍

[MarcoRossignoli]

We don't have always msbuild context available, coverlet could be used through vstest collectors(injected by test runtime) and .net tool(console app that manually instrument dll after build and remove the instrumentation at the end restoring files ) where build is done, maybe change the path a bit to include possible 1,2(regex) etc...I mean we should do something based only on produced artifact (pdb/dll)
Tmp idea is to search if file exists in case of "start with pattern" and seem work...should work also in case that pattern is a "real one" and not produced by determinism(if path is valid) https://github.com/MarcoRossignoli/coverlet/blob/issue_363/src/coverlet.core/Helpers/InstrumentationHelper.cs#L135

[tmat]

@MarcoRossignoli That's unfortunate. Other custom build targets can specify arbitrary path mapping.

Let's see if we can come up with a better solution than hard-coding some patterns. At what point do you actually need to read the source files? I'd assume you don't need to do so for the instrumentation itself. So you can produce instrumented binaries using just the pdb and the dll as inputs. Then later when you report which lines are covered you need to read the source files. Correct?

[MarcoRossignoli]

@tmat we use source in more than one phase, I'll try to explain workflow:

Coverlet has got mainly two phase:

1. prepare modules where we instrument asm and persist a structure where we map an "index"(simple integer) to sequence points/branches candidates https://github.com/tonerdo/coverlet/blob/master/src/coverlet.core/Instrumentation/Instrumenter.cs#L440-L498

We have two "hit candidates" sequence points for lines and branch point for branches.

In this phase we use Cecil to read and instrument asm, we use also pdb(DebugInformation) to get documents name and prepare "the structure" to persist and reload after testing.
In addition we need to know if a "pdb+local source file" exists to check if an asm is "instrumentable".
For instance when we read all asm from target folder we need to skip xunit dll or other dependeces for what we don't have source+pdb(it's not our code to cover), to do that we check if dll has got a pdb on local disk or an embedded one and if it's so we check that the "source file name" inside pdb are present on disk(for instance xunit has got an embedded pdb but we won't find source files on local disk and we'll skip without any need to use "filters", one of the most requested feature by users).

https://github.com/tonerdo/coverlet/blob/4f3f25fda9badcb50aac83e182c79bbc4c7ff8df/src/coverlet.core/Instrumentation/Instrumenter.cs#L69+L110

2. at the end of the test a file with hits will persisted and we load this file and togheter with persisted structure(modules/documents/hitscandidates mappings) we "account" hits

https://github.com/tonerdo/coverlet/blob/master/src/coverlet.core/Coverage.cs#L352-L385

We'll use file name inside persisted structure also to populate reports file(cobertura etc...) so tools like reportgenerator can show coverage on sources.

So to sum up we use "source file name path" everywhere.

Feel free to ask if it's not clear.

[MarcoRossignoli]

@tmat I'm trying with PathMap property on msbuild for now, some observation.

PathMap are generated and passed during "build" but "test" run after build so I don't get any PathMap data. BTW if I pass /p:DeterministicSourcePaths=true with dotnet test /p:CollectCoverage=true /p:DeterministicSourcePaths=true I get some map data, now does it make sense?Or map paths could be different from build one?

[olivier-spinelli]

@tmat Did you (Roslyn team?) consider systematically embedding the $(PathMap) as as either a standardized assembly attribute or as a resource (with a special name)?

Edit: LOL sooooorrry! Just realized that the whole point was to achieve deterministic build (this is what I'm currently fignting for) and that embedding the original paths is a totally stupid idea (to say the least!).

[clairernovotny]

Any update on this one? We're trying to push more assemblies to be deterministic and it's breaking coverlet

[MarcoRossignoli]

@clairernovotny something strange on test repo seem that some files are not in git index

C:\git\DeterministicBuilds (master -> origin)
λ ls
ClassLibrary1/  ClassLibrary1.sln  ClassLibrary1.sln.DotSettings.user  Directory.Build.props  Directory.Build.targets  LICENSE  README.md

I downloaded zip version, I get error

C:\git\DeterministicBuilds-retrieve-pathmap
λ dotnet build /p:TF_BUILD=true
Microsoft (R) Build Engine version 16.6.0-preview-20112-08+77da97f69 for .NET Core
Copyright (C) Microsoft Corporation. All rights reserved.

  Restore completed in 203,39 ms for C:\git\DeterministicBuilds-retrieve-pathmap\ClassLibrary2\ClassLibrary2.csproj.
  Restore completed in 203,37 ms for C:\git\DeterministicBuilds-retrieve-pathmap\ClassLibrary1\ClassLibrary1.csproj.
  Restore completed in 203,39 ms for C:\git\DeterministicBuilds-retrieve-pathmap\SomeLibrary.Tests\SomeLibrary.Tests.csproj.
  You are using a preview version of .NET Core. See: https://aka.ms/dotnet-core-preview
  You are using a preview version of .NET Core. See: https://aka.ms/dotnet-core-preview
  You are using a preview version of .NET Core. See: https://aka.ms/dotnet-core-preview
C:\Users\Marco\.nuget\packages\microsoft.build.tasks.git\1.0.0\build\Microsoft.Build.Tasks.Git.targets(24,5): warning : Unable to locate repository with working directory that contains directory 'C:\git\DeterministicBuilds-retrieve-pathmap\ClassLibrary2'. [C:\git\DeterministicBui
lds-retrieve-pathmap\ClassLibrary2\ClassLibrary2.csproj]
C:\Program Files\dotnet\sdk\5.0.100-preview.1.20155.7\Roslyn\Microsoft.Managed.Core.targets(104,5): error : SourceRoot items must include at least one top-level (not nested) item when DeterministicSourcePaths is true [C:\git\DeterministicBuilds-retrieve-pathmap\ClassLibrary2\Clas
sLibrary2.csproj]

Build FAILED.

C:\Users\Marco\.nuget\packages\microsoft.build.tasks.git\1.0.0\build\Microsoft.Build.Tasks.Git.targets(24,5): warning : Unable to locate repository with working directory that contains directory 'C:\git\DeterministicBuilds-retrieve-pathmap\ClassLibrary2'. [C:\git\DeterministicBui
lds-retrieve-pathmap\ClassLibrary2\ClassLibrary2.csproj]
C:\Program Files\dotnet\sdk\5.0.100-preview.1.20155.7\Roslyn\Microsoft.Managed.Core.targets(104,5): error : SourceRoot items must include at least one top-level (not nested) item when DeterministicSourcePaths is true [C:\git\DeterministicBuilds-retrieve-pathmap\ClassLibrary2\Clas
sLibrary2.csproj]
    1 Warning(s)
    1 Error(s)

Time Elapsed 00:00:01.91

What am I missing?

EDIT: it works with this on props, clearly with no sourcelink infos

<ItemGroup>
  <SourceRoot Include="$(MSBuildThisFileDirectory)/"/>
</ItemGroup>

[clairernovotny]

Weird...I just did a fresh clone and checkout the branch and it worked for me? The zip version def won't work as Source Link requires the git information.

After cloning https://github.com/clairernovotny/DeterministicBuilds, did you check out the retrieve-pathmap branch? master doesn't contain the stuff as I used that as a sample on how to configure the rest of the things.

[MarcoRossignoli]

did you check out the retrieve-pathmap

Thanks solved, didn't noticed branch name!

[MarcoRossignoli]

Plan

• msbuild support Add support for deterministic build for msbuild driver #796
• collectors support Add support for deterministic build for collectors driver #802
• add new integration tests Add integration tests for deterministic build support #811
• add workaround documentation

[clairernovotny]

The latest nightly I see for coverlet.collectors was published in March 30, I don't see one from last night?

[MarcoRossignoli]

Just pushed there was an issue with pushing and also after 2.8.1 release the order now is not preserved, btw this is the package https://www.myget.org/feed/coverlet-dev/package/nuget/coverlet.msbuild/2.8.1-g4e177f0c57 (you need to check Last updated column).

[clairernovotny]

Maybe we should talk versioning....

For prerelease builds, I tend to use a preview tag.

In the version.json, you can use 2.8.2-preview.{height} and that means that every build gets a new version. When you're ready to release, you can change it to 2.8.2, do the release, then change it to 2.8.3-preview.{height} again.

Another thing to consider is unifying the versions in the repo to make it easier. Take the highest version, move a single version.json to the root and then bump a major. If you do 6.0, then each build gets it's own patch version automatically. You could also do 6.0.0-preview.{height} as well.

[clairernovotny]

Also, I'm only using coverlet.collector and I only see a single version there:

[MarcoRossignoli]

In the version.json, you can use 2.8.2-preview.{height}

Yes this is my idea, since nightly we didn't have minor update and it worked until now, so that was an error, thanks, I'll fix asap

Another thing to consider is unifying the versions in the repo to make it easier

I'm not sure about it...at the moment we have 3 drivers(.net tool,msbuild,collectors) and not all version supports all features, have single version could confuse users.

Also, I'm only using coverlet.collector and I only see a single version there:

Yes we need to fix nightly, this is last package https://www.myget.org/feed/coverlet-dev/package/nuget/coverlet.collector/1.2.1-g4e177f0c57

[clairernovotny]

I'm not sure about it...at the moment we have 3 drivers(.net tool,msbuild,collectors) and not all version supports all features, have single version could confuse users.

FWIW, I think it could be okay to keep drivers at the same version long as what's in each is documented somewhere. If they all tend to rely on an underlying core library, then that could be what drives the main version. Totally up to you though of course.

[MarcoRossignoli]

FWIW, I think it could be okay to keep drivers at the same version long as what's in each is documented somewhere. If they all tend to rely on an underlying core library, then that could be what drives the main version

Thank's for the advice, I'll think about cc: @tonerdo

[MarcoRossignoli]

@clairernovotny nightly should be ok I tool a look at your Rx.NET repo and I prefer leave on version also commit id so users can do fast check without nbgv tool.
Now working on integration tests and docs.

[MarcoRossignoli]

@clairernovotny @tmat support/tests are ok for now, does it make sense work on documentation related to reactiveui/refit#902?

[clairernovotny]

@MarcoRossignoli What documentation for that issue? Not quite following but all docs are generally useful!

[MarcoRossignoli]

I mean the workaround to do to support deterministic build coverage, but it's not clear to me if we can release or if it doesn't work, related to your issue on reactive repo.

[clairernovotny]

I need @tmat to take a closer look at what's going on the the VSTest case.