search

toddams / RazorLight

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

DOTNET 6 Error - Cannot find compilation library location for package 'System.Security.Cryptography.Pkcs' #460

Closed jacodv closed 2 years ago

jacodv commented 2 years ago

Describe the bug Existing tests fail after upgrade.

To Reproduce Upgrade Test Project from Dotnet 5 to Dotnet 6:

Expected behavior The template should transform

Information (please complete the following information):

Additional context TEST

    public void Convert_GivenValidInput_ShouldConvert()
    {
      // arrange
      Setup();
      var data = new { Name = "TheName" };
      var template = "Name is @Model.Name";

      // action
      var result = _engine.Convert(data, template, true);

      // assert
      result .Should().Be("Name is TheName");
  }

IMPLEMETATION:

        var result = engine.CompileRenderStringAsync(GetTemplateCachedId(templateData), templateData, data,  (ExpandoObject)null).Result;

ERROR:


IIAB.Core.Common.Exceptions.RazorParsingException : One or more errors occurred. (Cannot find compilation library location for package 'System.Security.Cryptography.Pkcs')
   at IIAB.Razor.RazorTemplateEngine.Convert(Object data, String templateData, Boolean ifErrorTrySerializeAndDeserialize) in C:\Git\IIAB-netcore\src\IIAB.Razor\RazorTemplateEngine.cs:line 102
   at IIAB.Razor.Tests.RazorTemplateEngineTests.Convert_GivenValidInput_ShouldConvert() in C:\Git\IIAB-netcore\src\IIAB.Razor.Tests\RazorTemplateEngineTests.cs:line 32
jacodv commented 2 years ago

I saw this https://github.com/NuGet/Home/issues/8543

jacodv commented 2 years ago

@jzabroski I hope you recover soon. The new normal, after COVID has some advantages like working form home. But so many lives were lost and so many people were badly effected, physically and economically.

W.r.t a use case: We create a batch of say 10k emails. All with a razor email body that is captured and stored in a database. Then when we get to the send stage of our process, we get a JSON document containing the recipient data for each recipient. We then create messages in a queue with the JSON as well as the email body razor.

The queue messages are processed using a pub/sub mechanism across many servers.

Implementation

jzabroski commented 2 years ago

Got it. So you still need RazorEngine and see that whole class as staying put. That should be fine, as that dependency is where NoRazorProjectItem goes, and handles the scenario where people do crazy things with DI without reading any documentation that annoy me and cause a lot of false positive bug reports.

jzabroski commented 2 years ago

Maybe call it RazorLight.NoProjectSystem or similar. Need to review the code to see exactly what the right name would be.

jacodv commented 2 years ago

Maybe RazorLight.NoProject?

jzabroski commented 2 years ago

I like that.

jacodv commented 2 years ago

@jzabroski , another good thing that came from the exercise is that we helped detect something that would have reached JetBrains' next release.

https://youtrack.jetbrains.com/issue/RSRP-487181

image

jzabroski commented 2 years ago

@jacodv Impressive work. You have a great eye for detail and following up.

leighmetzroth commented 2 years ago

Hey, just curious how the fix for this is going?

I have also run into this when trying to migrate a project to .net 6. If it would help, I would be happy to branch and do up the PR for it.

jzabroski commented 2 years ago

@jacodv Has a list of branches here where he worked through this stuff: https://github.com/jacodv/RazorLightDotNet6/branches

I personally dont have much time to devote to this project any more. Happy to push through any fixes, though.

leighmetzroth commented 2 years ago

@jzabroski Thanks for the quick reply.

I have raised a PR based on the changes from the latest code by @jacodv in https://github.com/jacodv/RazorLightDotNet6/tree/branch_jdevil-Dotnet6-WebApi6

I have excluded some of the extraneous code though.

leighmetzroth commented 2 years ago

@jzabroski I noticed you pushed up the update to the nuget version, but won't the new version be 2.0.0-rc.5? https://github.com/toddams/RazorLight/commit/7585ff2ef60460dd3968f2b7e15ace16d763216c

jzabroski commented 2 years ago

Yes, I was fixing the readme to reflect the current latest. I forgot to update it. I might reach out to SImon Cropp to see if he has a GitHub Action for that as part of pushing new versions.

I plan to push to nuget soon enough. I wanted to see if it was possible to have a RazorLight.net48.csproj for old customers and then we can drop the -rc. If you want to tackle that yourself, that would expedite things.

leighmetzroth commented 2 years ago

If it's not done by Monday Brisbane time then I'll jump onto it. If you can give me a rough guide on how you expect/envisaged it to be implemented then that will help?

I'm guessing some of the implementation will need to be abstracted out into a common project?

