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

Downstream references are not included in publish unless used in code #31

Closed richardszalay closed 5 years ago

richardszalay commented 6 years ago

As originally raised by @bartlomiejmucha, downstream assembly references are only published if they are references by the Website project or if there's code that actually uses a type from the assembly. This is most apparent with Unicorn/Rainbow, since they're typically only referenced by config.

The current workarounds are:

I've held off on merging the PR since it relies on calling ResolveAssemblyReferences, which is already called during build, on each Helix module again during publishing. This is probably the single most expensive part of the standard build process behind compilation and I'd like to find a sneakier workaround if possible.

richardszalay commented 6 years ago

This has now been raised again by @coreyasmith, so should be the priority for the next release.

jeneaux commented 6 years ago

I also encountered this (via Glassmapper contained within a Foundation project).

bartlomiejmucha commented 6 years ago

Another question is: should assemblies from second level project be automatically included in publish or not? My PR resolves references only for modules directly referenced in WebRoot project. So if you reference Feature project to WebRoot, and Feature one has a reference to Foundation project, then Foundation references are not included in final publish unless Foundation project is directly referenced by WebRoot project.

ResolveAssemblyReferences is a good place because it resolves conflicts etc. However, maybe you find a better place for this. Maybe it is possible to write Resolved references to some temporary item group and then include to references for WebRoot compilation.

richardszalay commented 6 years ago

My PR resolves references only for modules directly referenced in WebRoot project

Yeah that's fine. It fits in with the existing model by using the modules as a source. I don't think indirect references are worth the added effort and complexity.

ResolveAssemblyReferences is a good place because it resolves conflicts etc. However, maybe you find a better place for this.

RAR is definitely the right way to do that work, but it's already called when building the individual modules so I'd like to see if I can avoid rerunning it during publish.

coreyasmith commented 6 years ago

How about new release with @bartlomiejmucha's fix in place, and then following that up with another release later to improve performance with your wizardry @richardszalay?

richardszalay commented 6 years ago

How about a beta release with the fix?

richardszalay commented 6 years ago

Published as 1.4.0-beta1

richardszalay commented 6 years ago

Looking at this further, I don't think there'll be a simpler workaround. ResolveAssemblyReferences uses the project reference output assembly as input and assemblies that it references (ie. in the IL), so there's no room for determining the additional assemblies without actually running ResolveAssemblyReferences again on the project (or possibly taking the assemblies from its output directly, but that seems hackier and more prone to error).

coreyasmith commented 6 years ago

Is the impact on build times that severe?

richardszalay commented 6 years ago

I don't think we'll know until we have something Habitat-sized that we can test against

coreyasmith commented 6 years ago

I installed HPP in Habitat and did some tests. With version 1.3.1, a full build took about 46 seconds, and an incremental build took about 7 seconds. I got the exact same performance with version 1.4.0-beta1. Great work @bartlomiejmucha!

The only strange thing I noticed is that Habitat doesn't seem to be affected by this issue. With both 1.3.1 and 1.4.0-beta1, a rebuild results in 174 files being copied to the /bin folder of the publish directory. I know that not all of the assemblies that are copied are referenced in a project--a solution search for Rainbow, for example, comes up with no using statements. Somehow Rainbow.dll is still published though.

On the other hand, the personal project I'm using HPP with is affected by this issue, and I have 6 more files copied to my /bin directory with 1.4.0-beta1 than 1.3.1. Rainbow.dll is one that only gets published with 1.4.0-beta1, but not 1.3.1.

The biggest difference between my personal project and Habitat is that Habitat is using the PackageReference format for NuGet whereas I'm using packages.config. I wonder if projects using the PackageReference format are somehow immune to this issue?

bartlomiejmucha commented 6 years ago

My personal project is also using packages.config and I had this issue, that's why I wrote a fix. It's worth to confirm if it works correctly with PackageReference without my fix.

bartlomiejmucha commented 5 years ago

I did a test on a fresh solution, with two projects. With packages.config the indirect references were not published in the output folder, however, when I changed to PackageReference, it worked.

I see that PackageReference is an evolving feature. For example with VS 15.7 my Unicorn.MSBuild nugget was not added to the output folder, according to my intention. But with VS 15.8 it is added by default.

That's why I'm not sure if it works because it's a feature or because it's a bug :)

kmac23va commented 5 years ago

I took a look at the PackageReference stuff, it's not as intuitive yet because the ASP.NET projects don't have that conversion option (I had to pop a class library project to see it). I think the less someone needs to hack a .csproj file to get started, the better; that said, if I'm missing an easier way to get things going, I'd love to know about it.

I did get the beta NuGet package and that worked just fine, I didn't notice any crazy increase in build times (though I'm still prototyping using it with CI on VSO and locally to find the optimum usages for us). If there's no "easy" way to do the PackageReference stuff, I think you could roll the beta out.

coreyasmith commented 5 years ago

@kmac23va if you want to start using PackageReference, there are a couple of Visual Studio extensions (like this one) you can install that will let you convert packages.config from the right-click menu. Also if you go to Tools > Options > NuGet Package Manager > General and change the Default package management format to PackageReference, any new projects you create (including ASP.NET projects) will use PackageReference instead of packages.config.

kmac23va commented 5 years ago

@coreyasmith Nice, thanks for the pointers. I made the change in my sample and it worked perfectly with the non-beta version.

kmac23va commented 5 years ago

Something interesting I found, after I converted a starting project from packages.config to PackageReference using the tool from @coreyasmith. I added Sitecore.Kernel and Sitecore.Mvc, no problem. But then I added Glass.Mapper.Sc.90, and the reference bit showed up, but none of the configs/code you expect (in App_Start and App_Config) came in. Anyone else run into this? I'm using the latest VS 2017 update.

richardszalay commented 5 years ago

Content files aren't copied to source using PackageReference, but they might be included with the published code.

coreyasmith commented 5 years ago

Yeah you'll run into that problem with Unicorn too. PackageReferences doesn't support the content folder where Glass Mapper and Unicorn put the config files in their NuGet packages. The maintainers will have to update their packages to also put the configs in a contentFiles folder to get support for PackageReferences.

richardszalay commented 5 years ago

I've written up a post on why this happens, if anyone is interested: https://blog.richardszalay.com/2019/04/21/unpublished-module-assemblies/