Add support for deterministic builds #883

slang25 · 2023-03-17T20:07:35Z

This adds support for deterministic builds by detecting the MSBuild PathMap property during the build target, and then setting an environment variable which Shouldly can use at runtime to replace deterministic path roots (/_/, /_1/ etc...) with the actual local paths.

This currently wouldn't work if you have a separate build and test step, but we could add support for this later by outputting our own file to the Output directory (bin) with the PathMap value, and reading from it during test.

With this addition, I've removed the intrusive build props inclusion, which was tripping people up and hard to discover.

slang25 · 2023-03-17T21:12:14Z

src/Shouldly/Shouldly.csproj

    <PackageReference Include="Microsoft.CSharp" Version="4.7.0" />
    <PackageReference Include="Microsoft.SourceLink.GitHub" Version="1.1.1" PrivateAssets="All" Condition="$(Configuration) == 'Release'" />
    <None Include="..\..\assets\logo_128x128.png" Pack="true" PackagePath="assets" />
+  </ItemGroup>
+  <ItemGroup Condition=" '$(TargetFramework)' == 'netstandard2.0' " >


This is an unrelated change to this PR, but it was a blocker to using Rider, so have bundled up this fix.

slang25 · 2023-03-17T21:14:50Z

src/Shouldly/ShouldlyConfiguration.cs

+    /// Shouldly can enhance assertion failure messages if it can find the source code at the call site.
+    /// When using deterministic builds, set this property to explicitly tell Shouldly the root path which symbol paths will be relative to.


The wording here is pretty awkward, suggestions welcome!

slang25 · 2023-03-17T21:19:04Z

Another thing to note is that this all goes away if and when we use CallerArgumentExpression, as that's really what all of these hacks are trying to reproduce.

iBrotNano

Hi @slang25,

for me removing build.props would definitly solve my build problems. I am not shure what happend if the other properties are not set properly.

    <DebugSymbols>true</DebugSymbols>
    <Optimize>false</Optimize>
    <DebugType>embedded</DebugType>

But this solution looks very promising.

iBrotNano · 2023-03-20T15:30:02Z

