Links to code for failed test 404 when deterministic builds are used

[martincostello]

Version

2.0.2

Details

If deterministic builds are enabled for a project, links to failing tests from the stack traces 404 instead of linking correctly to the code as they contain an incorrect /_/ segment in the GitHub link.

See this example for such a broken link.

A comparison of a broken and working link is shown below:

- https://github.com/martincostello/dependabot-helper/blob/06003a012895c3b1d70f7f1829e3fdd128180d0c/_/tests/DependabotHelper.Tests/ApiTests.cs#L1363
+ https://github.com/martincostello/dependabot-helper/blob/06003a012895c3b1d70f7f1829e3fdd128180d0c/tests/DependabotHelper.Tests/ApiTests.cs#L1363

Steps to reproduce

Clone https://github.com/martincostello/dependabot-helper/pull/615/commits/74df38bfd29b84f93c5e848838e525767be4406e and build in a GitHub Actions workflow with build.ps1 in the root of the repository.

[Tyrrrz]

Hello, @martincostello.

I'll preface it by saying that I don't fully understand how this setting is meant to work in .NET, but here is a suggestion:

1. Remove ContinuousIntegrationBuild from your .props/.csproj files
2. Pass it only during dotnet pack via a command line argument: dotnet pack -p:ContinuousIntegrationBuild=true

From what I understand, ContinuousIntegrationBuild messes up with the file paths in the pdb, which causes the issue you described.

Let me know if the suggestion applies/works.

[martincostello]

Hey there - it's right that setting it does change the file paths, but my understanding is that tooling should be able to handle them as it doesn't "mess them up", but remove the unpredictable parts by design, hence this issue.

As an example, here's a PR to the Shouldly library that improved their support for deterministic builds: https://github.com/shouldly/shouldly/pull/898

I don't think turning off the CI feature in a CI environment like GitHub Actions is a fix, that's just working around the issue here by turning off the benefit entirely.

It should "Just Work™️" whether or not the consuming test project is using deterministic builds or not.

[Tyrrrz]

It definitely does mess them up, because it replaces the path to the repository (or project, more specifically) with _. In the description of the PR that you linked, @slang25 implemented a workaround for this that does the following:

A method to check for paths which "look deterministic" if the build tooling fails for some reason. These paths are prefixed with /_/, /_1/ etc.. This is used in the ShouldMatchApproved method, where an accurate resolved location is essential.

From my perspective, relying on these heuristics is fragile and doesn't really satisfy the "Just Works" requirement. I also don't entirely see how doing this is any less of a workaround than "turning off the CI feature in a CI environment like GitHub Actions".

As another counterpoint, the official MS docs mention that the debugger — which is part of tooling too — does not work when using this option as well:

Since ContinuousIntegrationBuild only matters in the context of the produced nupkg, why shouldn't this option only be specific to dotnet pack?

[martincostello]

Well my counterpoint is why should I need to turn off a CI-specific feature to make a CI-specific feature work? It's also not just for dotnet pack - it's equally applicable to at least dotnet publish too.

Sure, I could change all my repos to make test projects opt-out of this behaviour so that the links in the workflow logs work, but I shouldn't have to do that in X repositories - it should just work regardless as I'm just using a standard .NET SDK option (nothing weird and home-rolled).

Here's another example, where the coverlet code coverage tool uses Roslyn's source path mappings to correctly map the paths for code coverage in binaries compiled in this way: https://github.com/coverlet-coverage/coverlet/blob/master/Documentation/DeterministicBuild.md

[slang25]

In defense of the mentioned "deterministic check" above, it isn't really needed as all of the relative MSBuild goop should do the job, but I felt that it would be a pragmatic pro-active check to add in, as /_/ at the start of the path isn't relative, so it would only give a false positive if you had a _ directory on the root of the drive you were building from, which I'm ok with saying will not work.

[Tyrrrz]

Well my counterpoint is why should I need to turn off a CI-specific feature to make a CI-specific feature work? It's also not just for dotnet pack - it's equally applicable to at least dotnet publish too.

You shouldn't. Both approaches are workarounds, I just feel like this is a less effort-heavy one and far less brittle.

