Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

allow creating deterministic packages #2775

Open
wants to merge 6 commits into
base: dev
from

Conversation

Projects
None yet
7 participants
@ctaggart
Copy link

commented Mar 26, 2019

For NuGet/Home#6229, following the notes in there. Additional tests should be added. I'd like to know of all existing ones pass, so I'm creating the PR.

{
if (_deterministic)
{
using (var hashFunc = new Sha512HashFunction())

This comment has been minimized.

Copy link
@tmat

tmat Mar 26, 2019

Contributor

SHA256 should be sufficient.

This comment has been minimized.

Copy link
@ctaggart

ctaggart Mar 26, 2019

Author

I agree, but that is what was available in NuGet.Packaging. If they jumped up from netstandard1.6 to netstandard2.0, the other crypto classes could be used.

This comment has been minimized.

Copy link
@nkolev92

nkolev92 Apr 8, 2019

Member

NuGet cross-targets net472 & netstandard2.0, but using the already available hash function one is enough.

@tmat

This comment has been minimized.

Copy link
Contributor

commented Mar 26, 2019

Looks like a reasonable approach to me.

@ctaggart ctaggart changed the title allow creating determinstic packages allow creating deterministic packages Mar 26, 2019

@ctaggart

This comment has been minimized.

Copy link
Author

commented Mar 26, 2019

How do I get CI to run for this?

@ctaggart ctaggart referenced this pull request Mar 27, 2019

Merged

add nuget lock #104

ctaggart added some commits Mar 29, 2019

@ctaggart

This comment has been minimized.

Copy link
Author

commented Mar 29, 2019

@nkolev92 Can you enable CI to run for this?

@zivkan

This comment has been minimized.

Copy link
Contributor

commented Apr 1, 2019

@ctaggart I'm not sure if you can see the list of checks (I know you can't open the details link), but there are 2 end to end test failures, 72 mac failures, 80 windows unit test failures, and 335 windows functional test failures. All of the failures I looked at were new failures starting at your commit. I've been looking for a way to download the test results xml so you can check it yourself, but haven't found a way yet. Here's a small sample of the failures:

NuGetExe_Caching_AllowsMissingPackageOnSource

System.ArgumentOutOfRangeException : Index and length must refer to a location within the string.\r\nParameter name: length

   at System.String.Substring(Int32 startIndex, Int32 length)
   at NuGet.Packaging.PackageBuilder.GenerateRelationshipId(String path) in E:\A\_work\184\s\src\NuGet.Core\NuGet.Packaging\PackageCreation\Authoring\PackageBuilder.cs:line 969

Most of the errors I saw were related to this ArgumentOutOfRangeException coming from NuGet.Packaging.PackageBuilder.GenerateRelationshipId, so if you can fix that, maybe we'll be lucky and many errors will disappear with this fixed.

Here's a different test failure: EnsureProjectFactoryWorksAsExpectedWithReferenceOutputAssemblyValuesComplex

nuget.exe DID NOT SUCCEED: Ouput is Attempting to build package from 'Link.csproj'.\r\nMSBuild auto-detection: using msbuild version '15.9.21.664' from 'C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\MSBuild\15.0\bin'.\r\nBuilding project 'E:\A\_work\184\s\.test\work\65ba780c\102fb556\Link\Link.csproj' for target framework '.NETFramework,Version=v4.5'.\r\nMicrosoft (R) Build Engine version 15.9.21+g9802d43bc3 for .NET Framework\r\nCopyright (C) Microsoft Corporation. All rights reserved.\r\n\r\nBuild started 3/29/2019 4:43:59 PM.\r\nProject "E:\A\_work\184\s\.test\work\65ba780c\102fb556\Link\Link.csproj" on node 1 (default targets).\r\nPrepareForBuild:\r\n Creating directory "out\".\r\n Creating directory "obj\Debug\".\r\nProject "E:\A\_work\184\s\.test\work\65ba780c\102fb556\Link\Link.csproj" (1) is building "E:\A\_work\184\s\.test\work\65ba780c\102fb556\A\A.csproj" (2:2) on node 1 (default targets).\r\nPrepareForBuild:\r\n Creating directory "out\".\r\n Creating directory "obj\Debug\".\r\nProject "E:\A\_work\184\s\.test\work\65ba780c\102fb556\A\A.csproj" (2:2) is building "E:\A\_work\184\s\.test\work\65ba780c\102fb556\C\C.csproj" (4:2) on node 1 (default targets).\r\nPrepareForBuild:\r\n Creating directory "out\".\r\n Creating directory "obj\Debug\".\r\nProject "E:\A\_work\184\s\.test\work\65ba780c\102fb556\C\C.csproj" (4:2) is building "E:\A\_work\184\s\.test\work\65ba780c\102fb556\D\D.csproj" (5:2) on node 1 (default targets).\r\nPrepareForBuild:\r\n Creating directory "out\".\r\n Creating directory "obj\Debug\".\r\nGenerateTargetFrameworkMonikerAttribute:\r\nSkipping target "GenerateTargetFrameworkMonikerAttribute" because all output files are up-to-date with respect to the input files.\r\nCoreCompile:\r\n C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\MSBuild\15.0\bin\Roslyn\csc.exe /noconfig /nowarn:1701,1702 /nostdlib+ /highentropyva+ /reference:"C:\Program Files (x86)\Reference Assemblies\Microsoft\Framework\.NETFramework\v4.5\mscorlib.dll" /reference:"C:\Program Files (x86)\Reference Assemblies\Microsoft\Framework\.NETFramework\v4.5\System.Core.dll" /debug+ /out:obj\Debug\D.dll /ruleset:"C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\Team Tools\Static Analysis Tools\\Rule Sets\MinimumRecommendedRules.ruleset" /subsystemversion:6.00 /target:library /utf8output Source.cs "C:\Users\dlab14\AppData\Local\Temp\.NETFramework,Version=v4.5.AssemblyAttributes.cs"\r\n Using shared compilation with compiler from directory: C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\MSBuild\15.0\bin\Roslyn\r\nCopyFilesToOutputDirectory:\r\n Copying file from "obj\Debug\D.dll" to "out\D.dll".\r\n D -> E:\A\_work\184\s\.test\work\65ba780c\102fb556\D\out\D.dll\r\n Copying file from "obj\Debug\D.pdb" to "out\D.pdb".\r\nDone Building Project "E:\A\_work\184\s\.test\work\65ba780c\102fb556\D\D.csproj" (default targets).\r\nGenerateTargetFrameworkMonikerAttribute:\r\nSkipping target "GenerateTargetFrameworkMonikerAttribute" because all output files are up-to-date with respect to the input files.\r\nCoreCompile:\r\n C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\MSBuild\15.0\bin\Roslyn\csc.exe /noconfig /nowarn:1701,1702 /nostdlib+ /highentropyva+ /reference:"C:\Program Files (x86)\Reference Assemblies\Microsoft\Framework\.NETFramework\v4.5\mscorlib.dll" /reference:"C:\Program Files (x86)\Reference Assemblies\Microsoft\Framework\.NETFramework\v4.5\System.Core.dll" /debug+ /out:obj\Debug\C.dll /ruleset:"C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\Team Tools\Static Analysis Tools\\Rule Sets\MinimumRecommendedRules.ruleset" /subsystemversion:6.00 /target:library /utf8output Source.c

EnsureProjectFactoryDoesNotAddFileThatIsAlreadyInPackage

System.IO.InvalidDataException : The file is not a valid nupkg. File path: E:\A\_work\184\s\.test\work\a13aebdd\feab334c\Assembly.1.0.0.nupkg\r\n---- System.IO.FileNotFoundException : Could not find file 'E:\A\_work\184\s\.test\work\a13aebdd\feab334c\Assembly.1.0.0.nupkg'.

   at NuGet.Packaging.PackageArchiveReader..ctor(String filePath, IFrameworkNameProvider frameworkProvider, IFrameworkCompatibilityProvider compatibilityProvider) in E:\A\_work\184\s\src\NuGet.Core\NuGet.Packaging\PackageArchiveReader.cs:line 126
   at NuGet.CommandLine.ProjectFactoryTest.<EnsureProjectFactoryDoesNotAddFileThatIsAlreadyInPackage>d__7.MoveNext() in E:\A\_work\184\s\test\NuGet.Clients.Tests\NuGet.CommandLine.Test\ProjectFactoryTest.cs:line 482

If anyone knows how to download the test results xml files from Azure DevOps, I'll be happy to attach them here.

@ctaggart

This comment has been minimized.

Copy link
Author

commented Apr 1, 2019

I'm not sure if you can see the list of checks (I know you can't open the details link), but there are 2 end to end test failures, 72 mac failures, 80 windows unit test failures, and 335 windows functional test failures. All of the failures I looked at were new failures starting at your commit.

