Orchard icon indicating copy to clipboard operation
Orchard copied to clipboard

Solution housekeeping => Turn off CopyLocal and cleanup unused references

Open Xeevis opened this issue 10 years ago • 36 comments

I believe Orchard could use some major solution house keeping as it's currently quite messy, it's unnecessarily prolonging build operations and creates hundreds of megabytes of duplicate assemblies (for source it's around 300 MB extra, for deployed it's around 10 MB). I also wonder if sheer excess of unused references isn't perhaps slowing down application startup? Idk.

Unused references

Almost all projects have references to assemblies they are not even using, for example Orchard.MessageBus

Microsoft.CSharp <- Unused NHibernate <- Unused (also duplicates it's assembly locally in source) Orchard.Core <- Unused + 4 more unused references to GAC assemblies

Cleanup of these should be relatively easy using tools like ReSharper (right click Modules solution folder > Refactor > Remove unused references).

There are also redundant usings in source files, would be nice if these were avoided, tho it's not really an issue.

CopyLocal

1.x has seen optimization for Orchard.Core and Orchard.Framework references which greatly reduced size and improved build speed, but it's only partial optimization as modules still create duplicates of one another. Just Orchard.Tokens.dll is present 28 times in 1.9 stable release folder, wouldn't it be better if it was there just once?

I propose Copy local is set to false to all but 3rd party libraries that are unique to module in question and their presence in bin or dependency folder can't be guaranteed, false can be set because:

  • If it's a module reference, manifest based module dependency ensures presence of referenced assembly so no need for local copies
  • Orchard.Framework has reference to libraries like NHibernate, log4net, Autofac, Newtonsoft.Json, System.Web.Mvc etc these are sure to be present in root bin folder, so there is no need for any module to have local copy of these

Final touches

  • To see difference in size you have to make sure you have bin/obj folders removed before rebuilding as Visual Studio won't clean it up automatically.
  • Views won't like this, their intellisense won't work and they'll throw errors if they are using assemblies which are not copied locally

Compiler Error Message: CS0012: The type 'Orchard.MediaLibrary.Models.ImagePart' is defined in an assembly that is not referenced. You must add a reference to assembly 'Orchard.MediaLibrary, Version=1.9.0.0, Culture=neutral, PublicKeyToken=null'. Source File: C:\Websites\Orchard\Modules\Orchard.Layouts\Views\EditorTemplates\Elements.Image.cshtml Line: 6

This is fixed by adding entries to module's web.config so for example Orchard.Layouts\web.config would look like:

<system.web>
    <compilation targetFramework="4.5.1">
        <assemblies>
            (...)
            <add assembly="Orchard.Framework"/>
            <add assembly="Orchard.Core"/>
            <add assembly="Orchard.MediaLibrary"/>
        </assemblies>
    </compilation>
</system.web>

Xeevis avatar May 12 '15 22:05 Xeevis

Ok, sounds great, can you create a fork with your suggestions?

sebastienros avatar May 14 '15 19:05 sebastienros

Maid here, I spent all night dusting and I think I'm done. It appears to be working flawlessly (no guarantees), there may still be some views that error out with missing assembly reference. It's a rare case tho and is quickly fixed, I couldn't figure out a way to find them automatically and going through every single view one by one manually, let's just say I don't get paid enough. :no_good: :grin:

I also avoided touching Tests as there appears to be an issue with NUnit Test Adapter & Copy Local 635abd7456470c1ff6d7398963b01bef684bfa1e

Xeevis avatar May 15 '15 02:05 Xeevis

This seems to be an awesome improvement. I just cloned your fork and did a build from scratch and it actually seems significantly faster. Good call to leave the unit test projecs alone.

I wonder if we might take this opportunity to also fix the Razor view situation for Roslyn. It seems Roslyn is a lot more picky with references, requiring more things to be directly referenced. For example I get a lot of these errors when opening up Razor views in VS 2015 RC:

Error CS0012 The type 'WebViewPage<>' is defined in an assembly that is not referenced. You must add a reference to assembly 'System.Web.Mvc, Version=5.2.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35'.

Any thoughts on that @Xeevis?

DaRosenberg avatar May 15 '15 15:05 DaRosenberg

I'm afraid I don't have experience with Roslyn or VS 2015, but unless something changed it should be picking up this reference from web.config ... Is there anything that would have effect on appearance of this error? Like does changing Copy Local on System.Web.Mvc fix it?

Xeevis avatar May 15 '15 15:05 Xeevis

Good suggestions! Setting CopyLocal=true on the System.Web.Mvc reference fixed about half of the errors. The other half was fixed by adding this reference to Web.config:

<add assembly="System.Core, Version=4.0.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35"/>

Can you test if this causes any problems in VS2013?

DaRosenberg avatar May 15 '15 16:05 DaRosenberg

Adding reference to System.Core in web.config throws

Could not load file or assembly 'System.Core, Version=4.0.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35' or one of its dependencies. The system cannot find the file specified.

It can't be even added as reference, it's implicit, that's why it's probably not necessary in VS2013

A reference to 'System.Core' could not be added. This component is already automatically referenced by the build system.

CopyLocal shouldn't be necessary and would be best if it could be avoided. It should be able to resolve everything through web.config so there may still be something else maybe missing there?

Xeevis avatar May 15 '15 17:05 Xeevis

Adding System.Core as a project file reference shows the same error message in VS2015. But adding it as a Web.config reference works fine. Weird that it doesn't for you - is the System.Core assembly not even in the GAC on your machine?

Agree that CopyLocal should be avoided if possible... perhaps you're on to something, that it's one of System.Web.Mvc's indirect dependencies that are missing. I'll dig some more and see if I can track it down.

DaRosenberg avatar May 16 '15 14:05 DaRosenberg

I think everything would go south if it wasn't in GAC :). It's there, just under different PublicKeyToken b77a5c561934e089 which works for me.

I'm shooting blind now, but maybe that can be somehow relevant to your problem with System.Web.Mvc? Different tokens on some dependencies? Maybe it would be best to check with gacutil.exe what else has changed.

Thinking out loud, if you need to specify System.Core I'm afraid it's not possible to make it fit both environments, actually I think you'd just lock it to single one. Maybe that's why it's not permitted to be explicitly referenced? It should just use what is available to remain portable. At least that's how I understand it. So my uneducated guess is either something is missing in web.config, different tokens, or bug in VS or compiler :).

