Implements targets that calculate PathMap from SourceRoots

[tmat]

Customer scenario

This change is a piece in larger effort to make SourceLink easy to use, robust and compatible with deterministic builds.

See https://github.com/tmat/repository-info/tree/master/docs for an overview of the end-to-end scenario and dotnet/msbuild#2960 for a list of all components that support the scenario.

Bugs this fixes

#25635

Workarounds, if any

Risk

Small.

Performance impact

Small.

Is this a regression from a previous update?

New feature.

Root cause analysis

How was the bug found?

Test documentation updated?

Implementation details

The data flow among msbuild targets involved here is a bit tricky. The high-level design assumes a set of producers and a set of consumers of source control information. Producing targets (e.g. target wrapping a GitLib) need to execute before consuming targets (e.g. AssemblyInformationalVersion generator, Roslyn compiler, NuSpec generator, etc.), thus they need to synchronize on an in-between target, which is InitializeSourceControlInformation target defined in Microsoft.Common.targets.

Source control info producer populates SourceRoot items. They can also be populated from source packages or added manually to the project.
MapSourceRootPaths takes SourceRoot items and adds MappedPath to metadata of each item. This is the path that the compiler will generate into the DLL/PDB. It's a path with a deterministic prefix if Deterministic is true, and the original path otherwise.
SourceRoot then flows to the other target defined here that calculates PathMap based on the MappedPath metadata of each root. PathMap is then used by csc task.
SoruceRoot also flows to Source Link generator target (which will be supplied by a SourceLink nuget package), which uses MappedPath and other info on the SourceRoot to generate sourcelink.json file that is passed to csc task.

TODO:

• Fix path map escaping issues: Unable to use paths with spaces in PathMap #22835, PathMap should accept and ignore empty mapping #23523

[tmat]

@nguerrera

[tmat]

@jaredpar Can these 3 issues with PathMap be addressed in 15.7 so we can clean this up?

[tmat]

@jaredpar Ping.

[jaredpar]

Is this asking to add a new warning / error to the compiler in the default config?

[tmat]

Yes, that's one of the issues. Right now if you pass /deterministic and /pathmap that doesn't cover all paths you don't know that the compiler outputs are still non-deterministic.

[tmat]

This particular issue is not blocking the SourceLink change. It's the other issues with quoting and escaping that are (to some extent).

[tmat]

Then add a new switch? Or you don't see value in reporting that error?

[tmat]

/deterministic:strict

[jaredpar]

It's not that I don't see value in the switch, I just understand the constraints we live under. There are scenarios which are far more valuable and easy to detect for which I can't issue new diagnostics.

---

In reply to: 176509304 [](ancestors = 176509304)

[tmat]

This scenario is somewhat unique though since one can't write an analyzer that reports these errors currently. We would need to expose a new API that the analyzer could hook into. This would be a callback that's invoked on every path that's embedded into the DLL/PDB. Then an analyzer could hook that and report errors if the paths do not match the PathMap. Would that be a preferred way?

[tmat]

I guess the analyzer can be written today that finds all the paths that end up embedded into the DLL/PDB, but it'd be hard to keep it up to date with the compiler.

[jaredpar]

Split-Path $dotnet [](start = 30, length = 18)

Should use Split-Path -Parent to make the use explicit here.

[jaredpar]

$env:DOTNET_INSTALL_DIR [](start = 4, length = 24)

This approach overall seems fragile. Our tests run in many contexts, most of which are not driven by this script. Hence there is no way to depend on this value being set.

[tmat]

A better idea? The test runner tries to use the DOTNET_INSTALL_DIR and falls back to looking for the right version of the SDK on PATH.

[jaredpar]

Unfortunately I don't have a better idea.

---

In reply to: 176506227 [](ancestors = 176506227)

[jaredpar]

DotNetSdkTests [](start = 17, length = 14)

If the intent is to test MSBuild you need to be deploying a Directory.Build.props, Directory.Build.targets, and editorconfig that disables parent lookup. That's necessary to make these type of tests stable.

[nguerrera]

Why editor config?

[tmat]

Good point. I'll add those.

[jcouv]

xml doc would be good here too #Closed

[tmat]

Will add. #Resolved

[jcouv]

Nit: local functions should be camelCased. If it's ok, please also move to the end of the method. #Closed

[tmat]

I have actually refactored it a bit here. #Resolved

[tmat]

Will change the casing.

[jcouv]

I'm looking at iteration 3. Should I wait until you push something more?

---

In reply to: 176874575 [](ancestors = 176874575)

[jcouv]

Please add a link for follow-up issue, rather than just a TODO comment. #Closed

[jcouv]

Perhaps sourceControlled? #Closed

[jcouv]

Perhaps add comment // root isn't nested #Closed

[jcouv]

root.ItemSpec [](start = 56, length = 13)

