Conversation
|
I added a note about the |
| available through the binlog. | ||
|
|
||
| - Package signing breaks the possibility of bit-by-bit reproducibility, due to | ||
| embedding a timestamp. Nuget has the concept of a contnet hash, that can |
There was a problem hiding this comment.
| embedding a timestamp. Nuget has the concept of a contnet hash, that can | |
| embedding a timestamp. NuGet has the concept of a content hash, that can |
There was a problem hiding this comment.
you could still strip the signature before comparing the packages right? if yes, that doesn't sound like a big deal to me
There was a problem hiding this comment.
Good point. I am worried about the sha hashes in the .deps.json files, though.
There was a problem hiding this comment.
.deps.json shouldn't be in the nuget packages though? or do you mean when you build an app with the sdk?
There was a problem hiding this comment.
ah they could be for tools packages. I wonder how that works today?
There was a problem hiding this comment.
They also end up in the .NET SDK itself:
$ find /usr/lib64/dotnet -iname '*.deps.json'
/usr/lib64/dotnet/packs/Microsoft.AspNetCore.App.Runtime.fedora.43-x64/10.0.3/runtimes/fedora.43-x64/lib/net10.0/Microsoft.AspNetCore.App.deps.json
/usr/lib64/dotnet/packs/Microsoft.NETCore.App.Runtime.fedora.43-x64/10.0.3/runtimes/fedora.43-x64/lib/net10.0/Microsoft.NETCore.App.deps.json
/usr/lib64/dotnet/shared/Microsoft.NETCore.App/10.0.3/Microsoft.NETCore.App.deps.json
/usr/lib64/dotnet/shared/Microsoft.AspNetCore.App/10.0.3/Microsoft.AspNetCore.App.deps.json
/usr/lib64/dotnet/sdk/10.0.103/Containers/tasks/net10.0/Microsoft.NET.Build.Containers.deps.json
/usr/lib64/dotnet/sdk/10.0.103/DotnetTools/dotnet-dev-certs/10.0.3-servicing.26075.103/tools/net10.0/any/dotnet-dev-certs.deps.json
/usr/lib64/dotnet/sdk/10.0.103/DotnetTools/dotnet-format/BuildHost-netcore/Microsoft.CodeAnalysis.Workspaces.MSBuild.BuildHost.deps.json
/usr/lib64/dotnet/sdk/10.0.103/DotnetTools/dotnet-format/dotnet-format.deps.json
/usr/lib64/dotnet/sdk/10.0.103/DotnetTools/dotnet-user-jwts/10.0.3-servicing.26075.103/tools/net10.0/any/dotnet-user-jwts.deps.json
/usr/lib64/dotnet/sdk/10.0.103/DotnetTools/dotnet-user-secrets/10.0.3-servicing.26075.103/tools/net10.0/any/dotnet-user-secrets.deps.json
/usr/lib64/dotnet/sdk/10.0.103/DotnetTools/dotnet-watch/10.0.103/tools/net10.0/any/BuildHost-netcore/Microsoft.CodeAnalysis.Workspaces.MSBuild.BuildHost.deps.json
/usr/lib64/dotnet/sdk/10.0.103/DotnetTools/dotnet-watch/10.0.103/tools/net10.0/any/dotnet-watch.deps.json
/usr/lib64/dotnet/sdk/10.0.103/DotnetTools/dotnet-watch/10.0.103/tools/net10.0/any/dotnet.deps.json
/usr/lib64/dotnet/sdk/10.0.103/FSharp/fsc.deps.json
/usr/lib64/dotnet/sdk/10.0.103/FSharp/fsi.deps.json
/usr/lib64/dotnet/sdk/10.0.103/Roslyn/bincore/VBCSCompiler.deps.json
/usr/lib64/dotnet/sdk/10.0.103/Roslyn/bincore/csc.deps.json
/usr/lib64/dotnet/sdk/10.0.103/Roslyn/bincore/vbc.deps.json
/usr/lib64/dotnet/sdk/10.0.103/Roslyn/Microsoft.Build.Tasks.CodeAnalysis.deps.json
/usr/lib64/dotnet/sdk/10.0.103/Sdks/Microsoft.Build.Tasks.Git/tools/net/Microsoft.Build.Tasks.Git.deps.json
/usr/lib64/dotnet/sdk/10.0.103/Sdks/Microsoft.NET.Sdk.BlazorWebAssembly/tools/net10.0/Microsoft.NET.Sdk.BlazorWebAssembly.Tool.deps.json
/usr/lib64/dotnet/sdk/10.0.103/Sdks/Microsoft.NET.Sdk.Razor/source-generators/Microsoft.CodeAnalysis.Razor.Compiler.deps.json
/usr/lib64/dotnet/sdk/10.0.103/Sdks/Microsoft.NET.Sdk.Razor/tools/rzc.deps.json
/usr/lib64/dotnet/sdk/10.0.103/Sdks/Microsoft.NET.Sdk.StaticWebAssets/tools/net10.0/Microsoft.NET.Sdk.StaticWebAssets.Tool.deps.json
/usr/lib64/dotnet/sdk/10.0.103/Sdks/Microsoft.SourceLink.AzureRepos.Git/tools/net/Microsoft.SourceLink.AzureRepos.Git.deps.json
/usr/lib64/dotnet/sdk/10.0.103/Sdks/Microsoft.SourceLink.Bitbucket.Git/tools/net/Microsoft.SourceLink.Bitbucket.Git.deps.json
/usr/lib64/dotnet/sdk/10.0.103/Sdks/Microsoft.SourceLink.Common/tools/net/Microsoft.SourceLink.Common.deps.json
/usr/lib64/dotnet/sdk/10.0.103/Sdks/Microsoft.SourceLink.GitHub/tools/net/Microsoft.SourceLink.GitHub.deps.json
/usr/lib64/dotnet/sdk/10.0.103/Sdks/Microsoft.SourceLink.GitLab/tools/net/Microsoft.SourceLink.GitLab.deps.json
/usr/lib64/dotnet/sdk/10.0.103/MSBuild.deps.json
/usr/lib64/dotnet/sdk/10.0.103/NuGet.CommandLine.XPlat.deps.json
/usr/lib64/dotnet/sdk/10.0.103/datacollector.deps.json
/usr/lib64/dotnet/sdk/10.0.103/dotnet.deps.json
/usr/lib64/dotnet/sdk/10.0.103/redist.deps.json
/usr/lib64/dotnet/sdk/10.0.103/testhost.deps.json
/usr/lib64/dotnet/sdk/10.0.103/vstest.console.deps.json
There was a problem hiding this comment.
I was thinking of the hash in the .deps.json, we sign the .dlls in the Microsoft build which presumably changes the hash which was recorded in .deps.json at build time?
There was a problem hiding this comment.
I don't know the answer. This will need more investigation.
Co-authored-by: Alexander Köplinger <alex.koeplinger@outlook.com>
During review, we agreed that the DeterministicTimestamp is a good name.
|
Please feel free to merge if this looks okay now. |
nkolev92
left a comment
There was a problem hiding this comment.
One clarifying question about the default.
Worth consider whether we enable this by default in 10.0.300/10.0.400 or 11.0.100.
My primary focus is on making the .NET SDK itself reproducible. To do that, I will need to consume this in arcade. I don't think arcade can depend on 10.0.3xx or 10.0.4xx. So I am okay with targeting this to 11.0.100. |
- Make summary more focused and explicit about changes. - Drop the technical explanation section.
|
The question that I have is whether it'll be enabled by default. Does everyone that's installing the .NET 11 SDK now get the new deterministic behavior by default given that it is set it the projects. That's the part that needs documented/announced and probably the thing that would not meet the 10.0.* bar. |
nkolev92
left a comment
There was a problem hiding this comment.
LGTM.
To me 11.0.100 as default make sense, and maybe something we can include in the spec, but not too worried as long as the documentation makes sense. SDKAnalysisLevel is what we can use in this case in the pack targets.
It's also worth considering how we're changing the legacy pack default now that it's auto imported in VS too. This is on the client team though.
That's basically explained in the "Functional explanation" section. With Deterministic=true (default in most projects) some previously random GUID-based file names like The slightly more risky change of having deterministic timestamps always requires opting in via |
|
Thanks for the clarification. Honestly, package/services/metadata/core-properties is probably good enough to do even when deterministic is not requested, since we took NuGet/NuGet.Client#6963. Either way, LGTM |
Co-authored-by: Frulfump <Frulfump@users.noreply.github.com>
| This includes the names of psmdcp files, which are otherwise random and | ||
| based on a GUID. The names will now be based on a hash that's going to be | ||
| deterministic. | ||
| This includes using a single timestamp for all files added to the archive. |
There was a problem hiding this comment.
This is now suggesting we're making unified timestamp by default, but I don't tihnk that's the case.
There was a problem hiding this comment.
This is the plan, indeed: #8601 (comment)
I forgot to spell this out in the doc earlier.
There was a problem hiding this comment.
In other words, everything will have the current wall-clock-time as the default timestamp.
With DeterministicTimestamp also set, that will be used as the timestamp instead of the current-wall-clock-time.
There was a problem hiding this comment.
So this isn't correct then:
There was a problem hiding this comment.
I think Andy's comment is clearer.
IMO this is now confusing since it's not necessarily deterministic if it chooses current time.
We're not aligned in the exact language, but aligned in the UX.
There was a problem hiding this comment.
I thinking having a revert makes sense.
If I'm reading it correctly, isn't the only new thing that Deterministic does is unify the timestamps?
Isn't that the switch to revert things?
Basically what if you had a file that was last modified 5 years ago which was preserved before but now when it gets packed it has UtcNow timestamp instead.
Yeah, this was the primary thing. I think @zivkan had the conversations and did some analysis here.
There was a problem hiding this comment.
If I'm reading it correctly, isn't the only new thing that Deterministic does is unify the timestamps?
yes but given Deterministic=true is the default since .net core 3.0 and influences other places in the SDK/compiler I'm not sure turning that one off is a good escape hatch.
What if we had:
DeterministicTimestamp=true: default fallback value in the net11 sdk, uses DateTime.UtcNowDeterministicTimestamp=<explicit value>orSOURCE_DATE_EPOCHset in the environment: optin to the fixed timestampDeterministicTimestamp=false: escape hatch to turn the feature off
There was a problem hiding this comment.
I don't mind that.
I was gonna suggest a DeterministicPack option potentially too in case someone wants to decouple it from the general determinism, but I think the timestamp approach is plenty powerful.
There was a problem hiding this comment.
I slept on this, but now I am wondering if it might be better to have a few literals other than true and false:
DeterministicTimestamp=wall-clock-timereplacestrue. It's more explicit about what it's doingDeterministicTimestamp=file-timeto replacefalse. It's more explicit about the behaviour.DeterministicTimestamp=<explicit value>orSOURCE_DATE_EPOCHhandled as before
Using some fixed literals makes it easier to understand and also allows us to add more knobs if we have to down the line. The values above are just suggestions, I am happy to use another convention/style to make it more consistent with other things.
Is there precedence for this in nuget or .NET ?
| - With `Deterministic=true` by default, support for `Deterministic=false` could | ||
| be fully dropped, and the code paths simplified. | ||
|
|
||
| - Should turning off `Deterministic=true` produce warnings or errors? |
There was a problem hiding this comment.
Feels like a natural place to start with a BuildCheck and then escalate over time?
There was a problem hiding this comment.
Interesting idea, I do wonder if we could get numbers from telemetry how often Build Checks are ran, I feel like that possibility(option) isn't advertised like at all. So what I'm getting at is that it might be used too rarely to be a good guard (should probably still include it but just wanted to highlight the "fact")
Co-authored-by: Matt Kotsenas <51421+MattKotsenas@users.noreply.github.com>
|
Sounds like this is good to merge now? |
Make NuGet pack more determinisitic, by default. This makes `Deterministic` property (default = `true`) work again for nuget pack via msbuild. It adds support for `DeterministicTimestamp` property (default = `true`) too. For more details about the properties and the design goals, see: - NuGet/Home#14785 - https://github.com/NuGet/Home/main/accepted/2026/deterministic-pack-revisited.md Fixes: NuGet/Home#8601
Make NuGet pack more determinisitic, by default. This makes `Deterministic` property (default = `true`) work again for nuget pack via msbuild. It adds support for `DeterministicTimestamp` property (default = `true`) too. For more details about the properties and the design goals, see: - NuGet/Home#14785 - https://github.com/NuGet/Home/main/accepted/2026/deterministic-pack-revisited.md Fixes: NuGet/Home#8601
Make NuGet pack more determinisitic, by default. This makes `Deterministic` property (default = `true`) work again for nuget pack via msbuild. It adds support for `DeterministicTimestamp` property (default = `true`) too. For more details about the properties and the design goals, see: - NuGet/Home#14785 - https://github.com/NuGet/Home/main/accepted/2026/deterministic-pack-revisited.md Fixes: NuGet/Home#8601
No description provided.