Lightup Deps.json feature by gkhanna79 · Pull Request #1727 · dotnet/core-setup

gkhanna79 · 2017-03-13T23:19:21Z

Ported from PR #1630 and fixes https://github.com/dotnet/core-setup/issues/1597

gkhanna79 · 2017-03-14T23:03:27Z

pakrym · 2017-03-14T23:30:43Z

How does this work with Microsoft.Extensions.DependencyModel .deps file loading? It only supports discovering two deps files: application and frameworks (see https://github.com/dotnet/core-setup/blob/master/src/Microsoft.Extensions.DependencyModel/DependencyContextPaths.cs)

gkhanna79 · 2017-03-14T23:42:05Z

It does not @pakrym. This is intended to be a scoped change only to support Azure lightup scenarios.

pakrym · 2017-03-14T23:45:48Z

There may be unexpected consequences, for example if Azure Lightup brings controller and MVC uses runtime assembly scan to discover controllers it would not get discovered. Just wanted to make sure this is known.

gkhanna79 · 2017-03-14T23:49:58Z

I think any assembly injected using this mechanism will not be discoverable via DependencyContext, from my naive understanding of it.

Either ways, I believe you own DependencyContext, so you may want to followup on this.

Sign in to view

+    if (additional_deps_serialized.empty())
+    {
+        // additional_deps_serialized stays empty if DOTNET_ADDITIONAL_DEPS env var is not defined
+        pal::getenv(_X("DOTNET_ADDITIONAL_DEPS"), &additional_deps_serialized);


Sign in to view

+                new deps_json_t(true, json_file, m_fx_deps->get_rid_fallback_graph())));
+        }
+    }
+    else


Sign in to view

+    pal::string_t additional_deps_path;
+    pal::stringstream_t ss(additional_deps_serialized);
+
+    // Process the delimiter seperated custom deps files.


Sign in to view

