dotnet / sdk

Core functionality needed to create .NET Core projects, that is shared between Visual Studio and CLI
https://dot.net/core
MIT License
2.59k stars 1.03k forks source link

Publish skips copying files that are different but not newer. #10754

Open cheenamalhotra opened 4 years ago

cheenamalhotra commented 4 years ago

Hi Team,

I'm coming from issue dotnet/sqlclient#445 where providing --runtime linux-x64 fails to load DLL from runtimes\unix\... folder.

RID ref: https://github.com/dotnet/runtime/blob/master/src/libraries/pkg/Microsoft.NETCore.Platforms/runtime.json

Is that expected behavior?

janvorli commented 4 years ago

cc: @ericstj

ericstj commented 4 years ago

Can you provide a repro that demonstrates what you claim is broken?

Here's how things are expected to work:

So if you can provide a simple repro, or the items listed here we can help you understand what's going wrong.

cheenamalhotra commented 4 years ago

Repro is available in original issue, copied below:

Clone this repo: https://github.com/davedx/sqlclient-repro

Then run:

docker build -t efgs -f ./Dockerfile.custom.dotnet.3.1 . docker run efgs

ericstj commented 4 years ago

The bug here is that this docker file is publishing both a framework-dependent and a standalone app to the same directory.

https://github.com/davedx/sqlclient-repro/blob/755f9ac2a43c422b275015d28ba9633956e3d741/Dockerfile.custom.dotnet.3.1#L16-L19

dotnet build -c Release -o /app dotnet publish -c Release -o /app --runtime linux-x64

The latter produces a self-contained app which is expecting to run without probing for runtimes, however the former already a bunch of files to the root directory which aren't runtime-specific.

Here's the log from the publish:

Skipping target "_CopyResolvedFilesToPublishPreserveNewest" because all output files are up-to-date with respect to the input files.

So this skips copying everything because it's all the same or older than the files copied by the previous build. To fix this use a clean directory for publishing with Runtime.

So this appears to be by-design since you're reusing the same directory. If you disagree the correct place for a bug is https://github.com/dotnet/sdk

davedx commented 4 years ago

Hi @ericstj, original bug reporter here, thanks for your reply!

Now that I understand what is happening here it makes sense. It did cost us a quite some time to get to this point because previously, running dotnet build then dotnet publish behaved differently (publish pulled in the platform dll's) so this dockerfile worked properly for us for the last 2 years without any issues. I think, re-reading the documentation again [1][2] it would be good to have a warning about combining both build and publish like we were and what the consequences of this can be, because it's quite a big gotcha and it's difficult to diagnose what's going wrong.

"Skipping target "_CopyResolvedFilesToPublishPreserveNewest" because all output files are up-to-date with respect to the input files."

Thinking about it, I find the behaviour that it skips copying dependencies because it thinks they are already in the output directory kind of weird, and actually even incorrect? If it's supposed to copy the dependencies for the specified platform but does not verify that the correct platform specific versions of the dependencies are there, that seems like a bug to me... I think I will file a bug on the sdk repo.

[1] https://docs.microsoft.com/en-us/dotnet/core/tools/dotnet-build [2] https://docs.microsoft.com/en-us/dotnet/core/tools/dotnet-publish

ericstj commented 4 years ago

I tend to agree. Let’s ask the SDK to change behavior of publish to copy if different rather than newer.

Kuinox commented 2 years ago

Hello, I was trying to figure out why dll in the publish folder does not follow a dependency downgrade. After extensive trouble shooting, I found this issue by searching if "_CopyResolvedFilesToPublishPreserveNewest" had any issue.

This behavior is dangerous because it lead to producing incorrect publish folder: Any CI that wont perform any cleaning won't be able to downgrade a dependency and will keep shipping the lastest DLL.

Kuinox commented 2 years ago

Hello, @marcpopMSFT Sorry for the ping, but it looks like this issue want unseen because it has 0 label due to being transfered.

marcpopMSFT commented 2 years ago

Publishing multiple types of builds to the same output folder is inherently dangerous. We generally use file timestamps to detect if newer and the entire incremental build system is based on that. @rainersigwald might have some other answer but I think our answer is don't publish to the same folder as that seems fairly avoidable. By default we put them into different folders. Maybe we could add checking for the output directory but that may cause other issues.

rainersigwald commented 2 years ago

I agree that the "publish multiple types to the same folder" scenario here isn't interesting, but I think the "downgrade a reference to the same assembly" case is interesting.

For assemblies, I don't think PreserveNewest makes that much sense as a behavior. It makes sense for things like config files because as you're debugging you might edit one + make a code change and rebuild and be sad if the config-file edit didn't get preserved.

It looks like this was done intentionally with dotnet/sdk#2684, but I think we missed this scenario.

Kuinox commented 2 years ago

@marcpopMSFT While "publish multiple types to the same folder" was the repro given here, I don't use it at all.
Like @rainersigwald said, my main issue is downgrading dependencies, and publishing with default settings.
This is a bug that currently annoy me a lot in my workflow, and probably cause issues in some CI workflows.

marcpopMSFT commented 2 years ago

@rainersigwald are you suggesting for assemblies we don't go based on timestamp like we do for everything else? Is there risk if we change that logic of getting the wrong files in the output more often? Could an incremental clean help or an implicit clean?

dsplaisted commented 2 years ago

@rainersigwald Can you expand on what the "downgrade a reference to the same assembly" case is? Pretty much everything in MSBuild is based on timestamps, would other things not be broken anyway? Does build not suffer this issue because it will usually delete the newer file with incremental clean first?

Kuinox commented 2 years ago

Can you expand on what the "downgrade a reference to the same assembly" case is?

Take a project with a single package dependencies, run publish, downgrade the minor (like 0.10.1 to 0.10.0), publish again, the publish folder withh contain the dll to the 0.10.1 dll.

First publish: image Second publish: image

rainersigwald commented 2 years ago

@rainersigwald are you suggesting for assemblies we don't go based on timestamp like we do for everything else? Is there risk if we change that logic of getting the wrong files in the output more often? Could an incremental clean help or an implicit clean?

Sort of. The Copy task has a SkipUnchangedFiles parameter that does exactly what we'd want for assemblies: it won't touch their timestamps if they are up-to-date, but will copy either older or newer assemblies over.

This is disabled in the default PreserveNewest infrastructure "because the application may want to change one of these files and not have an incremental build replace it."

https://github.com/dotnet/msbuild/blob/fb700f90493a0bf47623511edf28b1d6c114e4fa/src/Tasks/Microsoft.Common.CurrentVersion.targets#L5092-L5095

And that makes sense up to a point, but breaks this fairly-reasonable scenario.

Could an incremental clean help or an implicit clean?

Incremental clean would not, because the file should be present in the output. If we could detect the downgrade and run a clean before building, that would fix it but I don't see how to guarantee that.

jeremybparagon commented 1 year ago

I ran into this this week with an upgrade, upgrading Npgsql from Version 6.0.8 to Version 7.0.0, because the upgraded version is older than the lower version. We use dotnet publish as part of an automated process to prepare an Azure Batch job.

enkelmedia commented 7 months ago

I had the same issue as well on CI, installed a dependency (v1.5), and then had issues and wanted to go back to v1.4. All worked fine on the dev machine but changes were not included in the artifacts on the CI-machine. Quite a hard one to find and should really be fixed.