Also, if you don't want to set the option through the command line, you can instead set it conditionally inside a target that runs before the Pack target.

Sure, I could change all my repos to make test projects opt-out of this behaviour so that the links in the workflow logs work, but I shouldn't have to do that in X repositories - it should just work regardless as I'm just using a standard .NET SDK option (nothing weird and home-rolled).

Ultimately, I believe it's on Microsoft to provide a better user experience for this.

In defense of the mentioned "deterministic check" above, it isn't really needed as all of the relative MSBuild goop should do the job, but I felt that it would be a pragmatic pro-active check to add in, as /_/ at the start of the path isn't relative, so it would only give a false positive if you had a _ directory on the root of the drive you were building from, which I'm ok with saying will not work.

You're also relying on the implementation detail of that directory being _ in the first place, as well as _1/_2 in some cases (something I wasn't even aware of). I'm definitely not saying your solution is bad – more so that the fact that every tooling developer has to implement this is quite sad.

[martincostello]

Also, if you don't want to set the option through the command line, you can instead set it conditionally inside a target that runs before the Pack target.

FWIW, this how I already set it. I don't specify it on the command line: Directory.Build.props

[Tyrrrz]

Also, if you don't want to set the option through the command line, you can instead set it conditionally inside a target that runs before the Pack target.

FWIW, this how I already set it. I don't specify it on the command line: Directory.Build.props

I know, but I meant something like this:

<Target Name="SetContinuousIntegrationBuild" BeforeTargets="Pack">
  <ContinuousIntegrationBuild Condition=" '$(CI)' != '' ">true</ContinuousIntegrationBuild>
</Target>

Assuming the value carries over to the actual Pack target this way.

[slang25]

You're also relying on the implementation detail of that directory being _ in the first place

What I'm saying is I chose to add that, but I could have omitted it, it's not the actual implementation. For Shouldly we write out the "Path Maps" which is the source of truth for this stuff, and then replace out the supplied mappings, which should always give us the correct result.

However for GitHubActionsTestLogger a different approach is needed, the logic is already a "best endeavours" approach due to the fact that the stack trace is being parsed etc... so it might make sense to keep it simple and apply the "looks deterministic heuristic" (even though as you say it's not ideal).

However to get it more "properly" I think you'd find the assembly being tested (that might be easier said than done, as I don't know too much about TestResult and what vstest provides here) and read out the sourcelink information stamped into the symbols. This is the correct way to resolve a document to a web location when using deterministic builds.

The sourcelink document contains the assemblies mapping from deterministic prefixes to remote location like this:

{"documents":{"/_/*":"https://raw.githubusercontent.com/martincostello/dependabot-helper/6902b26586b66bad3185becf472821d36bc1f66b/*"}}

[baronfel]

What @slang25 just suggested is what I was going to say - this the same behavior I use in FsAutoComplete to enable go-to-definition for remote source files.

[Tyrrrz]

However for GitHubActionsTestLogger a different approach is needed, the logic is already a "best endeavours" approach due to the fact that the stack trace is being parsed etc... so it might make sense to keep it simple and apply the "looks deterministic heuristic" (even though as you say it's not ideal).

That's true, and I wish test runners would provide source information so that it wouldn't be necessary. There is a property for that and everything, and yet it's always null 🙄

At least parsing stacktraces is fairly easy.

However to get it more "properly" I think you'd find the assembly being tested (that might be easier said than done, as I don't know too much about TestResult and what vstest provides here) and read out the sourcelink information stamped into the symbols. This is the correct way to resolve a document to a web location when using deterministic builds.

Finding the assembly is pretty straightforward, so this should be doable.

The sourcelink document contains the assemblies mapping from deterministic prefixes to remote location like this:

I think this is the best approach, thanks for the tip. Does ContinuousIntegrationBuild set an environment variable of sorts? So that it can be reliably detected that it's enabled without scanning the source link documents beforehand? I know there is CI, but it would be nice to get one specific to the aforementioned property if it exists.

[Tyrrrz]

I found a slightly simpler heuristic because I still have the scrubbed local path in the form of GITHUB_WORKSPACE