It's the weekend here and we have 10 month old twins, so I can't really spare out of work time to do it sorry. My boss has given me the okay to work on this during business hours so it meets our requirements of .NET 6 compatibility and if we can take it further and say we're not using pre-release packages then that would be all the better.

leighmetzroth commented 2 years ago

@jzabroski I have done a little investigating this morning and I'm a little confused because it seems that it already supports .net Framework 4.8 as it already targets .net Standard 2.0

What I did: I added a new console app RazorLight.Sandbox.net48 project and then made it reference the RazorLight project. I then included the below code as the main function. Running this it successfully outputs "This is a test Bar" to the console.

Have I missed something about what you are needing?

        public static async Task Main(string[] args)
        {
            var engine = new RazorLightEngineBuilder()
                .UseMemoryCachingProvider()
                .Build();

            var renderedString = await engine.CompileRenderStringAsync(
                "templateKey",
                "This is a test @Model.Foo",
                new
                {
                    Foo = "Bar",
                });

            Console.WriteLine(renderedString);
            Console.ReadKey();
        }
jzabroski commented 2 years ago

The problem I ran into in the past was shipping a netstandard2.0 library rather than a net48 library that references a netstandard2.0 implementation.

RazorLight.Net48.csproj would support the people stuck on .NET Framework 4.8. It is effectively a copy paste of the RazorLight.csproj but using netstandard2.0 libraries. So the package name is a moniker to guide people to the right package to use for legacy Framework

jzabroski commented 2 years ago

I see. I guess my idea only goes so far. Adding a sample is probably best. I guess I should just go ahead and publish.

jzabroski commented 2 years ago

@leighmetzroth I remember my thought process now.

  1. We are targeting the following TFMs: https://github.com/toddams/RazorLight/blob/master/src/RazorLight/RazorLight.csproj#L3 a. netstandard2.0;netcoreapp3.1;net5.0;net6.0

  2. Of those TFMs, netstandard2.0 isn't a real target, but a pseudo-target. Because it's a pseudo-target, it can refer to netcoreapp3.1, net5.0, net6.0 or net48. a. netstandard2.1 started with .netcoreapp3.0. Therefore, in some sense, while netstandard2.0 can refer to .netcoreapp3.0 and greater, the PackageReference elements target AspNetCore 2.1 packages, which are out-of-date and not security patched. Fundamentally, net48 consumers can't realistically use netstandard2.1 frameworks (read: netcoreapp3.0 or later), because there are new features in the runtime like Span. b. Therefore, if you're using netstandard2.0, you are implicitly saying you plan to use it from a net48 project entrypoint - but some people want to use netstandard2.0 to simply mean a common library that can be used in various TFMs across their organization. This is what gets people into trouble and where they end up opening support requests on RazorLight Issues on GitHub (here), because they don't understand why they are getting runtime assembly conflict issues. c. Moreover, and this is really subtle, but what netstandard2.0 binds to when you compile depends upon what SDKs you have installed + if you override the SDK version number in your csproj via the rather unknown expression: dotnet new globaljson --sdk-version <yoursdkversion>, or use one of the options listed in How to: Reference an MSBuild Project SDK: 

    <Project Sdk="My.Custom.Sdk/1.2.3">
...
</Project>

    or

    <Project>
<Sdk Name="My.Custom.Sdk" Version="1.2.3" />
...
</Project>

There. Sorry I didn't get all this out on the weekend. Sometimes I turn my brain down a bit on the weekend.

leighmetzroth commented 2 years ago

Given this is blocking a ticket I'm working on that includes an upgrade to .net 6, are we able to publish the new pre-release nuget with .net 6 support so I can get some momentum back up on that work? As you may understand, it's a significant piece of work and each change that's made by my teammates or other tickets I'm working on needs to be merged into this branch; which in itself is costing me a lot of time.

Once my blocked ticket is progressing, I will be able to commit some time to making the change you're suggesting.

Also, is there a way that we could potentially talk through this and the requirements before I tackle it? Going by your profile, you're based in Boston when means that we could jump on a Google Meet early my time, which should be early evening your time.

jacodv commented 2 years ago

If I can assist, then I am willing to help. I will create a VM with all the required frameworks and help.

I am in South Africa and therefor in the middel, let me know if I can be of assistance.

jzabroski commented 2 years ago

Given this is blocking a ticket I'm working on that includes an upgrade to .net 6, are we able to publish the new pre-release nuget with .net 6 support so I can get some momentum back up on that work?

of course. https://github.com/toddams/RazorLight/actions/runs/1848527620 heading away from my desk for next 2 hours but i will follow up to see that it made it up to nuget.org I picked rc.5 for now - once you confirm it works fine, i will drop the rc tag

leighmetzroth commented 2 years ago

Looks like you've been fighting with failures 😔 and the latest failure is because it couldn't find the project.

