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

Added support for per-app runtime config files #227

Closed
wants to merge 6 commits into from

Conversation

BeneRasche
Copy link

Edge-js currently does not work correctly when only using a .NET runtime. It currently requires an installed .NET SDK.

That is not feasible for production environments as they typically use only the .NET runtime as a prerequisite.

The reason for the difference appears to be that edge-js tries to find a dotnet.runtimeconfig.json file which is only available when using the .NET SDK.

However a .NET project can generate its own per-app runtimeconfig file. This file should be used preferably if present in the EDGE_APP_ROOT directory. If there is more than one file, an exception is thrown. If there is no file, it should revert to the current mechanism where the SDK runtimeconfig.json is used.

@BeneRasche
Copy link
Author

See this issue for discussion

#226

agracio added a commit that referenced this pull request Oct 22, 2024
@agracio
Copy link
Owner

agracio commented Oct 22, 2024

Unfortunately this PR breaks most tests: https://github.com/agracio/edge-js/actions/runs/11468850643. I will investigate.

@agracio
Copy link
Owner

agracio commented Oct 22, 2024

test.dll is created with test.runtimeconfig.json with json content below, this causes majority of CoreCLR tests to fail:

{
  "runtimeOptions": {
    "tfm": "net6.0",
    "includedFrameworks": [
      {
        "name": "Microsoft.NETCore.App",
        "version": "6.0.33"
      },
      {
        "name": "Microsoft.AspNetCore.App",
        "version": "6.0.33"
      }
    ],
    "configProperties": {
      "System.GC.Server": true,
      "System.Reflection.Metadata.MetadataUpdater.IsSupported": false,
      "System.Runtime.Serialization.EnableUnsafeBinaryFormatterSerialization": false
    }
  }
}

@BeneRasche
Copy link
Author

Problem is runtime_config.cpp only tries to read "framework" but runtimeconfig.json uses "frameworks". Currently fixing it.

Note, starting with 3.0, the "framework" section is optional and a new "frameworks" section supports multiple frameworks

https://github.com/dotnet/sdk/blob/main/documentation/specs/runtime-configuration-file.md

@BeneRasche
Copy link
Author

For some reason you have a section called "includedFrameworks". This is not part of the official specs for runtimeconfig.json files. Where does this section come from?

For me generating a test.dll generates a section called "frameworks".

@BeneRasche
Copy link
Author

It turns out that "includedFrameworks" is a section that is generated instead of "frameworks" when publishing as a self-contained test.dll.

Super weird. I'll add it to my code..

https://stackoverflow.com/questions/69373434/why-does-runtimeconfig-json-contain-includedframeworks-instead-of-frameworks

@agracio
Copy link
Owner

agracio commented Oct 23, 2024

Btw you can always see results of your changes in your branch Actions: https://github.com/BeneRasche/edge-js-sdk-bug/actions/workflows/main.yml. However changes do not affect Windows runners since those use pre-compiled libs.

@BeneRasche
Copy link
Author

Everything should work now - but for some reason MACOS tests fail? I'm not sure why

@agracio
Copy link
Owner

agracio commented Oct 23, 2024

Tests are not failing - build is failing, check Setup Env step. But as I said previously changes are not applied to Windows runners. Same here: https://ci.appveyor.com/project/agracio/edge-js/history.
To test Windows build run manual Build Github workflow from your branch.

@BeneRasche
Copy link
Author

Is there anything I can still help with here?

@agracio
Copy link
Owner

agracio commented Oct 23, 2024

You could easily fix those C++ compile errors and also test Windows build as per my previous comment. I will take a look at it myself but cannot promise quick turnaround.

../src/CoreCLREmbedding/host/runtime_config.cpp:110:88: error: cannot pass object of non-trivial type 'pal::string_t' (aka 'basic_string<char, char_traits<char>, allocator<char>>') through variadic function; call will abort at runtime [-Wnon-pod-varargs]
    trace::verbose(_X("Found framework with version [%s] in runtimeconfig.json file"), m_fx_ver);

@BeneRasche
Copy link
Author

Oh I see it now, missing a .cstr()

Fixing it.

