Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RazorLight requires ASP.NET Core runtime #360

Open
Tyrrrz opened this issue Jul 28, 2020 · 30 comments
Open

RazorLight requires ASP.NET Core runtime #360

Tyrrrz opened this issue Jul 28, 2020 · 30 comments

Comments

@Tyrrrz
Copy link

Tyrrrz commented Jul 28, 2020

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

  • OS: Win10 x64
  • Platform .NET Core 3.1
  • RazorLight version 2.0-beta9
  • Are you using the OFFICIAL RazorLight package? yes
  • Rider 2020.1.3
@jzabroski
Copy link
Collaborator

@Tyrrrz How should it work instead?

@Tyrrrz
Copy link
Author

Tyrrrz commented Jul 28, 2020

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
Copy link
Collaborator

@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
Copy link
Author

Tyrrrz commented Jul 29, 2020

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

<FrameworkReference Include="Microsoft.AspNetCore.App" />
), 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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Author

Tyrrrz commented Jul 29, 2020

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
Copy link
Collaborator

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
Copy link
Author

Tyrrrz commented Jul 29, 2020

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
Copy link
Collaborator

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
Copy link
Author

Tyrrrz commented Jul 29, 2020

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
Copy link
Collaborator

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
Copy link
Owner

toddams commented Jul 30, 2020

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
Copy link
Collaborator

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

@jzabroski
Copy link
Collaborator

jzabroski commented Aug 5, 2020

@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
Copy link
Author

Tyrrrz commented Aug 5, 2020

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
Copy link
Collaborator

@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
Copy link

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
Copy link

schmitch commented Sep 7, 2020

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
Copy link
Collaborator

jzabroski commented Jan 6, 2021

@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
Copy link
Collaborator

jzabroski commented Jan 6, 2021

@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
Copy link
Author

Tyrrrz commented Jan 6, 2021

@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
Copy link
Collaborator

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: 76611d0

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
Copy link
Author

Tyrrrz commented Jan 11, 2021

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

@jzabroski jzabroski pinned this issue Jan 11, 2021
@knuxbbs
Copy link

knuxbbs commented Jan 12, 2021

@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
Copy link

knuxbbs commented Jan 12, 2021

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

@jzabroski
Copy link
Collaborator

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

OK, please add it :)

@Aledever4i Aledever4i mentioned this issue Jun 7, 2021
@tebeco
Copy link

tebeco commented Sep 14, 2021

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
Copy link

tebeco commented Sep 14, 2021

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
Copy link

tebeco commented Sep 14, 2021

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants