umbraco / Umbraco-CMS

Umbraco is a free and open source .NET content management system helping you deliver delightful digital experiences.
https://umbraco.com
MIT License
4.5k stars 2.69k forks source link

Nuget restore not updating files #10133

Closed drpeck closed 3 years ago

drpeck commented 3 years ago

If a colleague upgrades umbraco, when I pull and restore nuget packages, updated /umbraco/* files are not overwritten.

This issue became apparent as a result of the Default.cshtml change causing a runtime error after upgrades. See: https://our.umbraco.com/forum/using-umbraco-and-getting-started/105442-upgraded-to-8121-now-get-umbracowebeditorsbackofficemodel-does-not-contain-a-definition-for-iconcheckdata

Umbraco version

from 8.12.0 to 8.12.2

Reproduction

Bug summary

I don't know nuget package creation, but nuget restores don't seem to overwrite files in the same way that updates do.

Steps to reproduce

  1. Create an 8.12.0 Umbraco site, and commit to git (location 1) excluding the umbraco folder
  2. Clone that repo in to a differ folder (location 2)
  3. Location 1: Upgrade Umbraco to 8.12.2 and push commit
  4. Location 2: Pull latest, restore packages.

Expected result

The contents of /Umbraco/Views/Default.cshtml should be identical

Actual result

/Umbraco/Views/Default.cshtml in Location 2 includes iconHelper code on line 124, while it has been removed in Location 1.

drpeck commented 3 years ago

I suppose there is a question about if the Umbraco folder should or should not be included in Git or not. Many people don't.

If including it is a requirement then this isn't a bug. Is there an official view?

nul800sebastiaan commented 3 years ago

None of this has changed in years.

Indeed! We recommend you store the umbraco folder in git so new changes get distributed to the rest of your team.

We know many people disagree with this view but that's what we have for now, NuGet isn't particularly good at helping distribute content files like this, it's much more suitable for shipping "libraries".

As such, I'll close this as "works as intended" 😄

dawoe commented 3 years ago

Indeed! We recommend you store the umbraco folder in git so new changes get distributed to the rest of your team

@nul800sebastiaan are you sure about that ?

Because the documentation has been recommending us for years not to store the umbraco folders in source control.

https://our.umbraco.com/Documentation/Getting-Started/Code/Source-Control/index-v7#the-umbraco-folders https://our.umbraco.com/Documentation/Getting-Started/Code/Source-Control/#the-umbraco-folder

This how we have been source controlling umbraco for a long time.

drpeck commented 3 years ago

I guess this old nuget issue is pertinent. https://github.com/NuGet/Home/issues/3787

nul800sebastiaan commented 3 years ago

@dawoe interesting! Well I guess when I say "we" I meant "I" then. How do you deal with @drpeck's problem?

dawoe commented 3 years ago

Have not encountered such a issue yet. Only sites I have on 8.12 are on Cloud.

Dave

nul800sebastiaan commented 3 years ago

On previous sites? On Cloud.. we commit the umbraco folder 😅

dawoe commented 3 years ago

Yep..that's why I did not encounter the issue yet :-D

nul800sebastiaan commented 3 years ago

Understood, but I was drawing on your experience since you said:

This how we have been source controlling umbraco for a long time.

So.. you must have encountered this scenario before? 🤔

dawoe commented 3 years ago

@nul800sebastiaan

Probably the issue happened to us as well. But did not have the impact like it does now. But it explains now why we sometimes saw small backend issues appearing after upgrades (and not for all users)

drpeck commented 3 years ago

I think an interesting point is that this hasn't been raised as an issue before. I can only conclude that most people are committing the folder or working around the issue with a reinstall if the issue causes a critical error.

Could there be an Umbraco version number stored in the Umbraco folder. If that doesn't match the DLL version then an alert is shown? Maybe even just an extra health check that does a checksum comparison on files?

git clean will help resolve matters when this issue has been identified. Maybe a bit of documentation on troubleshooting is the best we can get?

nul800sebastiaan commented 3 years ago

@drpeck - I don't see a feasible solution like that, maybe the hack with a version marker file could work, but it's no guarantee that all the files are actually the correct version and it's just one more thing that can get out of sync. The only good way of doing it would be to hash all the files and do a check on each of them, but there's too many files to do that as well. Plus: that's exactly what git does really, really well already. 😄

dawoe commented 3 years ago

Hi @drpeck

I would be careful with git clean ? I just ran this command on one of my repositories : git clean -n -d -x And that shows that folders like Media and App_Data will be deleted as well. I needed to use -d -x otherwise nothing would be cleaned.

I asked around a bit in my network and it seems 90% of them is excluding the umbraco folder from source control.

Maybe including the Umbraco folder in your vs solution will fix it. You can include in your solution but exclude it from source control.

Will see if I can try that

rbottema commented 3 years ago

@dawoe We do it exactly the other way around: excluded in the solution, but included in source control.

drpeck commented 3 years ago

@nul800sebastiaan I don't think we can or should make it impossible for the /umbraco folder to be different to that shipped with the release. My thought was that perhaps we should try to make this issue more visible. It's not unreasonable to classify this as a user issue IMO, but I suspect it is a pretty common one that has flown under the radar.

@dawoe 's anecdotal suggestion of 90% roughly reflects my own experience. I've often found that some Umbraco installs seemed to be unstable for reasons unknown, and perhaps this is the reason. Perhaps there are 100s of people experiencing the same?

Having a file within Umbraco that just states the file version seems like a pretty pragmatic approach. This would suffer the fate of any other Umbraco file which is not updated on a restore. A backoffice notication on login, or a health check could the mismatch. Users could then manually restore the umbraco folder or it might even be possible to provide a button that automates this.

This is certainly not a hill I'm going to die on, it just strikes me as it might improve the developer experience.

It would be really useful to confirm the official Umbraco recommended approach for including the folder in VS and/or version control.

drpeck commented 3 years ago

The problem I've experienced with including the folder in VS is that unless you remember to remove and then re-include the folder, then new files (deep within the folder structure) might get excluded.

drpeck commented 3 years ago

@dawoe you're correct of course that a git clean might be too aggressive. It would make sense to identify the simplest approach to achieving this and include it in the upgrade instructions.

nul800sebastiaan commented 3 years ago

From experience: we've tried version markers for Forms in the past and it led to just as many problems as it solved in the end.

I am all for finding a solution for this, but as it stands, these workarounds are unfortunately not great. The reason really is that we're abusing NuGet for things we shouldn't. Copying in the new files is a powershell operation, which even at the best of times sometimes fails. There is no better way of doing this from NuGet at the moment, that I know of. We're also extremely wary of updating the NuGet logic as it is all too easy to make matters worse. (been there, done that, have the scars to prove it 😅).

As for VS: up to you, it doesn't solve anything IMO and indeed, new files don't get auto-included easily. If you're worried the files won't deploy in your automatic deployment: we have a props & targets files just for that use.

To conclude, I would suggest this is a documentation issue. Since we shouldn't rely on NuGet to help us here (it's not what it's for) the documentation should make people aware that it's a good idea to source control the Umbraco folder. The original recommendation was added here: https://github.com/umbraco/UmbracoDocs/pull/978 and I think it's missing this subtlety.

Additionally, I LOVE putting this folder in source control so I can easily see what HQ updated between versions and it would allow me to pinpoint some bugs I've encountered over the years.

dawoe commented 3 years ago

@drpeck you can included the umbraco folder and it's contents using a wildcard in your csproj file

<Content Include="umbraco\**\*.*" />

I have just tested this on a project, and when I delete the contents of the umbraco folder it will get restored on build.

@nul800sebastiaan personally I don't like the idea of source controlling more that 2800 files I am not going to change. I don't want to review a PR containing a Umbraco upgrade.

Shazwazza commented 3 years ago

As seb says, the main issue is using (abusing) the powershell copy logic to copy out all of the static asset files. This powershell execution isn't supported by nuget.exe which is a super annoying problem because trying to automate nuget installation for Umbraco becomes near impossible since only VisualStudio itself knows how to execute these custom powershell scripts.

The previous option is to include ALL static files as content files in the Nuget package. This was previously done many many many years ago and we ended up in this same discussion because due to the way Nuget works, it meant that it would add all of these files to your csproj which is a worse experience than we have today because you'd almost have to commit this to source control. This is why the powershell work around was done.

Fast forward to today and we have the same problem with Nuget and .netcore except there is now possibility for powershell work arounds. Instead we use MSBuild work arounds by using custom MSBuild target files to copy out all of the static files. This actually works quite well, is cross platform compatible and works with build servers because msbuild is just msbuild.

I would suggest that this same approach that we are using in netcore may very well work for the current netframework project. If this were the case we could remove all the custom old powershell stuff that isn't supported by nuget.exe and build servers could 'just work' because the msbuild scripts will be executed on build.

The work around for now for your own solutions (though this doesn't solve the automation issue) is to not commit these files and include in your readme that for any newcomers to the project, they will need to do a Update-Project UmbracoCms -reinstall to get started. This is what I do in Articulate too because I don't want to commit all of those files.

Would love to see if the msbuild approach would work for the current netframework project too. @bergmania what do you think?

bergmania commented 3 years ago

I haven't tested using the new target files in nuget, but I can imaging this should be due to .NET (Core). Instead, it could very likely be related to the SDK format for the *.csproj files.. So maybe it is required to update the SDK format, for allow this to work

dmce commented 3 years ago

Our experience is much like that of Shazwazza, however we cant use Update-Package UmbracoCMS -r commands on our build server. So we either just include the config and Umbraco folder files in source code or use some <Copy...> commands in the .csproj file.

We tried to only include the config folder in source control as that is one that has files that might change? It was nice that when reinstalling UmbracoCMS that it prompted if the file had been changed. This felt better until we had the build server problem

If we tried to continue using the config folder only in source, then is it possible making use of the new <contentFiles> rather than <content> in the nuget package might work for the Umbraco folder only? It supports copying immutable files into the project. Although i found it tough getting some clear guidance on this.

https://docs.microsoft.com/en-gb/nuget/reference/nuspec#including-content-files

mistyn8 commented 3 years ago

I've been banging my head against this with azure pipelines over the last couple of days.

https://our.umbraco.com/documentation/getting-started/Code/Source-Control/#the-umbraco-folder

As it says here, I'm using webDeploy in a build step, but the umbraco folder doesn't get restored. trying to debug and it seems that this in the package build folder should be working (only copy if not present also allows for adhok overrides in files specifically added to sourcecontrol)

      <Target Name="CopyUmbracoFilesToWebRootForDeploy" AfterTargets="CopyAllFilesToSingleFolderForMsdeploy">
        <PropertyGroup>
          <UmbracoFilesFolder>$(MSBuildThisFileDirectory)..\UmbracoFiles\</UmbracoFilesFolder>
        </PropertyGroup>
        <ItemGroup>
          <UmbracoFiles Include="$(UmbracoFilesFolder)**\*" />
        </ItemGroup>
        <Copy SourceFiles="%(UmbracoFiles.FullPath)" DestinationFiles="$(_PackageTempDir)\%(RecursiveDir)%(Filename)%(Extension)" Condition="!Exists('%(RecursiveDir)%(Filename)%(Extension)')" />
      </Target>

But isn't.. I see "CopyAllFilesToSingleFolderForMsdeploy" step in my deploy log.. but not the "CopyUmbracoFilesToWebRootForDeploy"

Messing around with targets in my csproj file I can get <Target Name="CopyUmbracoFilesToWebRootForDeploy" BeforeTargets="CopyAllFilesToSingleFolderForMsdeploy"> to show up but never afterTargets..

As I did see uSync restoring things, I noticed that the implementation is different there, following a dependson build/*.props file

<?xml version="1.0" encoding="utf-8"?>
<Project ToolsVersion="4.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
    <PropertyGroup>
        <CopyAllFilesToSingleFolderForPackageDependsOn>
          AdduSyncDataToOutput;
          $(CopyAllFilesToSingleFolderForPackageDependsOn);       
        </CopyAllFilesToSingleFolderForPackageDependsOn>

        <CopyAllFilesToSingleFolderForMsdeployDependsOn>
          AdduSyncDataToOutput;
          $(CopyAllFilesToSingleFolderForPackageDependsOn);       
        </CopyAllFilesToSingleFolderForMsdeployDependsOn>
    </PropertyGroup>
</Project>

And also nicely adding in ForPackage as well as Msdeploy for when msDeploy isn't the only option.

I'm also finding the same to be true for third party App_plugins, where really I should be able to let nuget handle restoring the files there too.. but again nada..

mistyn8 commented 3 years ago

@dawoe <Content Include="umbraco\**\*.*" /> I've tried that locally I believe that is restored because of the

<Target Name="CopyUmbracoFilesToWebRoot" BeforeTargets="AfterBuild">
    <!-- This copies the files listed in `AddUmbracoFilesToOutput` to the webroot during NuGet install and should -->
    <!-- not be altered to support automated builds, use `CopyUmbracoFilesToWebRootForDeploy` for that instead -->
    <PropertyGroup>
      <UmbracoFilesFolder>$(MSBuildThisFileDirectory)..\UmbracoFiles\</UmbracoFilesFolder>
    </PropertyGroup>
    <ItemGroup>
      <UmbracoFiles Include="$(UmbracoFilesFolder)**\*" />
    </ItemGroup>
    <Copy SourceFiles="%(UmbracoFiles.FullPath)" DestinationFiles="%(RecursiveDir)%(Filename)%(Extension)" Condition="!Exists('%(RecursiveDir)%(Filename)%(Extension)')" />
  </Target>

Looking in the logs again for msbuild in the azuredevops pipeline.. after build is too late in the process the msdeploy zip has already been created at that point .. and it is also restoring to the working directory rather than the obj\Release\Package\PackageTmp\ for packaging??? Which I think is what the intended msdeploy <Copy SourceFiles="%(UmbracoFiles.FullPath)" DestinationFiles="$(_PackageTempDir)\%(RecursiveDir)%(Filename)%(Extension)" Condition="!Exists('%(RecursiveDir)%(Filename)%(Extension)')" /> is set up to do.. athough isn't in my case :-(

Also trying the <Content Include="umbraco\**\*.*" /> with other folders like <Content Include="app_plugins\**\*.*" /> in vsstudio is good until you include another file, or nuget package.. and then Visual Studio in it's wisdom recreates the csproj file with all the separate includes.. and you have to revisit in sourcecontrol to reset. Also found that vs needs to unload/reload the project to pick up on new files that should be included by **\*.* if you manually add a file via the file system :-( (or forinstance when modelsbuilders generates a file)