I've never really dealt with github actions before, but looking at the code here for alirezanet/publish-nuget, I can see that it just references this.projectFile = process.env.INPUT_PROJECT_FILE_PATH whereas the others reference both the environment variable with and without the INPUT_ part. I also spotted this issue where there was a bug where the INPUT_ was being ignored.

Perhaps we need to switch PROJECT_FILE_PATH: src\RazorLight\RazorLight.csproj for INPUT_PROJECT_FILE_PATH: src\RazorLight\RazorLight.csproj temporarily?

jzabroski commented 2 years ago

It looks like it still failed. What's odd is I thought I fixed this 2 months ago. I do remember commenting on this PR https://github.com/brandedoutcast/publish-nuget/pull/62 but I dont remember how I pushed rc.4 - e.g., I think its possible I uploaded everything successfully but the GItHub action was reporting failure

jzabroski commented 2 years ago

I think the remaining issue is that the way the code is retrieving the environment var is based on the current process's env variable, which, at the time the step executes, probably is a stale copy of the env variable. I'm not an expert in GitHub actions, so I am going to try that - mostly explaining what I am doing here to leave breadcrumbs in the future.

jzabroski commented 2 years ago

So, there were several issues.

The latest seems to be that pack is failing for some reason... but the GitHub Action isnt checking that commands results for errors (yay!). Ideally these would be smaller actions, but what do I know.

The other issue I observe, that we havent even gotten to yet, is the Action is using --skip-duplicate, so there is no way to know if the workflow silently failed. Awesome.

leighmetzroth commented 2 years ago

It was looking so good! Looks like it tried to push as rc-4 again

leighmetzroth commented 2 years ago

Does it need the version updated in the Directory.Build.props?

jzabroski commented 2 years ago

https://github.com/toddams/RazorLight/actions/runs/1855287487

it succeeded but did it publish rc.5? I can't check right now. Driving

jzabroski commented 2 years ago

OK, I see the problem. I forgot I put the version in the src/Directory.Build.props not the top level build props. Should be fixed. It will be published as rc.6 unfortunately

jzabroski commented 2 years ago

@leighmetzroth @jacodv We did it! https://www.nuget.org/packages/RazorLight/2.0.0-rc.6

leighmetzroth commented 2 years ago

It is strange that it's not showing there, but it does show in the Frameworks section below. Maybe there's some process that updates it later???

