richardszalay / helix-publishing-pipeline

Unified publishing for Sitecore Helix solutions that extends existing Visual Studio and command line workflows
MIT License
68 stars 18 forks source link

Support publish of content and contentFiles with NuGet PackageReference format #50

Open coreyasmith opened 5 years ago

coreyasmith commented 5 years ago

For Sitecore 9.1 and beyond, it's recommended to convert the ASP.NET Web Application projects in your solution to use the NuGet PackageReference format for reasons I won't expound here. After you convert an ASP.NET Web Application project to use the PackageReference format, config files in NuGet packages (e.g., Unicorn, Synthesis, Glass Mapper) will no longer be automatically added in to your project when those NuGet packages are installed or updated. Instead, you have to track down the configs in the %USERPROFILE%\.nuget\packages folder and manually copy them in to your project. This makes it easy to miss critical config changes when upgrading NuGet packages.

The ASP.NET Core Web Application project type does not suffer from this problem. When an ASP.NET Core Web Application project is published, all assets in the contentFiles directory of the NuGet packages that the project references are automatically published to the publish target. Due to this mechanism, it is not necessary for assets in contentFiles to be added to ASP.NET Core Web Application project types.

It would be awesome if HPP handled publishing of assets in the content and contentFiles folders of referenced NuGet packages when using the PackageReference format with the ASP.NET Web Application similar to the way they are handled for ASP.NET Core Web Application projects. This feature should not be enabled for ASP.NET Web Application projects still using the packages.config format. I think there should be some intelligence for packages with configs/assets duplicated in both the content and contentFiles folders for backwards compatibility (e.g., Synthesis) so that a double publish doesn't occur, if possible.

richardszalay commented 5 years ago

Great idea. I propose the following rules:

Sound good?

coreyasmith commented 5 years ago

Sounds great.

ezlateva commented 5 years ago

hey guys, the solution I'm working with right now is configured with PackageReferences and I've hit the same problem @coreyasmith described above. I'm wondering if this feature request is already being worked on as it sounds so much better than building my own custom build steps. If not, I'd be happy to take a stab at it

richardszalay commented 5 years ago

@ezlateva just to clarify, if the PackageReference with the content a reference of Website or one of the modules?

ezlateva commented 5 years ago

in my case, both actually

richardszalay commented 5 years ago

Interesting. Looking the NuGet.targets, the content files should get published. I wonder if it doesn't follow that path when running via a legacy (non-SDK) csproj

coreyasmith commented 5 years ago

Yes I think that's the case @richardszalay.

richardszalay commented 5 years ago

In which case, solving it for Website should be simple enough as we can trigger the same target and/or Task that gets used by sdk projects.

Modules will be more difficult. Need to think it through a little more.

richardszalay commented 5 years ago

I'll look into it again as soon as I can. If either of you want to do the same, to figure it what's executing I recommend the MSBuild binary log viewer.

richardszalay commented 5 years ago

I've had a little time to investigate this further:

Files in content/ are not supported at all in the PackageReference model. Since the're used by Unicorn and pretty much every other legacy-supporting package, we should definitely support this.

Files in contentFiles/ are explicitly skipped by both Visual Studio and NuGet CLI restores when a legacy project is using PackageReference`. I propose we do not support contentFiles due to:

  1. It's clearly intended only for .NET Core projects
  2. It supports additional complexities like language specific transforms

If/when Sitecore supports .NET Core, we can review the above.

Thoughts?

In both cases, adding support for content/ in PackageReferences on the Website project is on the lower end of effort but supporting PackageReferences of referenced modules is a bit more complex.

NB. We should definitely use the project lock file rather than the PackageReference items, since the former includes downstream references and the latter does not.

coreyasmith commented 5 years ago

I think that by the time all of Sitecore is running on .NET Core, we'll be using project formats that support all of the Package Reference features natively and HPP won't need to support contentFiles.

Focusing on publishing only files in the content folder will definitely be a big win. However, it'd still be nice to support publishing from the contentFiles folder just because I think it promotes better development habits.

richardszalay commented 5 years ago

My only issue is that supporting it properly (ie. without breaking a bunch of packages), we'd have to reimplement all of the tokenisation transform support. content/ is much easier to support since it only supports a bunch of files. It does support config transforms, but we can leave for a future release.

richardszalay commented 5 years ago

Here's my plan for the implementation:

  1. Create a custom task that:
    1. Parses the lock file (from $(ProjectLockFile))
    2. Finds packages that haven't been excluded (possibly via a custom ExcludeAssets metadata value?)
    3. Outputs a list of items as long as they don't exist in the current project
  2. Create a target that uses the new task to add the located content files into FilesForPackagingFromProject
  3. Create another target (in it's own file) that runs the task and adds the content files to Content, and configure this target with BeforeTargets="ContentFilesProjectOutputGroup"
  4. Update CollectFilesFromHelixModulesContent to inject the new targets file into the ContentFilesProjectOutputGroup invocation by using CustomBeforeMicrosoftCommonTargets

There will be no support for prioritising content from multiple projects that both include the same package. As with regular content files, this will generate a publish-time warning via WPP.