src/Shouldly/Internals/SourceCodeTextGetter.cs

            if (sourceRoot != null)
            {
-                fileName = fileName.Replace("/_/", sourceRoot + Path.PathSeparator);
+                return fileName.Replace("/_/", sourceRoot + Path.PathSeparator);


I am working on a windows system. The files are processed with this inputs and outputs:

Non deterministic test project build:

Input: E:\Temp\shouldly\src\DeterministicTests\Tests.cs
Output: E:\Temp\shouldly\src\DeterministicTests\Tests.cs

This is fine.

Deterministic test project build:

Input: /_/src/DeterministicTests/Tests.cs
Output: E:\Temp\shouldly;src/DeterministicTests/Tests.cs

Path.PathSeperator should be Path.DirectorySeperator and the slashes must be converted.

return fileName.Replace("/_/", sourceRoot + Path.DirectorySeparatorChar) .Replace(Path.AltDirectorySeparatorChar, Path.DirectorySeparatorChar);

Great spot, thanks

iBrotNano · 2023-03-20T15:36:38Z

src/Shouldly/Internals/SourceCodeTextGetter.cs

@@ -57,15 +66,14 @@ private void ParseStackTrace(StackTrace? stackTrace)
                    TryFindGitRepoRoot(assemblyDirectory!, out sourceRoot);


Maybe searching for the SLN could be an alternative to the directory .git. It would remove the dependency to Git if you start a project and add Git afterwards.

private static bool TryFindProjectFolder(string startDirectory, [NotNullWhen(true)] out string? projectFolder) { try { var currentDirectory = new DirectoryInfo(startDirectory); while (currentDirectory != null) { if (IsGitParentDirectory(currentDirectory) || IsSlnParentDirectory(currentDirectory)) { projectFolder = currentDirectory.FullName; return true; } currentDirectory = currentDirectory.Parent; } } catch { } projectFolder = null; return false; } private static bool IsGitParentDirectory(DirectoryInfo currentDirectory) { var gitDirectory = Path.Combine(currentDirectory.FullName, ".git"); return Directory.Exists(gitDirectory); } private static bool IsSlnParentDirectory(DirectoryInfo currentDirectory) { var foundFiles = Directory.EnumerateFiles(currentDirectory.FullName, "*.sln"); return foundFiles.Any(); }

Further checks could be implemented as needed. I think the best solution would be to look for the CSPROJ of the test projects and parse for the referenced projects to test. But there could be multiple. It would add some complexity.

I think we'd get too many false positives, I just checked a handful of open source .NET projects and about half had the .sln file one level in (Newtonsoft.Json, Spectre.Console etc...).

The git approach is quite crude, but might cover 90% of scenarios, for everything else we can tell users to pass a SourceRoot.

For deterministic builds, there an MSBuild ItemGroup item named SourceRoot which ideally we'd want to reuse, but the only way to get it would be some sort of source code generation.

SimonCropp · 2023-03-20T22:30:23Z

@slang25 whats stopping us from using CallerArgumentExpression?

slang25 · 2023-03-20T22:37:52Z

@slang25 whats stopping us from using CallerArgumentExpression?

We could add it but there's a few trade offs:

We don't have a single point of entry like FluentAssertions do (.Should().), so we'd have to add it everywhere
For older compiler version (think .NET Framework users) I think it would just look like a random optional parameter, so we'd want to only expose it to the .NET 6 (we should bump it from .NET 5) and conditional compilation all-the-things.
It needs to be an optional parameter, so that means a major version bump as although it remains source compatible, it's not a binary compatible change

The combination of the first two points will make it a bit of a messy change, but the more I think about it the more I think we should do it.

slang25 · 2023-03-20T22:49:17Z

Expect a separate PR shortly, I think it won't take too long

SimonCropp · 2023-03-20T23:21:06Z

For older compiler version (think .NET Framework users) I think it would just look like a random optional parameter, so we'd want to only expose it to the .NET 6 (we should bump it from .NET 5) and conditional compilation all-the-things.

this supports CallerArgumentExpression back to net461 https://github.com/SimonCropp/Polyfill#callerargumentexpressionattribute

slang25 · 2023-03-20T23:25:06Z

For older compiler version (think .NET Framework users) I think it would just look like a random optional parameter, so we'd want to only expose it to the .NET 6 (we should bump it from .NET 5) and conditional compilation all-the-things.

this supports CallerArgumentExpression back to net461 https://github.com/SimonCropp/Polyfill#callerargumentexpressionattribute

That's only if the consumer of the package has a compiler version that supports it, we can't guarantee the version of tooling or build environment consumers of this would be using.

So for example your Polyfill library would work for users using .NET SDK 6 and up targeting the net461 tfm, but those clunky old .NET Framework ye olde projects will never work and they will look unusable.

SimonCropp · 2023-03-20T23:32:24Z

So for example your Polyfill library would work for users using .NET SDK 6 and up targeting the net461 tfm, but those clunky old .NET Framework ye olde projects will never work and they will look unusable.

yeah thats true

slang25 · 2023-04-04T08:53:00Z

I think this is good to merge @SimonCropp, let me know if there are any objections

SimonCropp · 2023-04-04T09:14:51Z

did u consider using msbuild to embed the SolutionDir as an attribute in the consuming assembly, and then reading that at runtime?

SimonCropp · 2023-04-04T09:29:00Z

it is the approach i use in verify

slang25 · 2023-04-04T10:11:44Z

I had considered it, but then I realised that it defeats the purpose of deterministic builds, because now the binaries will differ depending on the machine and location they were built on.

A reasonable solution would be to pass it using an environment variable by the build runner. The SourceRoot property leaves that to the consumer to figure out how to plumb it in, but we could be a bit more opinionated and say that we have a well-known environment variable we look out for, like SHOULDLY_SOURCE_ROOT.

slang25 · 2023-04-04T13:05:57Z

Actually, let's do that. I'll remove the config value and use an env var, and have a target that will set it if it detects the right conditions.

slang25 · 2023-04-09T11:45:41Z

@SimonCropp I've pushed a new approach, not fully tested yet outside of the test project, I'm trying to figure out if buildTransitive is the right thing to use (I think it is, but want to check).

This approach uses the PathMap property set by the SDK, which can contain multiple pairs of mappings, for example /_/, /_1/, /_2/ etc...

This approach currently only works when you do dotnet test, and not in scenarios where you do a build & test in separate steps, for this we could output the mapping to our own ShouldlyPathMaps file in the output folder, this is similar to how coverlet solve this problem (their problem is a little bit more complex that ours, but it's still relevant).

slang25 · 2023-04-11T16:58:54Z

@SimonCropp any feedback?

sungam3r · 2023-04-17T18:11:43Z

@slang25 @SimonCropp after upgrading to v4.2 one of our CI workflows in GraphQL.NET parser failed with message

  System.IO.DirectoryNotFoundException : Could not find a part of the path '/_/src/GraphQLParser.ApiTests/GraphQLParser.received.txt'.

graphql-dotnet/parser#299
https://github.com/graphql-dotnet/parser/actions/runs/4720850703/jobs/8373407193?pr=299

sungam3r · 2023-04-17T18:42:24Z

The same here https://github.com/sungam3r/SteroidsDI/actions/runs/4723228231/jobs/8378900941?pr=122

slang25 · 2023-04-17T20:00:10Z

Thanks for reporting @sungam3r

What we were doing in Shouldly 4.1.0 and lower was to disable deterministic builds for any projects that referencing Shouldly, this would be my first suggestion here if it's not something you want (as a short term workaround).

The current support added in this PR only works when the dotnet test step also does the build, and does not work when they are run in separate steps. It could be extended to add that, and it's looking like this will be common enough to invest the effort in doing so, I'll see if I can get another PR spun up with an implementation.

sungam3r · 2023-04-17T20:25:32Z

OK. Our workflows intentionally use dotnet test --no-restore --no-build in separate step after dotnet build.

sungam3r · 2023-04-19T18:30:33Z

New error detected in Xabaril/AspNetCore.Diagnostics.HealthChecks#1801:

 Error Message:
   System.UnauthorizedAccessException : Access to the path '/_' is denied.
---- System.IO.IOException : Permission denied

slang25 force-pushed the support-deterministic-builds branch from 0aa752d to 30be3e9 Compare March 17, 2023 20:47

Add support for deterministic builds

591fb21

slang25 force-pushed the support-deterministic-builds branch from 30be3e9 to 591fb21 Compare March 17, 2023 20:49

Approve Public API change

c0dceda

slang25 mentioned this pull request Mar 17, 2023

Deterministic build #881

Closed

slang25 commented Mar 17, 2023

View reviewed changes

slang25 added 3 commits March 18, 2023 13:31

Refactor for clarity

03a1507

Fix minor error

9c7152f

Enable deterministic build of Shouldly itself

14c1b0c

iBrotNano reviewed Mar 20, 2023

View reviewed changes

Update SourceCodeTextGetter.cs

0323ecc

SimonCropp and others added 2 commits April 1, 2023 16:43

Merge branch 'master' into support-deterministic-builds

e0050a5

Merge branch 'master' into support-deterministic-builds

30d9dee

slang25 requested a review from SimonCropp April 2, 2023 19:34

slang25 added 3 commits April 7, 2023 17:10

Use environment variable instead of config for source root

70252b1

Try using PathMap for source roots

7774682

Skip when SHOULDLY_SOURCE_PATH_MAP already set

f2ac390

SimonCropp approved these changes Apr 12, 2023

View reviewed changes

slang25 merged commit d0f1664 into master Apr 12, 2023
2 checks passed

slang25 deleted the support-deterministic-builds branch April 12, 2023 21:48

slang25 mentioned this pull request Apr 12, 2023

Installing Shouldly silently disables optimization and changes other properties #795

Open

slang25 mentioned this pull request Apr 22, 2023

Improved deterministic build support #898

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for deterministic builds #883

Add support for deterministic builds #883

slang25 commented Mar 17, 2023 •

edited

slang25 Mar 17, 2023

slang25 Mar 17, 2023

slang25 commented Mar 17, 2023

iBrotNano left a comment

iBrotNano Mar 20, 2023

slang25 Mar 20, 2023

iBrotNano Mar 20, 2023

slang25 Mar 20, 2023 •

edited

SimonCropp commented Mar 20, 2023

slang25 commented Mar 20, 2023

slang25 commented Mar 20, 2023

SimonCropp commented Mar 20, 2023

slang25 commented Mar 20, 2023

SimonCropp commented Mar 20, 2023

slang25 commented Apr 4, 2023

SimonCropp commented Apr 4, 2023

SimonCropp commented Apr 4, 2023

slang25 commented Apr 4, 2023

slang25 commented Apr 4, 2023

slang25 commented Apr 9, 2023 •

edited

slang25 commented Apr 11, 2023

sungam3r commented Apr 17, 2023

sungam3r commented Apr 17, 2023

slang25 commented Apr 17, 2023 •

edited

sungam3r commented Apr 17, 2023

sungam3r commented Apr 19, 2023

		/// Shouldly can enhance assertion failure messages if it can find the source code at the call site.
		/// When using deterministic builds, set this property to explicitly tell Shouldly the root path which symbol paths will be relative to.

		@@ -57,15 +66,14 @@ private void ParseStackTrace(StackTrace? stackTrace)
		TryFindGitRepoRoot(assemblyDirectory!, out sourceRoot);

Add support for deterministic builds #883

Add support for deterministic builds #883

Conversation

slang25 commented Mar 17, 2023 • edited

slang25 Mar 17, 2023

Choose a reason for hiding this comment

slang25 Mar 17, 2023

Choose a reason for hiding this comment

slang25 commented Mar 17, 2023

iBrotNano left a comment

Choose a reason for hiding this comment

iBrotNano Mar 20, 2023

Choose a reason for hiding this comment

slang25 Mar 20, 2023

Choose a reason for hiding this comment

iBrotNano Mar 20, 2023

Choose a reason for hiding this comment

slang25 Mar 20, 2023 • edited

Choose a reason for hiding this comment

SimonCropp commented Mar 20, 2023

slang25 commented Mar 20, 2023

slang25 commented Mar 20, 2023

SimonCropp commented Mar 20, 2023

slang25 commented Mar 20, 2023

SimonCropp commented Mar 20, 2023

slang25 commented Apr 4, 2023

SimonCropp commented Apr 4, 2023

SimonCropp commented Apr 4, 2023

slang25 commented Apr 4, 2023

slang25 commented Apr 4, 2023

slang25 commented Apr 9, 2023 • edited

slang25 commented Apr 11, 2023

sungam3r commented Apr 17, 2023

sungam3r commented Apr 17, 2023

slang25 commented Apr 17, 2023 • edited

sungam3r commented Apr 17, 2023

sungam3r commented Apr 19, 2023

slang25 commented Mar 17, 2023 •

edited

slang25 Mar 20, 2023 •

edited

slang25 commented Apr 9, 2023 •

edited

slang25 commented Apr 17, 2023 •

edited