Xeevis avatar May 16 '15 15:05 Xeevis

OK so to summarize, it seems there are two separate issues with VS2015.

  1. The Razor editor doesn't get that System.Web.Mvc is actually there unless it's CopyLocal=true. It matters not that it's referenced both in the project file and in Web.config - the editor still complains. I have tried also adding project file references to all of its dependencies like Microsoft.Web.Infrastructure and System.Web.WebPages and so on, but even if I add them all, it still doesn't work. I'm at my wit's end as to why - maybe I should file a bug report on Connect and see what the VS team says.
  2. In addition, the Razor editor needs a Web.config reference to System.Core for some other types to be brought in properly. Simply adding this one works great for me - but I'm a little fuzzy as to why that doesn't work for you? You say you get an exception, but when and where? Are we talking YSOD?

DaRosenberg avatar May 16 '15 15:05 DaRosenberg

I did a little experiment. Added a brand new MVC project to the solution in VS2015. The project template configured everything based on MVC 5.2.3 and .NET Framework 4.5.2. All the non-GAC assemblies were referenced from Nuget packages with CopyLocal=true and the Views/Web.config file looked like this fresh out of the template:

  <system.web.webPages.razor>
    <host factoryType="System.Web.Mvc.MvcWebRazorHostFactory, System.Web.Mvc, Version=5.2.3.0, Culture=neutral, PublicKeyToken=31BF3856AD364E35" />
    <pages pageBaseType="System.Web.Mvc.WebViewPage">
      <namespaces>
        <add namespace="System.Web.Mvc" />
        <add namespace="System.Web.Mvc.Ajax" />
        <add namespace="System.Web.Mvc.Html" />
        <add namespace="System.Web.Optimization"/>
        <add namespace="System.Web.Routing" />
        <add namespace="WebApplication1" />
      </namespaces>
    </pages>
  </system.web.webPages.razor>

  <appSettings>
    <add key="webpages:Enabled" value="false" />
  </appSettings>

  <system.webServer>
    <handlers>
      <remove name="BlockViewHandler"/>
      <add name="BlockViewHandler" path="*" verb="*" preCondition="integratedMode" type="System.Web.HttpNotFoundHandler" />
    </handlers>
  </system.webServer>

  <system.web>
    <compilation>
      <assemblies>
        <add assembly="System.Web.Mvc, Version=5.2.3.0, Culture=neutral, PublicKeyToken=31BF3856AD364E35" />
      </assemblies>
    </compilation>
  </system.web>

