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

dotnet pack creates odd dates in package using version 3.0.100-preview9-014004 #3388

Closed
phillipsj opened this issue Sep 17, 2019 · 44 comments
Closed

Comments

@phillipsj
Copy link

dotnet pack creates odd dates in package using version 3.0.100-preview9-014004

After switching to the version listed above our nuget packages started having very strange dates. The first date that is being displayed is inside of the nuget packages before being unpacked. The date is 12/29/1899.

image

Once we unpack the package, the date changes to 12/29/1979.

image

This only started occuring after we upgrade from 3.0.100-preview7-012821 to 3.0.100-preview9-014004.

@Macromullet
Copy link

I have confirmed this issue persists in RC1

@livarcocc
Copy link

@rrelyea

@Macromullet
Copy link

This seems to be related to deterministic packages introduced in preview 9:

https://docs.microsoft.com/en-us/nuget/release-notes/nuget-5.3
NuGet/Home#6229

Commit:
NuGet/NuGet.Client@4590967

@Macromullet
Copy link

Just confirmed using
dotnet pack -p:Deterministic=false
serves as a workaround, but it's odd that the default is to enable deterministic builds.

@DamianEdwards
Copy link
Member

I reproduced the behavior locally on one of my existing projects too.

@nkolev92
Copy link
Contributor

nkolev92 commented Sep 17, 2019

The deterministic property is set in the project itself.

The compiler respects the <Deterministic>true</Deterministic> property, and starting with 3.0 so does NuGet at pack time.

https://docs.microsoft.com/en-us/nuget/reference/msbuild-targets#deterministic

1980 was chosen as the min zip supported date.

The reason behind that if the customer wants deterministic binaries at build time, having deterministic packages is important too.

As @Macromullet linked NuGet/Home#6229 contains the reasoning.
Spec at https://github.com/NuGet/Client.Engineering/blob/master/Specs/Deterministic-Pack.md

@phillipsj
Copy link
Author

@nkolev92 we don't have that property set at all in our project. Why would it default to being enabled?

@nkolev92
Copy link
Contributor

@phillipsj
Copy link
Author

So it is set to true by default? That is a huge change IMO to make it default to true. Since it defaults to an older date we have packages that are not getting updated on deployment of our web app because of how deployment tools like web deploy uses the dates.

@nkolev92
Copy link
Contributor

@phillipsj

How are you deploying? What's the tooling used for deployment purposes?

//cc @tmat

@Macromullet
Copy link

I echo Jamie's sentiments. The change is sensible, but breaking, and I'm not sure customers have been made fully aware of how this changes deployments in Azure. I did find that prop, and did notice that it was set to true by default, which is why I commented on it. Windows and many other deployment systems use modified time as a way to know what should be overwritten. If you don't know what's newer (if it's an alpha or beta binary and you're using semantic versioning, then then major.minor.build may not increase).

@phillipsj
Copy link
Author

@nkolev92 we are packing with Azure DevOps using the SDK.

@rrelyea
Copy link

rrelyea commented Sep 17, 2019

@Macromullet @phillipsj - @nkolev92 is asking about the "tooling used for deployment purposes" so we can understand more details when @phillipsj said:

Since it defaults to an older date we have packages that are not getting updated on deployment of our web app because of how deployment tools like web deploy uses the dates.

What are repro steps that show that this is a significant problem?

@DamianEdwards
Copy link
Member

@phillipsj where does it go after that? I can see how this will effect deployments where assets are moved through a pipeline as a NuGet package, and then expanded out in another step and treated as loose files again, because their dates will now be fixed.

@DamianEdwards
Copy link
Member

Hypothetical example of a build pipeline that will be broken by this:

  1. Build a csproj into a nupkg and mark the nupkg as the stage output
  2. Take the previous stage output and put it on a feed
  3. Download the latest version of the package from the feed
  4. Expand the package and then run a copy to a different target output (e.g. folder) based on file timestamps so you only copy what's newer

In this situation, you'll never get the new copies of the files as they went through a format that doesn't ever update the timestamps of the contents.

@phillipsj
Copy link
Author

@rrelyea @DamianEdwards @nkolev92 so we are building our packages, then we push to our NuGet feed. We have web projects that consume these feeds. When we deploy the dependent projects some are deployed using Web Deploy, some Zip Deploy, and some with the new run from package in Azure. The run from package deployments work as expected because of the virtual filesystem. However, ones where files get overwritten if newer fail, because the newer versions have 1980 but the prior versions before this was toggled just two previews ago have newer dates because this wasn't enabled.

So 1.1.1 is 9/1/19 and 1.1.2 is 12/31/1980. That doesn't work in a many deployment scenarios.

@phillipsj
Copy link
Author

phillipsj commented Sep 17, 2019

@DamianEdwards nailed the situation and that is the default for things like Web Deploy and Zip Deploy.

@tmat
Copy link
Member

tmat commented Sep 17, 2019

@DamianEdwards

Expand the package and then run a copy to a different target output (e.g. folder) based on file timestamps so you only copy what's newer

This step seems odd to me. I'd expect the version of all binaries to be always higher in the newer NuGet package, so there are no savings in comparing timestamps and this step could just as well overwrite all files.

@phillipsj
Copy link
Author

@tmat the versions are higher, but many deployment tools don't compare the versions they compare the timestamps. Web deploy is a good example of this behavior.

@tmat
Copy link
Member

tmat commented Sep 17, 2019

@phillipsj Sounds like an issue with those tools then. It'd be good to file issues and follow up on those tools.

Relying on timestamp of files built on a different machine is very fragile as it depends on the build server time settings relative to the time settings of the machine that reads the time stamps.

@DamianEdwards
Copy link
Member

Thanks @phillipsj, we're looking at this now internally to figure out next steps.

@DamianEdwards
Copy link
Member

@tmat this might indeed be true (and we get stung but it ourselves when dogfooding really fresh builds) but the reality is this is how a lot of our default deployment experiences work, so this is potentially very impactful.

@tmat
Copy link
Member

tmat commented Sep 17, 2019

@DamianEdwards Sure, I'm not disagreeing with that. I'm just saying we need to fix the deployment tools.

@DamianEdwards
Copy link
Member

@phillipsj would you be able to provide as much detail as possible about how an AzDO pipeline (or pipelines) and project(s) has to be setup in a minimal fashion to reproduce this issue? We're just trying to make sure we understand the circumstances that this will occur given our default experiences.

@rrelyea
Copy link

rrelyea commented Sep 17, 2019

Preparing a disable it for now fix: NuGet/NuGet.Client#3058
Still deliberating

@phillipsj
Copy link
Author

phillipsj commented Sep 17, 2019 via email

@DamianEdwards
Copy link
Member

We've narrowed this down and understand it now. We've confirmed that the file timestamp remains unchanged from the point of dotnet pack all the way through to a project's publish output. This is indeed super impactful to any system that looks at publish output and uses file timestamps to determine what to copy to the destination. This includes Web Deploy which is used by Visual Studio Publish to IIS/Azure, VS for Mac, Azure CLI, AzDO publish, etc.

We're looking at making a fix for this in 3.0 to effectively disable the feature in NuGet (see @rrelyea's comment above) and I assume they'll need to revisit/iterate on their design for deterministic package build in a future update.

In the meantime, folks can work around this by disabling deterministic builds in their project files:

<PropertyGroup>
    <Deterministic>false</Deterministic>
</PropertyGroup>

Any package that's been with an SDK with this behavior though will be "poisoned" and need to re-built and re-published. We'll look at if we can do a proactive analysis of whether any such packages exist on nuget.org and reach out to those owners.

@phillipsj
Copy link
Author

phillipsj commented Sep 17, 2019 via email

@nguerrera
Copy link
Contributor

Longer term, would it make sense to have nuget update the timestamps on unpack to global packages folder, and go back to the fixed date in the zip metadata?

@nkolev92
Copy link
Contributor

@nguerrera
Thinking out loud, zip doesn't require us to set the timestamps, so one solution might be not setting the timestamps at all and just setting them at extraction.

That hasn't been investigated though, NuGet/Home#8601 for updates :)

@phillipsj
Copy link
Author

phillipsj commented Sep 17, 2019 via email

@Macromullet
Copy link

@nguerrera
Thinking out loud, zip doesn't require us to set the timestamps, so one solution might be not setting the timestamps at all and just setting them at extraction.

That hasn't been investigated though, NuGet/Home#8601 for updates :)

