Conversation
aa6ce30 to
50b60af
Compare
50b60af to
1ca6cc7
Compare
00711c7 to
57cd90d
Compare
57cd90d to
eda2ff0
Compare
e1e6e37 to
8e626f9
Compare
| PackageLicenseExpressionVersion="$(PackageLicenseExpressionVersion)" | ||
| Readme="$(PackageReadmeFile)" | ||
| Deterministic="$(Deterministic)" | ||
| DeterministicTimestamp="$(DeterministicTimestamp)" |
There was a problem hiding this comment.
Should I add a test for the task with deterministic timestamp?
There was a problem hiding this comment.
looks like there are already two skipped tests that should be re-enabled: https://github.com/search?q=repo%3ANuGet%2FNuGet.Client+Deterministic+path%3Atest%2F**&type=code
There was a problem hiding this comment.
Yup, already enabled.
I was thinking more of a test involving the msbuild task (without running the pack command directly) somehow.
| } | ||
|
|
||
| private PackageBuilder(bool includeEmptyDirectories, bool deterministic) | ||
| : this(includeEmptyDirectories: false, deterministic: deterministic, logger: NullLogger.Instance) |
There was a problem hiding this comment.
Is this includeEmptyDirectories: false a bug?
| { | ||
| } | ||
|
|
||
| public PackageBuilder(string path, Func<string, string> propertyProvider, bool includeEmptyDirectories, bool deterministic, string deterministicTimestamp, ILogger logger, string versionOverride) |
There was a problem hiding this comment.
This class is approaching 20 constructors and is quite hard to parse, specially in terms of how constructors invoke each other (via : this(...)) Any tips on what to do here?
There was a problem hiding this comment.
can you use a get; init; property for anything new? It only works if the "main" constructor doesn't do any real business logic. You'll need to make sure the new properties are not required and are either nullable or have default values that do not cause API or ABI breaking changes. But this is my favorite technique for avoiding constructor overload explosion.
| private static DateTimeOffset ParseTimestamp(string timestamp) | ||
| { | ||
| if (timestamp != null && | ||
| long.TryParse(timestamp, NumberStyles.None, CultureInfo.InvariantCulture, out long unixTimeSeconds)) |
There was a problem hiding this comment.
This lets us do something like DeterministicTimestamp = $(SOURCE_BUILD_EPOCH) and have that work.
Alternatively, should I add explicit support for SOURCE_BUILD_EPOCH directly?
There was a problem hiding this comment.
my preference would be if setting the SOURCE_DATE_EPOCH env var would just work without the user needing to pass it explicitly to the build
There was a problem hiding this comment.
That would be my preference too. Let me do that.
There was a problem hiding this comment.
I think env vars should only be used as an absolute last resort.
They are the worst UX because they're effectively a thing that keeps getting perpetuated because it's easy.
This all to say, I get that there's some sort of consensus on that env var, but if that were to be used, I think it should just be used as a default rather than something the nuget code reads internally (so have it initialize an msbuild property).
There was a problem hiding this comment.
so have it initialize an msbuild property.
+1
| </package>"); | ||
|
|
||
| var command = "pack packageA.nuspec -Deterministic -OutputDirectory {0}"; | ||
| var timestamp = DateTimeOffset.UtcNow; |
There was a problem hiding this comment.
Should I hardocde a fixed time to make test runs easier to compare?
nkolev92
left a comment
There was a problem hiding this comment.
Thanks for doing the work here @omajid, we really appreciate it.
The comment in the PR I left tells me we are still effectively designing how the feature should work, and the PR description itself doesn't really carry details about what's been agreed upon in the long thread.
I think we should have some sort of a design here. This should both aid the reviewer here, as well as the documentation later on, on how determine pack can and should be used.
| private static DateTimeOffset ParseTimestamp(string timestamp) | ||
| { | ||
| if (timestamp != null && | ||
| long.TryParse(timestamp, NumberStyles.None, CultureInfo.InvariantCulture, out long unixTimeSeconds)) |
There was a problem hiding this comment.
I think env vars should only be used as an absolute last resort.
They are the worst UX because they're effectively a thing that keeps getting perpetuated because it's easy.
This all to say, I get that there's some sort of consensus on that env var, but if that were to be used, I think it should just be used as a default rather than something the nuget code reads internally (so have it initialize an msbuild property).
Can you point me to an example design? I am trying to understand the amount of detail and the actual format needed for the design. Thanks! |
|
Template: https://github.com/NuGet/Home/blob/dev/meta/template.md. The amount of detail is dependent on the complexity of the change. https://github.com/NuGet/Home/tree/dev/accepted/2025 has a bunch of specs: |
|
Design doc at NuGet/Home#14785 |
8e626f9 to
0433cdc
Compare
|
I am waiting for the design doc to be approved before updating this PR. |
This bot is so annoying, is there no tag that could be added to get it to go away? A large portion of my GitHub notifications is from this bot or similar. |
6e87959 to
4b52b5d
Compare
|
/azp run (retriggering due to the Azure Pipelines outage yesterday) |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
No pipelines are associated with this pull request. |
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
4b52b5d to
1f979fb
Compare
Bug
Fixes: NuGet/Home#8601
Description
Re-enable support for deterministic pack
Make NuGet pack more determinisitic, by default.
This makes
Deterministicproperty (default =true) work again for nuget pack via msbuild.It adds support for
DeterministicTimestampproperty (default =true) too.For more details about the properties and the design goals, see:
PR Checklist