@zivkan Flying blind here. Any change you can make the tests public? I am unauthorized:

image

An alternative would be to commit a azure-pipelines.yml that we can use in our forks.

@zivkan

This comment has been minimized.

Copy link
Contributor

commented Apr 1, 2019

That's why I copy/pasted 3 of the failures here. From my looking around, I believe all the unit test failures to be caused from the GenerateRelationshipId ArgumentOutOfRangeException, and probably 90% of the windows functional tests.

You should be able to run the unit tests locally. By default if you run our build.ps1 script without the -SkipUnitTest argument, it will run the unit tests locally. I've also had good luck running the tests in Visual Studio.

@ctaggart

This comment has been minimized.

Copy link
Author

commented Apr 1, 2019

Thanks @zivkan. Installing net 472 targeting pack and will run them in VS.

@ctaggart

This comment has been minimized.

Copy link
Author

commented Apr 2, 2019

@zivkan I can't actual build the dev branch and the corresponding tests. I created NuGet/Home#7945 with details.

@ctaggart

This comment has been minimized.

Copy link
Author

commented Apr 2, 2019

@zivkan I fixed the GetHashBytes and all the NuGetExe_Caching_AllowsMissingPackageOnSource tests pass locally. Can someone please kick of CI again?

@ctaggart

This comment has been minimized.