Perhaps extract local for string path = root.ItemSpec; #Closed

[jcouv]

Is there harm to leave the zero in /_0/ in the i==0 case? #Closed

[jcouv]

Can or should we assert that there was no metadata called MappedPath on the root before setting our own? #Closed

[jcouv]

Maybe "...source-controlled non-nested roots first:" #Closed

[jcouv]

Maybe "...other non-nested roots:" #Closed

[tmat]

@jcouv I rebased the PR and your comments are not visible here anymore:

Is there harm to leave the zero in /_0/ in the i == 0 case?

No harm, just the common case will be a single root, so I didn't want to include 0 for aesthetic reasons :).
Note that these paths will be visible thru CallerFilePathAttribute.

Can or should we assert that there was no metadata called MappedPath on the root before setting our own?

Good question. we can't assert, but we can report a warning if there is one. Rewriting metadata in msbuild does not usually produce warnings though, so I'm not sure we should here.

#Resolved

[jcouv]

I looked around for a similar UsingTask to import the Csc task. I found it in WriteMSBuildFiles which makes a .props file for the package.
Should we use the same mechanism for the MapSourceRoots task?
#Closed

[tmat]

No, this needs to be in the Microsoft.Managed.Core.targets. These are targets are only used via a package reference if you explicitly drop Microsoft.Net.Compilers to your project. By default these are picked up from msbuild, where we insert them to.

[jcouv]

ContinuousIntegrationBuild [](start = 117, length = 26)

Btw, it would be helpful to indicate in your documentation for ContinuousIntegrationBuild that it's a boolean.
https://github.com/tmat/repository-info/tree/master/docs #Closed

[tmat]

Done.

[jcouv]

Typo: This target ... #Closed

[tmat]

Fixed.

[jcouv]

I think the tests/framework simulating end-to-end msbuild are great, and will be useful for future work with custom targets. I wonder if the msbuild repo has something similar by now (seems it would be useful there too). #Resolved

[tmat]

The dotnet SDK repo has this kind of test API. The problem is none of these are in a form that's reusable elsewhere. It would indeed be good to create some model that can be reused via a NuGet package.

[jaredpar]

Consider EnsureEndsWithSlash.

[tmat]

Fixed

[jaredpar]

      var rootByItemSpec = new Dictionary<string, ITaskItem>(); [](start = 1, length = 68)

Why are we using ordinal path comparison here? The compiler is pretty inconsistent here but generally favors insensiitive.

[tmat]

Are you saying we do not support source directories that differ by casing on Linux?

[jaredpar]

Different parts of the compiler differ here. It's not consistent.

[jaredpar]

var topLevelMappedPaths = new Dictionary<string, string>(); [](start = 16, length = 59)

Why are we using ordinal path comparison here? The compiler is pretty inconsistent here but generally favors insensiitive.

[tmat]

These paths are generally produced by a source control package (e.g. one based on GitLib library). It is expected that the tool produces paths that have consistent casing. This particular dictionary is used to map submodule paths to their containing source roots. When the submodule paths are produced by the GitLib library they should match the casing exactly.

[jaredpar]

DotNetSdkTestBase [](start = 34, length = 17)

I have concerns with how this is intended to work. There are really two options to doing MSBuild integration tests

1. Use an official layout mechanism to place the compiler binaries on dusk. For example unzipping the compiler NuGet package.
2. Adhoc copy the binaries on disk to a single directory.

This test seems to be doing the latter. We've been moving away from that because we've found it's both flaky and it doesn't actually catch all of the issues it's intended to catch. I'm worried we're creating a new case where we're tricking ourselves into believing scenarios are covered that simply aren't.

[tmat]

I'm not sure I understand. I'm not copying binaries anywhere. The compiler build task is not even executed in these tests scenarios - I replace the csc target. The only binary that matters for these tests is the one that defines the build tasks. The goal of these tests is to test the .targets files, not the compiler.

[jaredpar]

That's a bit more stable. Overall though I'm highly skeptical of tests like this. There are reasons we yanked out similar ones for MSBuild. Their sustainability doesn't seem to hold up over time.

[tmat]

I'd be more than happy to use any other mechanism to test these targets once it's available. I think msbuild or dotnet SDK should come up with a test framework that can be imported as a package. Until that happens though we need to test the targets somehow.

[tmat]

test this please #Resolved

[tmat]

test this please

[tmat]

test windows_debug_unit64_prtest please

[tmat]

test windows_release_vs-integration_prtest please

[tmat]

@dotnet/roslyn-compiler Ping.

[jcouv]

LGTM Thanks

[jcouv]

LGTM
@jaredpar @dotnet/roslyn-compiler for a second review. Thanks

[tmat]

@jaredpar Is any ask mode approval needed for merge?

[jaredpar]

@tmat yes. Mine 😄. Approved

[tmat]

@jaredpar 👍 Thanks