My assumption was that having different timestamps in the zip would affect the zip bytes (since the timestamps are encoded in the bytes that the zip represents), so that if you were to keep the timestamps, then the same build/pack scenario would lead to a different byte stream representation, thereby making it non-deterministic. I assumed that's why the change was made in the first place.

@Macromullet
Copy link

Longer term, would it make sense to have nuget update the timestamps on unpack to global packages folder, and go back to the fixed date in the zip metadata?

Even if you do this, you might zero the bytes, but to re-apply the dates on unpack, it assumes an additional metadata file that contains that data. Since that data comes during the pack process from the file, and is encoded in the zip, then any mutation of the original file would mutate this metadata file, thereby changing the zip file as well.

@nkolev92
Copy link
Contributor

nkolev92 commented Sep 18, 2019

@Macromullet

My assumption was that having different timestamps in the zip would affect the zip bytes (since the timestamps are encoded in the bytes that the zip represents), so that if you were to keep the timestamps, then the same build/pack scenario would lead to a different byte stream representation, thereby making it non-deterministic. I assumed that's why the change was made in the first place.

My suggestion was no timestamps in the zip :)
Then at extract time, the timestamps will be set to now.

@Macromullet
Copy link

@Macromullet

My assumption was that having different timestamps in the zip would affect the zip bytes (since the timestamps are encoded in the bytes that the zip represents), so that if you were to keep the timestamps, then the same build/pack scenario would lead to a different byte stream representation, thereby making it non-deterministic. I assumed that's why the change was made in the first place.

I was suggestion, no timestamps in the zip :)
Then at extract time, the timestamps will be set to now.

OK that makes more sense, but I feel that leads to ancillary issues, namely that you cannot know when anything in a nuget file was built. Maybe the argument is that you shouldn't, but all tooling up to this point has worked under the assumption that when those files come out, the date is useful. If you were to set the date on unpack, for one you'll have to push those files on every deploy, since the timestamp will always be new. Worse, if you put in a replacement file on the server that's newer manually, then redeploy (i'm not suggesting this is a good idea), then it would always overwrite it. It violates common publishing semantics.

@nkolev92
Copy link
Contributor

namely that you cannot know when anything in a nuget file was built.

Which at its core is counter to determinism.

We'll definitely factor in that input into our future work for pack determinism.

@phillipsj
Copy link
Author

phillipsj commented Sep 18, 2019 via email

@phillipsj
Copy link
Author

BTW, I would like it if we can ignore timestamps, unfortunately, legacy is catching us on this.

@tmat
Copy link
Member

tmat commented Sep 18, 2019

@phillipsj Making the timestamp settable just shifts the problem elsewhere but it does not address the problem.

I think what NuGet pack could do is to calculate the timestamp based on the NuGet package version. That would make it monotonically increasing for release packages. Pre-release packages that do not differ in the 3-part number would still get the same timestamp. Would that be ok?

In any case I think the deployment systems still need to be updated to stop relying on timestamps.

@phillipsj
Copy link
Author

@tmat That is a reasonable solution too.

@nguerrera
Copy link
Contributor

I am very unfamiliar with these deployment technologies, but naively, I don't actually understand why you would ever want to deploy based on newer timestamp or newer versions. If I downgrade a package reference due to a bug that broke my app, shouldn't it deploy the older binaries from my newer source?

@Macromullet
Copy link

I think a potential mitigating factor is that the deployment technologies can be diverse. We are using webdeploy in some places, but In others we favored Run From Package, primarily to avoid locking, but the atomicity perks were also helpful. I’m wondering if this is even an issue for that given its behavior.

For thick client apps most people use MSI, and things certainly get worse there. Major/Minor/Revision are the only things that are counted as I recall with a tiebreaker on modified time. This certainly throws some of that out of whack.

@nkolev92
Copy link
Contributor

nkolev92 commented Sep 19, 2019

We have disabled the deterministic pack feature in the GA .NET Core version.

NuGet/Home#8601 is the issue to follow for further pack + determinism updates.

edit
I guess this issue can be closed now?

NickCraver pushed a commit to DapperLib/Dapper that referenced this issue Sep 21, 2019
See dotnet/core#3388 on <Deterministic>false</Deterministic> required to work around a preview9/rc1 bug.
@scalablecory
Copy link
Contributor

Closing issue, it appears it was resolved in Nuget.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

9 participants