OrchardCMS / Orchard

Orchard is a free, open source, community-focused Content Management System built on the ASP.NET MVC platform.
https://orchardproject.net
BSD 3-Clause "New" or "Revised" License
2.37k stars 1.12k forks source link

Solution housekeeping => Turn off CopyLocal and cleanup unused references #5272

Open Xeevis opened 9 years ago

Xeevis commented 9 years ago

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:

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>
sebastienros commented 9 years ago

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

Xeevis commented 9 years ago

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

DaRosenberg commented 9 years ago

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?

Xeevis commented 9 years ago

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?

DaRosenberg commented 9 years ago

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?

Xeevis commented 9 years ago

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?

DaRosenberg commented 9 years ago

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.

Xeevis commented 9 years ago

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 :).

DaRosenberg commented 9 years ago

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 commented 9 years ago

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.

Xeevis commented 9 years ago
  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:

DaRosenberg commented 9 years ago
  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?

Xeevis commented 9 years ago

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>
DaRosenberg commented 9 years ago

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.

Xeevis commented 9 years ago

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.

mjy78 commented 9 years ago

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 commented 9 years ago

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.

Xeevis commented 9 years ago

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?

mjy78 commented 9 years ago

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

Xeevis commented 9 years ago

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

mjy78 commented 9 years ago

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 commented 9 years ago

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>
Piedone commented 9 years ago

@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.

Xeevis commented 9 years ago

@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:

sebastienros commented 9 years ago

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?

mjy78 commented 9 years ago

@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?)
dcinzona commented 9 years ago

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

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

brunoAltinet commented 8 years ago

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?

mjy78 commented 8 years ago

@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.

Xeevis commented 8 years ago

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:.

petedavis commented 7 years ago

I am wondering if there is some merit in still committing some changes towards this PR. I just did a quick update to the Autoroute and AuditTrail modules and had the precompiled build for the Autoroute reduce from 1.45MB to 194KB and the AuditTrail bin folder reduce from 3.23 MB to 658KB.

The only change was to turn other Orchard modules to CopyLocal=False so that each module only has its module assembly, and any other nuget package reference that is not available as part of the orchard framework.

Piedone commented 7 years ago

It seems to me that we haven't completely covered all the arising problems here, see the notes about modifying Web.configs above.

petedavis commented 7 years ago

@Piedone I have read back over the comments to see how things are behaving with current tooling. With leaving CopyLocal to true on MVC assemblies, false for other orchard modules, and true for module specific libs and nuget packages. And then also adding required Orchard Module refrences in web.config, I have full intellisense in VS2017 and working precompiled build.

Is there something else I have missed? Just changing the copying around of other module references, and adding them to web.config it can make a substantial difference.

MVC assemblies are not in the precompiled bin folder when creating a build, so leaving copylocal to true for MVC assemblies doesnt have an effect on final build size.

MatteoPiovanelli-Laser commented 1 year ago

Sorry to reopen this after such a long time.

We were trying to make our releases smaller and speed up our startup times and found this.

We have 100+ custom modules, with a lot of cross dependencies. We found that if we just brought the .csproj files in the released folders for modules, their shapes seem to work fine at runtime, without having to change the module's web.config.

Do you see any drawback to this? We are currently testing this further to make sure there are not surprises and everythig actually works (we haven't covered every case yet, just a handful of important ones to validate this as a possible approach).

sebastienros commented 1 year ago

We found that if we just brought the .csproj files in the released folders for modules, their shapes seem to work fine at runtime, without having to change the module's web.config.

I don't understand what move where

MatteoPiovanelli-Laser commented 1 year ago

When we release our modules on servers we usually have a process that copies only some files, to limit the size of the releases. We never copy .csproj files, hence Orchard can't use them for discovery of dependecies on our "live" instances, unlike on our dev environments. For this reason, we were seeing an issue where, disabling most "copylocal" flags things would work normally locally, but error out on servers, unless we either started looking for those issues and changing (and then maintaining) each module and theme's web.config, or copied their csproj, so that it could be used for dll discovery by Orchard's DefaultExtensionCompiler.

We are currently also testing a configuration where we have either VS2022 or msbuild precompile every shape of every module and theme in our solution. That comes with a few of its own messes, because the only way we found for this to work, right now, is to move eveyr module and theme's dll to Orchard.Web's bin folder (as it is the solution's main project) before attempting to build the cshtmls, otherwise the Razor compiler will complain it is unable to find references.