Copy link
Author

commented Apr 2, 2019

Looks like an improvement. Only 3 test suites failing now instead of 4. Tests on Mac succeeded.

image

Not sure why Functional Tests on Windows failed to run.

image

@zivkan

This comment has been minimized.

Copy link
Contributor

commented Apr 2, 2019

I think our CI didn't successfully update GitHub Checks. When I look at the build, all Windows Functional Tests were green. The 1 failed test on EndToEnd has been failing for many builds, so unrelated to your change. Apex tests fail due to environment. So, I think it's all good now.

@ctaggart

This comment has been minimized.

Copy link
Author

commented Apr 8, 2019

What does it take to get this merged? Does it still need review by more people?

@nkolev92

This comment has been minimized.

Copy link
Member

commented Apr 8, 2019

@ctaggart We need some reviews(I am actually reviewing it now) and approval internally when some people are back from their vacation next week.

@zivkan

This comment has been minimized.

Copy link
Contributor

commented Apr 8, 2019

I just had the thought that this has probably only added the feature into nuget.exe pack. NuGet.Build.Tasks.Pack should also be changed to work with msbuild /t:pack. Does Roslyn have a MSBuild property to enable deterministic builds? If so, we should probably use the same property name.

@nkolev92 Do we expect community contributors to make their contributions work with nuget.exe, pack targets and dotnet cli, or do we allow an "incomplete" implementation and we fill in the rest later? Do you know if we have to change NuGet.CommandLine.XPlat to prepare for a new dotnet pack argument, or does dotnet pack work just off the pack targets?

{
if (_deterministic)
{
using (var hashFunc = new Sha512HashFunction())

This comment has been minimized.

Copy link
@nkolev92

nkolev92 Apr 8, 2019

Member

NuGet cross-targets net472 & netstandard2.0, but using the already available hash function one is enough.

var entry = package.CreateEntry(entryName, CompressionLevel.Optimal);
entry.LastWriteTime = lastWriteTime;
var entry = CreateEntry(package, entryName, CompressionLevel.Optimal);
if(!_deterministic)

This comment has been minimized.

Copy link
@nkolev92

nkolev92 Apr 8, 2019

Member

format: Use braces.

private ZipArchiveEntry CreateEntry(ZipArchive package, string entryName, CompressionLevel compressionLevel)
{
var entry = package.CreateEntry(entryName, CompressionLevel.Optimal);
if (_deterministic)

This comment has been minimized.

Copy link
@nkolev92

nkolev92 Apr 8, 2019

Member

format: use braces


private static string ToHexString(byte[] arr)
{
return BitConverter.ToString(arr).ToLower().Replace("-", "");

This comment has been minimized.

Copy link
@nkolev92

nkolev92 Apr 8, 2019

Member

Why ToLower?

@tmat

This comment has been minimized.

Copy link
Contributor

commented Apr 8, 2019

Does Roslyn have a MSBuild property to enable deterministic builds? If so, we should probably use the same property name

Yes, Deterministic is the existing property.

@nkolev92

This comment has been minimized.

Copy link
Member

commented Apr 8, 2019

The pack targets should probably define something like PackDeterministic and that would default to Deterministic.

@tmat

This comment has been minimized.

Copy link
Contributor

commented Apr 8, 2019

The pack targets should probably define something like PackDeterministic and that would default to Deterministic.

Seems unnecessary to me. Either your build is deterministic or not. I don't see a point of having part of the build deterministic and other part (packaging) non-deterministic, or vice versa.

Note that if the compiler does not produce deterministic DLL then your package won't be deterministic no matter what, as it already contains non-deterministic bits.

@plynkus

This comment has been minimized.

Copy link

commented Apr 9, 2019

First---thank you, Cameron, for taking this on. Deterministic packages will help the workflow here substantially.

I pulled latest from ctaggart:deterministic, gave it a quick run here with determinism toggled on and ran into one issue with a CLI pack test. A forced _unixEpoch at year 1970 results in an exception:

The DateTimeOffset specified cannot be converted into a Zip file timestamp.
Parameter name: value

... which relates to limits documented here.

The exception appears to be thrown by ZipArchiveEntry, referring to constants atop ZipHelper that match the documented limits.

A quick change from 1970 -> 1980 and a retest resolved the issue.

@rrelyea

This comment has been minimized.

Copy link
Contributor

commented May 13, 2019

Yes, thanks @ctaggart - we'd love to make pack deterministic.
@zivkan - no we should not take partial submissions of features - it builds up too much debt.
@ctaggart - looks like you haven't yet followed up on the latest feedback from @nkolev92

design wise, i'd lean towards agreeing with @tmat and just be more deterministic, rather than have a switch. If that isn't a slam dunk, let's talk it through.

@ctaggart

This comment has been minimized.

Copy link
Author

commented May 13, 2019

@rrelyea Any body willing to take this across the finish line to make it complete? The date should be switched to be compatible as @plynkus suggested. Tests, more targets, and minor formatting should be added as @nkolev92 suggested.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.