+            }
+            else
+            {
+                Console.WriteLine("Lightup assembly successfully loaded!");


Sign in to view

+          "version": "1.1.0",
+          "exclude": "all"
+        },
+        "Microsoft.CSharp": {


Sign in to view

+  "frameworks": {
+    "netcoreapp2.0": {
+      "dependencies": {
+        "Microsoft.NETCore.App": {


Sign in to view

@@ -0,0 +1,386 @@
+{


analogrelay · 2017-03-15T19:01:09Z

Could we have a flag in runtimeconfig.json to allow me, as an app developer, to turn this feature off completely, even if the environment variables are set?

I feel like some app developers may want to be a little paranoid and disable this loading completely, to avoid having to worry about it at all. It may be a little overkill (and it's certainly not perfect), but it feels a little cleaner to have a complete opt-out that remains in the control of the app developer.

In ASP.NET, our "plugin loading" system which will consume this additional deps file also has this "opt-out" functionality, but there could be cases where even loading the new deps file could be problematic to app developers, so having a global opt-out just seems like a Good Thing here ;).

Ideally, this runtimeconfig.json flag would be propagated from an MSBuild Property too ;)

(/cc @DamianEdwards @glennc)

gkhanna79 · 2017-03-15T19:11:12Z

Having a flag to override a feature that is opt-in by design sounds extremely odd to me. You should not set it for the app in question.

DamianEdwards · 2017-03-15T19:17:35Z

It's not opt-in for the app, it's opt-in for the server/hoster.

…

On Mar 15, 2017 12:11 PM, "Gaurav Khanna" ***@***.***> wrote: Having a flag to override a feature that is opt-in by design sounds extremely odd to me. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1727 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAPNADwbGo61VvwXYb-8Ggkf3qdKaqY6ks5rmDfSgaJpZM4Mb8C8> .

analogrelay · 2017-03-15T19:21:14Z

It's not opt-in for the app, it's opt-in for the server/hoster.

Exactly. If I as an App Developer don't want hosters messing with my loaded app, I think it should be possible to disable this feature.

gkhanna79 · 2017-03-15T19:23:31Z

But then why would they need to unconditionally apply to all apps? Why can't the app tell the host not do any "injection of plugins" so that the host only specifies the plugin deps for the apps that have not opted out.

I do not see why the host has to participate in enabling override for something that is optional by design. IMHO, the server/hoster should do the right thing in determining to whom they can apply the flag.

CC @richlander

analogrelay · 2017-03-15T19:25:50Z

It definitely shouldn't apply to all apps, I'm talking about a way for an individual app to set a setting in their own runtimeconfig.json file that tells the host not to load the additional deps files.

gkhanna79 · 2017-03-15T19:29:46Z

It definitely shouldn't apply to all apps, I'm talking about a way for an individual app to set a setting in their own runtimeconfig.json file that tells the host not to load the additional deps files.

My understanding is that an application does not ever know that such an injection may happen - such an injection is magic. This was one of the reasons why the idea to include plugin dependencies in the app.deps.json, at the time of the app publish, was turned down since the app would then need to know about this.

What you are proposing above translates to app knowing about this even though we are claiming it is magical injection. I would assert that this is contradictory - either you make it explicit for the app to bring in (or not) the dependencies or handle things for them. The opt-in nature of the feature enables the server to exclude the injection from happening for apps that may choose to request.

eerhardt · 2017-03-15T23:01:11Z

IMHO, the server/hoster should do the right thing in determining to whom they can apply the flag.

Agreed. If the web host wants to allow individual apps from opting out of this behavior, the host should have the flag to allow individual apps from opting out. Then the host doesn't set the Environment Variable, and it doesn't pass in --additional-deps. That's how the opt-out should work. If someone set the variable, then this behavior kicks in. Don't set the variable if you don't want this behavior.

Sign in to view

+    // application, then add them to the mix as well.
+    for (const auto& additional_deps : m_additional_deps)
+    {
+        auto additional_deps_entries = additional_deps->get_entries(deps_entry_t::asset_types::runtime);


Sign in to view

+        else
+        {
+            // We'll search deps files in 'base_dir'/shared/fx_name/fx_ver
+            append_path(&additional_deps_path, _X("shared"));


Sign in to view

+        {
+            // We'll search deps files in 'base_dir'/shared/fx_name/fx_ver
+            append_path(&additional_deps_path, _X("shared"));
+            append_path(&additional_deps_path, fx_name.c_str());


Sign in to view

+        for (pal::string_t json_file : m_additional_deps_files)
+        {
+            m_additional_deps.push_back(std::unique_ptr<deps_json_t>(
+                new deps_json_t(true, json_file, m_fx_deps->get_rid_fallback_graph())));


Sign in to view

+            // We'll search deps files in 'base_dir'/shared/fx_name/fx_ver
+            append_path(&additional_deps_path, _X("shared"));
+            append_path(&additional_deps_path, fx_name.c_str());
+            append_path(&additional_deps_path, fx_ver.c_str());


gkhanna79 · 2017-03-21T16:50:20Z

@eerhardt PR is updated to only support the feature for portable apps.

Sign in to view

+         * graph in the build output.
+         * https://github.com/dotnet/cli/issues/2343
+         */
+        private static void ReplaceTestProjectOutputHostInTestProjectFixture(TestProjectFixture testProjectFixture)


Sign in to view

+          "version": "1.1.0",
+          "exclude": "all"
+        },
+        "Microsoft.CSharp": {


eerhardt

Just a few minor test comments.

Sign in to view

@@ -0,0 +1,17 @@
+{


dnfclas added the cla-already-signed label Mar 13, 2017

gkhanna79 force-pushed the custom_Deps branch 2 times, most recently from 4a5bbb9 to 36c58c0 Compare March 14, 2017 19:01

gkhanna79 changed the title ~~[WIP Lightup Deps.json feature~~ [WIP] Lightup Deps.json feature Mar 14, 2017

gkhanna79 force-pushed the custom_Deps branch from 5505505 to 70cd2a6 Compare March 14, 2017 19:44

gkhanna79 changed the title ~~[WIP] Lightup Deps.json feature~~ Lightup Deps.json feature Mar 14, 2017

eerhardt reviewed Mar 15, 2017

View reviewed changes

Comment thread src/corehost/cli/deps_resolver.cpp Outdated

new deps_json_t(true, json_file, m_fx_deps->get_rid_fallback_graph())));

}

}

else

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

eerhardt reviewed Mar 15, 2017

View reviewed changes

Comment thread TestAssets/TestProjects/LightupClient/Program.cs Outdated

}

else

{

Console.WriteLine("Lightup assembly successfully loaded!");

This comment was marked as spam.

Sign in to view

eerhardt reviewed Mar 15, 2017

View reviewed changes

Comment thread TestAssets/TestProjects/LightupLib/project.json

"version": "1.1.0",

"exclude": "all"

},

"Microsoft.CSharp": {

This comment was marked as spam.

Sign in to view

eerhardt reviewed Mar 15, 2017

View reviewed changes

Comment thread TestAssets/TestProjects/LightupLib/project.json

@@ -0,0 +1,386 @@

{

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

gkhanna79 force-pushed the custom_Deps branch from f6702fc to 9718c29 Compare March 15, 2017 19:08

eerhardt reviewed Mar 16, 2017

View reviewed changes

Comment thread src/corehost/cli/deps_resolver.cpp

else

{

// We'll search deps files in 'base_dir'/shared/fx_name/fx_ver

append_path(&additional_deps_path, _X("shared"));

This comment was marked as spam.

Sign in to view

eerhardt reviewed Mar 16, 2017

View reviewed changes

gkhanna79 force-pushed the custom_Deps branch 2 times, most recently from f7c07fb to fa996c3 Compare March 17, 2017 03:39

eerhardt reviewed Mar 17, 2017

View reviewed changes

Custom deps feature implemented.

1d3afa5

gkhanna79 force-pushed the custom_Deps branch from 37f1cda to 1d3afa5 Compare March 21, 2017 00:06

Remove standalone test

a865ff4

gkhanna79 added the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Mar 21, 2017

eerhardt reviewed Mar 21, 2017

View reviewed changes

Comment thread TestAssets/TestProjects/LightupLib/project.json

"version": "1.1.0",

"exclude": "all"

},

"Microsoft.CSharp": {

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

eerhardt approved these changes Mar 21, 2017

View reviewed changes

Cleanup unrequired function

8b5afec

gkhanna79 merged commit 7b2eafe into dotnet:master Mar 21, 2017

AustinWise mentioned this pull request Mar 22, 2017

Fix the reading of host_interface_t to be backwards compatible. #1818

Merged

davidfowl reviewed Mar 24, 2017

View reviewed changes

Comment thread TestAssets/TestProjects/LightupClient/project.json

@@ -0,0 +1,17 @@

{

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

Conversation

gkhanna79 commented Mar 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gkhanna79 commented Mar 14, 2017

Uh oh!

pakrym commented Mar 14, 2017

Uh oh!

gkhanna79 commented Mar 14, 2017

Uh oh!

pakrym commented Mar 14, 2017

Uh oh!

gkhanna79 commented Mar 14, 2017

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

analogrelay commented Mar 15, 2017

Uh oh!

gkhanna79 commented Mar 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DamianEdwards commented Mar 15, 2017 via email

Uh oh!

analogrelay commented Mar 15, 2017

Uh oh!

gkhanna79 commented Mar 15, 2017

Uh oh!

analogrelay commented Mar 15, 2017

Uh oh!

gkhanna79 commented Mar 15, 2017

Uh oh!

eerhardt commented Mar 15, 2017

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

gkhanna79 commented Mar 13, 2017 •

edited

Loading

gkhanna79 commented Mar 15, 2017 •

edited

Loading