@agracio
Copy link
Owner

agracio commented Oct 23, 2024

And don't forget to run Build in your GitHub Actions to test Windows.

@agracio
Copy link
Owner

agracio commented Oct 23, 2024

No need to commit Windows binaries, once all tests are passing I would recompile them myself.

@agracio
Copy link
Owner

agracio commented Oct 23, 2024

macOS: CoreClrEmbedding::Initialize - Error initializing the dependency resolver: A fatal error was encountered, missing dependencies manifest at: /Users/runner/.dotnet/shared/Microsoft.NETCore.App/6.0.0/Microsoft.NETCore.App.deps.json.

I have a feeling that this resolver would not work locally either if you do not have a specific version on .NET Runtime. Will test everything myself.
Likely solution would be to walk though dotnet/shared/Microsoft.NETCore.App and other frameworks the same way it walks thought .NET SDK folders to find the most recent one instead of pointing to a specific version.

The reason it works on other runners it likely because they have .NET Runtime 6.0 installed.

@agracio
Copy link
Owner

agracio commented Oct 23, 2024

I will take a look at it later, no need to rush.

@agracio
Copy link
Owner

agracio commented Oct 23, 2024

Looks like it is not going to be a very easy fix, will be looking through code to figure out the best approach.

@agracio agracio linked an issue Oct 23, 2024 that may be closed by this pull request
@agracio
Copy link
Owner

agracio commented Oct 23, 2024

I think the easiest approach would be to use [appname].runtimeconfig.json only if .NET SDK is not found.

@BeneRasche
Copy link
Author

I will change the code accordingly

@agracio
Copy link
Owner

agracio commented Oct 24, 2024

Thanks for doing those changes, would get around it as well but tied up in other work at the moment. Will be much faster if you do it.

@agracio
Copy link
Owner

agracio commented Oct 24, 2024

I am working on it now.

@BeneRasche
Copy link
Author

It's no problem at all. I need these changes for our software which runs node.js via pkg and calls a .NET dll.

So basically I am paid to make these changes, anyway. If you need help or if I should make the changes, just tell me.

@BeneRasche
Copy link
Author

I made the neccessary changes, you can have a look if it's what you meant

@agracio
Copy link
Owner

agracio commented Oct 25, 2024

I started working on those changes yesterday myself just did not have a chance to commit and test as it was getting late. But if you already finished then it makes it simpler.

@agracio
Copy link
Owner

agracio commented Oct 25, 2024

Looks like there is an issue with either test runners or specific action for PRs, could you rerun your actions on your branch.
Did you test your changes on a PC without SDK to make sure it resolves the issue?

@agracio
Copy link
Owner

agracio commented Oct 25, 2024

I will merge your changes to my branch https://github.com/agracio/edge-js/tree/clr-runtime and will rerun everything including full binaries rebuild.

@BeneRasche
Copy link
Author

BeneRasche commented Oct 25, 2024

I tested with node 20 on windows VM: With only runtime and with SDK installed. It seemed to work for me, yes.

@BeneRasche
Copy link
Author

I am unsure how to trigger the rerun. The windows action went through without a problem. But the linked action in this PR cannot be retriggered by me? I suppose you can run the tests yourself with that clr-runtime branch now, anyway?

agracio added a commit that referenced this pull request Oct 25, 2024
@agracio
Copy link
Owner

agracio commented Oct 25, 2024

I meant rerunning GitHub Action on your branch since it was cancelled, but there is no need anymore. Merged your changes into my branch with additional improvements https://github.com/agracio/edge-js/actions/runs/11520535845.
master...clr-runtime

agracio added a commit that referenced this pull request Oct 25, 2024
@agracio
Copy link
Owner

agracio commented Oct 25, 2024

#229 merged, new Edge,js version 23.1.0 published to NPM. Test in your project.

@agracio
Copy link
Owner

agracio commented Oct 26, 2024

Some quality-of-life improvements: code base now includes C++ x64 .sln file to make it easier to work on future updates.

@BeneRasche BeneRasche closed this Oct 28, 2024
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

Successfully merging this pull request may close these issues.

2 participants