Everything works fine of course, runtime as well as tooling.

Then I go through and set CopyLocal=false on all references. Sure enough Razor editor breaks down completely.

Until we've tracked down what the tooling philosophy is around this on the new compiler platform, I'm a little wary of merging this pull request. If it turns out that everything will break on Roslyn even after it RTMs, and that the VS tooling will depend on these assemblies being CopyLocal=true with no available workaround, then it's definitely not worth the trade-off to sacrifice Razor editor functionality for file size and build time, and as a consequency we will have to revert most of these changes.

On VS2013 (old compiler platform) it seems the workaround of adding <add assembly ... > elements to Web.config works, but if this workaround will not be viable going forward on Roslyn with VS2015 and beyond. Therefore I think we should investigate and be reasonably sure there will be a way to get this working on Roslyn before we merge this PR.

It's worth noting that what Orchard does is pretty unorthodox, i.e. all modules are web projects and considered by VS to be their own executing host applications, whereas at runtime they are actually not executed as such but rather compiled dynamically into the hosting Orchard.Web application and their files served by virtue of being subfolders physically. Probably it's reasonable to expect that this approach will always necessitate some compromises, such as being forced to duplicate some files on the file system in order to keep both the tooling and runtime stories happy.

DaRosenberg avatar May 16 '15 16:05 DaRosenberg

  1. So does it error only for the System.Web.Mvc or other references as well? CopyLocal is set to false in dev branch already on Orchard.Core and Orchard.Framework, do you get errors in Razor for these too? (Make sure solution is built :wink: )
  2. Yes it's YSOD (when set in root web.config), for modules it's error in logs, apparently your System.Core is signed with different private key from my System.Core so effectively different assemblies. So I guess if you specify Core the way you do, it will throw YSOD for everyone else not running on new environment unless they change PublicKeyToken to match theirs. That can't be right.

This new MVC project you created doesn't have System.Core in web.config either, I still don't think it should be there. Damn new pre-release software ... :grinning:

Xeevis avatar May 16 '15 16:05 Xeevis

  1. No errors for anything else that I've seen yet, only for System.Web.Mvc - although I can't be sure, might simply be because types no from Orchard.Core or Orchard.Framework are directly referenced from the Razor code.
  2. OK, gotcha. Yeah, sounds very weird indeed that there are two versions of the same assembly with different keys.

In the new MVC project I created, the System.Core reference is actually there under the References node as a project-level reference. I think that's why it doesn't need to be in Web.config in my case. Question is, how the heck did it end up there considering the error message we are both seeing when trying to add it to an existing module project?

DaRosenberg avatar May 16 '15 17:05 DaRosenberg

That may be a clue. Maybe project needs it in references and then it's not necessary in web.config, and I can see that this reference doesn't need PublicKeyToken so it might work everywhere.

Since Visual Studio won't let you add it, can you try to add reference to it manually in csproj file (and delete it's entry from web.config) and see if it solves the Razor issue with System.Core stuff?

Look at how it's added in that new MVC project, should look like

<Reference Include="System.Core">
  <RequiredTargetFramework>3.5</RequiredTargetFramework>
</Reference>

Xeevis avatar May 16 '15 17:05 Xeevis