Anyway, the good news is that the new version works in a test .NET 6 project. The bad news is that it looks like I'm about to be in "DLL Hell" as it was called earlier in this thread as it appears that something is pulling in the old version of System.Security.Cryptography.Pkcs in our solution. I'm fairly sure I know what it is (an internal NuGet that we have that's still referencing .NET 2.2).

image

jzabroski commented 2 years ago

The Frameworks tab I think is brand new. I am guessing its a UI bug. I actually just sent Jon Douglass a tweet about this: https://twitter.com/johnzabroski/status/1494499821469810697 He is the PM for Nuget.

jacodv commented 2 years ago

Fantastic!

I am resuming our upgrade next week!

On Fri, 18 Feb 2022 at 04:35, John Zabroski @.***> wrote:

The Frameworks tab I think is brand new. I am guessing its a UI bug. I actually just sent Jon Douglass a tweet about this: https://twitter.com/johnzabroski/status/1494499821469810697 https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Ftwitter.com%2Fjohnzabroski%2Fstatus%2F1494499821469810697&data=04%7C01%7C%7C31a8f371f5044c483cfd08d9f287587c%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C637807485421447557%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=Mv576ISR3fho34TPxehfpophmazbAjTGmshqqi7boMM%3D&reserved=0 He is the PM for Nuget.

— Reply to this email directly, view it on GitHub https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Ftoddams%2FRazorLight%2Fissues%2F460%23issuecomment-1043762327&data=04%7C01%7C%7C31a8f371f5044c483cfd08d9f287587c%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C637807485421447557%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=vubNEByDnOJrFKUu1Lsan0VpSwL%2FnzRx5UJW7DqalxM%3D&reserved=0, or unsubscribe https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FABMLO347EHY2KM4DRAF2WV3U3WV7PANCNFSM5IBGGZTQ&data=04%7C01%7C%7C31a8f371f5044c483cfd08d9f287587c%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C637807485421447557%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=lv7VaG92owzZDx%2BJw4vLpMhD48wJxD6zDhVuXUK3gkY%3D&reserved=0 . Triage notifications on the go with GitHub Mobile for iOS https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fapps.apple.com%2Fapp%2Fapple-store%2Fid1477376905%3Fct%3Dnotification-email%26mt%3D8%26pt%3D524675&data=04%7C01%7C%7C31a8f371f5044c483cfd08d9f287587c%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C637807485421447557%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=9qdTU6C2v2upJfArTaTiO%2BQdNAqJxx4fz4TOsS43J0A%3D&reserved=0 or Android https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fplay.google.com%2Fstore%2Fapps%2Fdetails%3Fid%3Dcom.github.android%26referrer%3Dutm_campaign%253Dnotification-email%2526utm_medium%253Demail%2526utm_source%253Dgithub&data=04%7C01%7C%7C31a8f371f5044c483cfd08d9f287587c%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C637807485421447557%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=PiqZsoqhMKql89UzxBfmmAJUGY8hAlJalNoOwFaPKhg%3D&reserved=0.

You are receiving this because you were mentioned.Message ID: @.***>

jzabroski commented 2 years ago

@jacodv @leighmetzroth Please do follow up on success. If you have success, I will drop the rc.6 tag and make it official. It's possible but not certain I am migrating to .NET 6 next month as well, so your efforts are appreciated.

leighmetzroth commented 2 years ago

After a heap of digging, I found that we have a transitive dependency on System.Security.Cryptography.Pkcs via MailKit (it has a dependency on MimeKit, which takes the dependency on System.Security.Cryptography.Pkcs).

After upgrading to the latest MailKit, I still had issues and ended up trawling through all of our own internal nugets (of which quite a few were in dire need of upgrading to the latest frameworks too 😬 ). After all of that, it still didn't work; uninstalling MailKit and commenting out the code that sends the emails using it, it did work....

I did some more Google-foo and eventually landed on this, which has an answer for adding <GenerateRuntimeConfigDevFile>true</GenerateRuntimeConfigDevFile> to the csproj files (the same stack of projects that need the PreserveCompilationContext set to true). I tried that, and it worked. So I have a feeling it's got something to do with the refactor they made to these DLLs and with the removal of the runtime config dev file, it has left projects like this unable to discover these dependencies for some reason.

Is it worth adding some instructions to the FAQ for this? If you think it is, just let me know and I'll write up the instructions and PR it in.

jzabroski commented 2 years ago

Yes, but... Please mention the actual issue rather than StackOverflow: https://github.com/dotnet/sdk/issues/16818

Make sure you edit README.Source.md and build. Check in both files with your PR

jacodv commented 2 years ago

WOW, I added that element to the project file weeks ago, but it was to to get ReSharper tests to work. Who would have thought....

jzabroski commented 2 years ago

@jacodv It's funny, isn't it. We were told that getting rid of Assembly Binding Redirects and shadow copy assemblies would make dotnet core infinitely easier than .NET Framework. And here we are, playing with a bunch of new knobs. Feels an awful lot like the old new thing, doesn't it? I do think its slightly improved in some core ways, but everything outside that core shell is now a mess. This has been my biggest takeaway working with .NET Core and later - dynamically loading plugins without a well-defined assembly load context is a bad idea. But the problem is the compiler framework doesn't have any way to hint at what dynamic load targets you might plan, e.g. if you target netstandard2.0.

@leighmetzroth Let me know if you still plan to do the P.R.

leighmetzroth commented 2 years ago

@jzabroski I am planning on doing it, just a bit busy right now between sick kids at home and having to take time off, and getting this .NET 6 upgrade completed and tested so we can roll it out to our customers.

jzabroski commented 2 years ago

Sounds good.

jacodv commented 2 years ago

@leighmetzroth @jzabroski - We have successfully upgraded to .Net 6. Thanks for all the help.

jzabroski commented 2 years ago

Yes, thanks. Were there any additional gotchas?

jacodv commented 2 years ago

Nope, Added the required project attributes [PreserveCompilationContext, GenerateRuntimeConfigDevFile] and all good.

mikart143 commented 2 years ago

So for people still having the issue the ultimate solution for this is setting PreserveCompilationContext to true and adding ExcludingAssembly like:

            services.AddRazorLight(() =>
            {
                return new RazorLightEngineBuilder()
                    .UseFileSystemProject(Path.GetDirectoryName(Assembly.GetExecutingAssembly().Location))
                    .UseMemoryCachingProvider()
                    .ExcludeAssemblies(typeof(System.Security.Cryptography.Pkcs.AlgorithmIdentifier).Assembly.GetName().Name) // Workaround for loading CompileLibraries
                    .EnableDebugMode(Debugger.IsAttached)
                    .Build();
            });

If you will try to Exclude Assembly with FullName It will not work. Additonally the RazorLightDependancyEngineBuilder is broken and all ExcludedAssemblies are ignored further in DefaultMetadataReferenceManager. @jzabroski

jzabroski commented 2 years ago

Additonally the RazorLightDependancyEngineBuilder is broken and all ExcludedAssemblies are ignored further in DefaultMetadataReferenceManager. @jzabroski

Can you explain what you mean by opening a new issue?