toddams / RazorLight

Template engine based on Microsoft's Razor parsing engine for .NET Core
Apache License 2.0
1.52k stars 259 forks source link

RazorLight requires ASP.NET Core runtime #360

Open Tyrrrz opened 4 years ago

Tyrrrz commented 4 years ago

Describe the bug

Because RazorLight has a FrameworkReference to Microsoft.AspNetCore.App, the user is expected to have the corresponding runtime. This is a show stopper for console or WPF apps as users of those will not have it installed (and shouldn't need to).

To Reproduce

  1. Create a WPF app
  2. Add RazorLight
  3. Publish it
  4. Try running on a machine where neither ASP.NET Core runtime nor SDK is installed

image

Expected behavior

It should work with just the base .NET Runtime.

Information (please complete the following information):

jzabroski commented 4 years ago

@Tyrrrz How should it work instead?

Tyrrrz commented 4 years ago

Well ideally it shouldn't depend on ASP.NET Core runtime because in the majority of cases you would want to run it outside of an ASP.NET Core environment. Taking dependency on an entire framework kinda clashes with the idea of it being a light and independent Razor engine.

jzabroski commented 4 years ago

@Tyrrrz I think you're misunderstanding my comment. You're asking for support for your specific deployment scenario but not providing me with your csproj PackageReference(s), etc. I take it your problem is in the master branch here: https://github.com/Tyrrrz/DiscordChatExporter ? The notion that a WPF app needs to be "published" also indicates there is something else going on beyond just using WPF.

RazorLight can run on .NET Core without ASP.NET Core. Some people have had success (somehow) running it on legacy .NET Framework. But it should run on .NET Core without ASP.NET Core, since that is the main use case: e.g., from a console app, send emails using RazorLight email templates.

Tyrrrz commented 4 years ago

@Tyrrrz I think you're misunderstanding my comment. You're asking for support for your specific deployment scenario but not providing me with your csproj PackageReference(s), etc. I take it your problem is in the master branch here: https://github.com/Tyrrrz/DiscordChatExporter ? The notion that a WPF app needs to be "published" also indicates there is something else going on beyond just using WPF.

Yes, that is correct. I'm not asking for support with my deployment, I'm letting you know about this behavior, which I assume is not intended.

Not sure what you mean by "something else going on beyond just using WPF". It's not about WPF, the same can be observed with console applications. Just to clarify, by "publishing" I'm talking about dotnet publish.

It can run on .NET Core without ASP.NET Core, but only if ASP.NET Core Runtime or SDK is installed. If you're testing on your dev machine, you will have it installed.

By using FrameworkReference (as in here: https://github.com/toddams/RazorLight/blob/cc9ab9674179c8f3d981704889474946df57df76/src/RazorLight/RazorLight.csproj#L38), you're referencing a shared framework. It doesn't resolve to some NuGet package (it simply doesn't exist), but instead it expects the runtime with all the required libraries to be installed where the application is running.

This problem seems to have appeared starting from 2.0.0-beta2.

jzabroski commented 4 years ago

Got it. Thanks for that help. Very helpful.

I got involved after 2.0.0-beta2, so it makes sense I was unaware of this disconnect.

jzabroski commented 4 years ago

The other remark I have here is that FrameworkReference(s) are supposed to be installed locally. It's also a misnomer to say RazorLight depends on the runtime; it depends on the framework, not the runtime. We only load the assemblies we need (although I shoud write an integration test to verify this behavior, as, to your point, it defeats the purpose if it doesn't)

Tyrrrz commented 4 years ago

It's also a misnomer to say RazorLight depends on the runtime;

These are mostly the same when it comes to .NET. By depending on the runtime, I mean that the end users need to have this installed:

image

jzabroski commented 4 years ago

This may be a gap in my knowledge, but isn't the correct term for those runtimes in the screenshot a "Runtime Pack"? In other words, the runtime is the actual VM. The Runtime Pack is the core dependencies. Both Desktop Runtime (Pack) and ASP.NET Runtime (Pack) still have dotnet.exe as an entry point. The whole of what FrameworkReference does is to allow GAC-like behavior so we're not installing all these dependencies several times via "Framework-Dependent Deployment". Microsoft employee Nick Guerrera wrote the design for this feature for .NET Core 3.x, including how RollForward works.

Tyrrrz commented 4 years ago

Haven't heard the term "Runtime Pack", but maybe. The point is that, in order to use a project that references RazorLight, the end user needs to have ASP.NET Core Runtime installed, in addition to other runtime they would otherwise need. Normally, if it's just a console application, they only need the base runtime. Similarly, if it's a WPF or WF application, they would only need a desktop runtime. With RazorLight, they will need ASP.NET Core Runtime (which contains base) or ASP.NET Core Runtime + Desktop runtime (which is not contained within ASP.NET Core Runtime). The VM and dotnet.exe are distributed with all of them, but they differ in the libraries, like you mentioned.

jzabroski commented 4 years ago

I do understand your point, I am just describing my understanding of the issues and why they arise, so that I can backtrack to what the most elegant solution is (unless you already have a solution in mind that works). Because the idea of removing FrameworkReference somewhat flies in the face of my understanding of how things should work. The other idea is that this is why I created #299 , because there are so many deployment scenarios to truly regression test against that maintaining high quality is quite tricky. I even bought a Mac Mini three weeks ago for $100 to troubleshoot some issues with building on Mac, because Apple doesn't support docker containers.

Tyrrrz commented 4 years ago

It's not entirely clear how to best solve it, especially since RazorLight could also be used from ASP.NET Core context. Ideally, if you could reduce the dependencies just to Microsoft.AspNetCore.Razor.Language and Microsoft.CodeAnalysis.CSharp, that would solve it, but I assume RazorLight depends on other parts of ASP.NET Core as well.

jzabroski commented 4 years ago

I believe that there are parts of the infrastructure that are effectively tightly coupled to the runtime pack itself, like Antiforgery token. I never dug into why exactly but it is what it is. I know that some of the leaders on Microsoft's front have improved the modularity in some respects but also made it more tightly coupled in other areas (e.g., to reduce misconfiguration issues and cut down on combinatorial explosion in "configuration test" cases).

toddams commented 4 years ago

There is definitely a way to get rid of shared framework, as I remember, FrameworkReference got here from some contributor in order to solve some bug or smth.

jzabroski commented 4 years ago

@toddams I looked at the blame and you're right. Should be easy fix. I will do it this weekend.

jzabroski commented 4 years ago

@Tyrrrz @toddams Just an update on this.

I think I almost got this all fixed.

The remaining error I am a bit stumped by and need to push through to figure out why its not working is this:

Severity Code Description Project File Line Suppression State Error CS1061 'IHostBuilder' does not contain a definition for 'ConfigureWebHostDefaults' and no accessible extension method 'ConfigureWebHostDefaults' accepting a first argument of type 'IHostBuilder' could be found (are you missing a using directive or an assembly reference?) RazorLight.Tests (netcoreapp3.0), RazorLight.Tests (netcoreapp3.1) D:\source\GitHub\RazorLightPRs\tests\RazorLight.Tests\Extensions\ServiceCollectionExtensionsTest.cs 93 Active

I found this StackOverflow post, but the advice (use Sdk="Microsoft.NET.Sdk.Web") doesn't seem to help us with a test project: https://stackoverflow.com/a/58097655/1040437 However, I do not understand WHY the Sdk project type must be <Project Sdk="Microsoft.NET.Sdk.Web"> - this especially doesnt make sense for RazorLight.Tests.csproj

Tyrrrz commented 4 years ago

Microsoft.NET.Sdk.Web is quite common on test projects, at least those that test web applications. As long as it's not on the actual project that gets packed into NuGet, it should be fine to use that, imo.

jzabroski commented 4 years ago

@Tyrrrz Doing that causes a lot of cshtml errors, because it invokes the whole magic of Visual Studio support for Razor Web projects, so I think it's a non-starter.

First you get these errors: image

Even if you delete WrongRazorSyntax.cshtml from the Assets/Embedded and Assets/Files folders, you get another slew of errors, so I don't think it's as simple as including Microsoft.NET.Sdk.Web, especially when a major point behind RazorLight is to not require the SDK:

image

karoberts commented 4 years ago

I tried removing this from RazorLight for core 3.0 and 3.1 <FrameworkReference Include="Microsoft.AspNetCore.App" /> and added this <PackageReference Include="Microsoft.AspNetCore.Razor.Runtime" Version="2.2.0" />

and then put this into RazorLight.Tests for core 3.0 and 3.1 <FrameworkReference Include="Microsoft.AspNetCore.App" />

After that, everything compiled and all unit tests pass. I did not see the errors shown above.

I think the only reason that the framework reference is necessary is because the tests project uses some aspnetcore types like Microsoft.AspNetCore.Hosting.IHostingEnvironment

schmitch commented 4 years ago

well adding Microsoft.AspNetCore.Razor.Runtime should probably work but the problem is that this package is not published for 3.x i.e. the 2.2.0 package might break in the future.

so using Microsoft.AspNetCore.App is safer, or it would be good to raise the issue on the dotnet core github.

jzabroski commented 3 years ago

@schmitch I looked into this, and I think using FrameworkReference is correct, but the missing piece is possibly guidance on adding trimming logic to the deployment so that we strip out stuff that isn't needed.

See: https://docs.microsoft.com/en-us/dotnet/core/deploying/trim-self-contained

Have you used this before?

@karoberts I think your approach has the downside of not getting any bugfixes for those libraries or using new C# language features.

jzabroski commented 3 years ago

@Tyrrrz I think the right way to handle a bloated FrameworkReference is to using trimming. That's literally what the guidance says to do. It stinks that FrameworkReference is not implemented as a meta-package that is stored on Nuget.org, but it is what it is. It also stinks that trimming is documented as an "experimental" feature. But, it is what it is, right?

As I mentioned in my earlier comment, this is what Microsoft wants us to do for what they call "framework-dependent deployments." And the documentation literally starts off saying its super successful model.

Tyrrrz commented 3 years ago

@jzabroski I think the big issue here is that by depending on ASP.NET Core, it puts a dependency on ASP.NET Core Runtime. Imagine you're building a desktop application in WPF that uses RazorLight, which makes it so your end users need to install both the desktop runtime AND the ASP.NET runtime (for seemingly no reason).

As I mentioned in my earlier comment, this is what Microsoft wants us to do for what they call "framework-dependent deployments." And the documentation literally starts off saying its super successful model.

I think you mean "self-contained deployments" (framework-dependent leads to the scenario I explained above), as it indeed seems to be what Microsoft is pushing for. Unfortunately, even with trimming, their file size is still too large (upwards of 100mb) for even the most basic apps and I don't think it will reduce much in the future.

jzabroski commented 3 years ago

Yes, I meant self-contained deployments. What are you doing in Razor.Mini to avoid pulling in all the dependencies?

I started thinking through how to trim dependencies through a cleaner architecture. See: https://github.com/toddams/RazorLight/commit/76611d0d4a44ea042befc923ef4a9383fbf746fa

In this approach, I basically create my own stubs for much of ASP.NET Core components, like IUrlHelperFactory and IUrlHelper. They are not 1 to 1, because it doesn't make sense (in my opinion) for RazorLight to have an ActionContext object. Instead, our mock ActionContext can just be:

  1. the Root Path (Namespace for EmbeddedResourceRazorProject or Folder for FileSystemRazorProject)
  2. plus the "Path Separator", and
  3. the current template Key.

This is fairly different from ASP.NET ActionContext, but at least for people using RazorLight in net472 / ASP.NET MVC 5 scenarios with plans to upgrade to .NET 5 in the future, it gives them a well-defined upgrade path.

Thoughts? Just brainstorming. (Always feel free to reach out to me as well to brainstorm any ideas you have).

Tyrrrz commented 3 years ago

What are you doing in Razor.Mini to avoid pulling in all the dependencies?

I only depend on Microsoft.AspNetCore.Razor.Language which is pulled from NuGet

https://github.com/Tyrrrz/MiniRazor/blob/3aee7cd74263bf83186098a0f6cc13434fe466a0/MiniRazor/MiniRazor.csproj#L26

knuxbbs commented 3 years ago

@karoberts I think your approach has the downside of not getting any bugfixes for those libraries or using new C# language features.

Getting rid of Microsoft.AspNetCore.App is the way to go. Portability is more important than new features in this case, and I don't think that trimming is related to this issue.

I think that the name of this library is RazorLight because it does not depends on ASP.NET Core runtime (like Microsoft.AspNetCore.App).

knuxbbs commented 3 years ago

Off topic, but compile templates at build time is a feature that RazorLight should have.

jzabroski commented 3 years ago

Off topic, but compile templates at build time is a feature that RazorLight should have.

OK, please add it :)