Tried adding the ´System.Core` reference manually - had no effect.

I guess the next logical step is to simply eliminate the differences between an existing module and a newly created MVC project one by one until the module starts behaving right, and figure out the key difference.

DaRosenberg avatar May 16 '15 22:05 DaRosenberg

Maybe you have to bump up taget .NET Framework or something, I won't do a guesswork as it can be number of things as almost everything has changed there. I'll see what I can do to set up my own testing environment to help with solving that.

I however found a view that is using both Orchard.Framework and module Orchard.MediaLibrary at

\src\orchard.web\modules\orchard.imageeditor\views\content-imageeditor.cshtml

In dev branch Framework has CopyLocal set to false and module is set to true. Are there errors in this view? Does it matter if it's true or false? I'm trying to determine if problem comes from web.config being ignored to resolve references for views or if it's problem unique to System.Web.Mvc.

Xeevis avatar May 17 '15 15:05 Xeevis

Has there been any progress on this issue since May? It seems like a useful change to work towards if we can overcome the VS2015 Roslyn problems. Would make patching live production environments easier if module DLLs were duplicated throughout the install base.

mjy78 avatar Sep 14 '15 00:09 mjy78

As I understand from the above comments there are some problems with removing Copy Local = True. So here's a change I made today to the build process which reduces the duplication of DLLs in modules and themes. Seems to work pretty well and results in the Precompiled build being about 25% smaller.

Here's the commit https://github.com/mjy78/Orchard/commit/df40d590ea588d86717969273ae194542cc85521

Would it be worthwhile at least pulling this in to clear up the Precompiled build output? It's not going to save time building, but at least the end result will be cleaner.

mjy78 avatar Sep 14 '15 06:09 mjy78

VS2015 appears to be messing this up, build works, it's lean, builds fast and is considerably lesser wear & tear for SSDs ... just unable to get full intellisense back for .cshtml views. Copy Local for Orchard.Framework and Orchard.Core are already disabled at least in dev branch so I assume intellisense is already broken for everybody on VS2015 in distro modules right?

Xeevis avatar Sep 14 '15 10:09 Xeevis

So on VS2013 everything is working fine? I'm still on VS2013 and would gladly do with faster build times and less SSD wear.

mjy78 avatar Sep 14 '15 11:09 mjy78

Yes on VS2013 everything worked fine for me. You can try this fork and see for yourself https://github.com/Xeevis/Orchard/tree/cleanup

Xeevis avatar Sep 14 '15 12:09 Xeevis

Thanks. I merged in your changes and apply similar changes to some of my own custom modules and some other third party modules that I have in my solution. One thing I've observed in my production environment with these changes, is that some views fail with the exception...

The type or namespace name 'Users' does not exist in the namespace 'Orchard' (are you missing an assembly reference?)"

To resolve this I had to place a copy of the Orchard.Users.dll in the bin folder of my custom module and restart the app. I've had to to this in a few cases where views in modules are accessing namespaces from other modules. The modules do have a reference to these other modules, but I do have Copy Local = False. What's interesting is the code runs fine from the debug/dev environment.

I thought all DLLs are copied to the App_Data/Dependencies folder and run from within there?

mjy78 avatar Sep 14 '15 23:09 mjy78

I've discovered another alternative to setting Copy Local = true on these references that are needed within razor views is to add the assembly to the web.config in the system.web / compilation / assemblies section. See below as example where a view in a module tries to access Orchard.Users.Models.UserStatus...

  <system.web>
    <compilation targetFramework="4.5.1">
      <assemblies>
        <add assembly="System.Web.Abstractions, Version=4.0.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35"/>
        <add assembly="System.Web.Routing, Version=4.0.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35"/>
        <add assembly="System.Data.Linq, Version=4.0.0.0, Culture=neutral, PublicKeyToken=B77A5C561934E089"/>
        <add assembly="System.Web.Mvc, Version=5.2.3, Culture=neutral, PublicKeyToken=31bf3856ad364e35"/>
        <add assembly="System.Web.WebPages, Version=3.0.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35"/>
        <add assembly="Orchard.Users"/>
      </assemblies>
    </compilation>
  </system.web>

mjy78 avatar Sep 15 '15 05:09 mjy78

@mjy78 The issue with this approach is that the Web.configs should be then maintained as well. Currently pretty much every module Web.config is the same, but this would introduce something new to care about.

Piedone avatar Sep 15 '15 11:09 Piedone

@mjy78 Yes I know, I mentioned that at the end of the original post.

@Piedone Yes that is a con, but a small one in my opinion. If module creators add reference it's automatically with copy local and it will work. If they want to slim their modules down, they can remove copy local and add assembly entry into web.config, it's purely optional optimization.

It's not often you add or remove references and if you do, you just have to remember that if you change copy local or remove reference you have to modify web.config as well. I think it's a small price to pay for not having lots of duplicate assemblies all over the place. Given intellisense works correctly, which I still have issue with on VS2015, on VS2013 it works perfectly. :disappointed:

Xeevis avatar Sep 15 '15 13:09 Xeevis

I too think it's ok to add the reference in the web.config only. At the same time, I don't see how referencing Users in a custom module has much impact on the overall dll duplication. It would only happen if lots of modules depends on your custom module.

Do we have a list of these references where we need to decide between Copy Local and web.config reference?

sebastienros avatar Sep 15 '15 17:09 sebastienros

@sebastienros The Orchard.Users reference was only a problem with one custom module of my own making.

A few cases where core modules had to have assembly references added in web.config (which @xeevis has done in the fork above) are Orchard.Layout needing Orchard.MediaLibrary to avoid...

System.Reflection.TargetInvocationException: Exception has been thrown by the target of an invocation. ---> System.Web.HttpCompileException: c:\inetpub\orchard.1.9\Modules\Orchard.Layouts\Views\Elements\Image.cshtml(3): error CS0234: The type or namespace name 'MediaLibrary' does not exist in the namespace 'Orchard' (are you missing an assembly reference?)

and Orchard.DynamicForms needing a reference to Orchard.Layouts to avoid...

System.Reflection.TargetInvocationException: Exception has been thrown by the target of an invocation. ---> System.Web.HttpCompileException: c:\inetpub\orchard.1.9\Modules\Orchard.DynamicForms\Views\Elements\Form.cshtml(3): error CS0234: The type or namespace name 'Layouts' does not exist in the namespace 'Orchard' (are you missing an assembly reference?)

mjy78 avatar Sep 15 '15 23:09 mjy78

@mjy78 That error should only occur with Pre-compile enabled on publish.

see https://github.com/OrchardCMS/Orchard/issues/5605

dcinzona avatar Sep 16 '15 18:09 dcinzona

Hey guys. I was wondering where did we get to with this issue? I am building a site in Orchard, and I can see that after doing build precompiled, my build directory gets to a whopping 1,7 GB and builds take a long time. I only have 1 extra theme and a few minor tweaks. The precompiled version is only about 80MB. Really not sure what's going on, but i suspect that CopyLocal is wreaking havoc on my poor SSD. The deployments in general could use a bit of streamlining and ease. I realize it's not Orchard issue, same thing happens on this other project I work on with lots of projects. I see massive space for improvement here, currently it's just too clumsy. Is it possible to have some subset Solution that would include just some custom themes, modules and related modules? And everything else could compile to a separate directory and simply copied? I think that would be a great addition. Like StarYourDevelopmentHere.sln?

brunoAltinet avatar Dec 10 '15 11:12 brunoAltinet

@brunoAltinet I've continued in my product environment with the approach above that @Xeevis started.

Over time there have been a few more missing assembly references (similar to the exceptions above in my previous comment) in a few core modules and custom modules that have popped up. I've resolved these as I've become aware of them by adding the assembly to the web.config for that particular module. I don't have a complete list of the core modules that I've updated at hand, but it's not many.

This approach has certainly resulted in faster build (Precompiled) times and cleaner/smaller builds. I'm still running on VS2013, but I believe one downfall with the approach I've taken with VS2015 is that unless copylocal=true you loose razor intellisense, although that may have been resolved since the latest VS2015 service pack.

mjy78 avatar Dec 10 '15 11:12 mjy78

As far as I can remember new Orchard.Dashboards needed Orchard.Layouts in config. Still no dice with Razor IntelliSense & VS2015. Since it's only for views and most views are dynamic I chose to have cleaner, faster and smaller solution over occasional whispering voice. I wouldn't be mad if VS team fixed this in the future tho :blush:.

Xeevis avatar Dec 10 '15 12:12 Xeevis