tebeco commented 3 years ago

so using Microsoft.AspNetCore.App is safer

yes

or it would be good to raise the issue on the dotnet core github.

they probably won't change it as it using NugetPackage in the first place was created tons of issue like the one that is discussed here. I would stick with FrameworkReference and Shared code + 2 project => 2 nugets if you want to get rid of FrameworkReference IE: you would break most consumer running AspNetCore stack today by pinning to lower version more and more "transitive"

tebeco commented 3 years ago

Getting rid of Microsoft.AspNetCore.App is the way to go. Portability is more important than new features in this case, and I don't think that trimming is related to this issue.

It kinda does, depending on a FrameworkReference when using SelfContained just grow your artifact size. I don't see other impact it could have. If you remove FrameworkReference, then don't replace by any older and unsupported package Msft.Anc.* AFAIK, since August 21, 2021 2.1 reached EOL, which was the last 2.x LTS / supported, which means it's probably even worse to go that way (possible CVE + breaking application with MissingMethodException You would need to get rid of all Microsoft.AspNetCore.* that is not 3.1.x (LTS) / Net6.0 (RC1 => GoLive + LTS) / net5.0 (EOL in ~february 2022)

tebeco commented 3 years ago

I think you mean "self-contained deployments" (framework-dependent leads to the scenario I explained above), as it indeed seems to be what Microsoft is pushing for. Unfortunately, even with trimming, their file size is still too large (upwards of 100mb) for even the most basic apps and I don't think it will reduce much in the future.

I would pay the few MB at the price of stability and no CVE (see previous point) If you're shipping for IOT I understand your pain though, if you're shipping container / webapplication / ... then it's probably fine until someone find code replacement (not pinning old libraries)

But I really think few MB should not block 2.0 here until someone figure out a safe way.