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

Runtime binding proposal #51

Merged
merged 12 commits into from
Feb 7, 2019
Merged

Runtime binding proposal #51

merged 12 commits into from
Feb 7, 2019

Conversation

richlander
Copy link
Member

Proposal for .NET Core 3.0

Related content:

* `minor` -- Roll forward to the lowest higher minor verison, if requested minor version is missing.
* `major` -- Roll forward to lowest higher major version, and lowest minor version, if requested major version is missing
* `UseLatestMinor` -- Roll forward to latest minor version, even if requested minor version is on machine. Intended for COM hosting scenario.
* `UseLatestMajor` -- Roll forward to latest major and highest minor version, even if requested major is on machine. Intended for COM hosting scenario.
Copy link
Contributor

@nguerrera nguerrera Feb 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit 1: I think the values should all have the same canonical casing, so Patch, Minor, Major

Nit 2: I don't this Use is adding much value, how about LatestMinor, LatestMajor?

Copy link
Member Author

@richlander richlander Feb 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good feedback. Let me share some of my thinking and see what you say. I original went with exactly what you have (ignoring the casing topic) Minor and LatestMinor. The problem is that the two settings are not symmetrical:

  • Minor -- Do something only as a fallback case if a preferred runtime version is not found.
  • LatestMinor -- Do something always.

That's why I added Use to LatestMinor to make it more obvious that this setting is more of a command than a request/hint. I also thought of ForceLatestMinor but that seemed a bit too forceful.

I also thought of splitting these two forms of configuration into two different settings. That seemed like the worst idea, both because it would be confusing and there is no scenario where both settings would be set.

Thoughts?

Will fix the casing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe:
AlwaysLatestMinor
AlwaysLatestMajor

Copy link
Contributor

@nguerrera nguerrera Feb 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe more explicit names for all cases?

These are long, but for completeness in the names as an illustration:

LatestPatch
FallbackToNearestMinorWithLatestPatch
FallbackToNearestMajorWithLatestPatch
LatestMinorWithLatestPatch
LatestMajorWithLatestPatch

@nguerrera
Copy link
Contributor

nguerrera commented Feb 2, 2019

LGTM. I really like the single option, and the elimination of magic [0-2] numeric values. I also never liked the "onNoCandidateFx" name.

* `.runtimeconfig.json` properties (AKA "json")
* CLI arguments
* Environment variables (AKA "ENV")

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Earlier a project (.csproj/.vbproj) setting was proposed. It's not mentioned here in precedence. Is that because it rolls into the runtimeconfig? Cool, but we should mention it I think.

Copy link
Member Author

@richlander richlander Feb 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that because it rolls into the runtimeconfig?

Yes. Project values have no meaning at runtime since (in most cases) it no longer exists as an accessible artifact. I will add a note for that.

* `UseLatestMinor` -- Roll forward to latest minor version, even if requested minor version is on machine. Intended for COM hosting scenario.
* `UseLatestMajor` -- Roll forward to latest major and highest minor version, even if requested major is on machine. Intended for COM hosting scenario.

There is no default setting. See **Configuration Precedence** for more information.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well that's not exactly true. If nothing is specified anywhere, the default behavior is Minor - it basically comes from the default rollForwardOnNoCandidateFx. I think this should be stated explicitly.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is what I see at runtime:

C:\testapps\twooneapp>dotnet bin\Debug\netcoreapp2.1\twooneapp.dll
Hello World!
C:\Program Files\dotnet\shared\Microsoft.NETCore.App\2.1.7\System.Private.CoreLib.dll

C:\testapps\twooneapp>dotnet --fx-version 2.1.0 bin\Debug\netcoreapp2.1\twooneapp.dll
Hello World!
C:\Program Files\dotnet\shared\Microsoft.NETCore.App\2.1.0\System.Private.CoreLib.dll

for this code:

static void Main(string[] args)
{
    Console.WriteLine("Hello World!");
    Console.WriteLine(typeof(Object).Assembly.Location);
}

This is the basis of the logic I wrote.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that is expected. By default (1st case) the app rolls forward to the latest patch only. Note that "Minor" in the new setting is the same 1 in rollForwardOnNoCandidateFx, which only kicks in if the requested Major.Minor doesn't exist. In this case it does exist (the machine apparently has at least 2.1.0 and 2.1.7). So the roll forward logic doesn't really kick in.
Rolling forward on patches is a separate piece of functionality, and that always rolls forward to latest (unless completely disabled).

The second case uses the --fx-version which currently means exactly that version and no other. Absolutely no roll forward will happen (Major/Minor or Patch). So since you requested 2.1.0, you got 2.1.0.

* If set at the same scope (json, CLI or ENV), then `version` establishes a floor for any roll-forward behavior.
* The `roll-forward` value does not affect higher precedent scopes. For example, a `version` setting specified at the CLI scope overwrites both a `version` and `roll-forward` setting that might exist at the json scope. Same thing with ENV configuation w/rt CLI arugments and json.
* A `version` setting can flow to higher scopes if it is not replaced by another `version` value. This enables a `roll-forward` setting at a higher scope to compose with a `version` setting at a lower scope.
* The default `roll-forward` for json scope is `minor`. There is no default roll-forward value for CLI and ENV scopes.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There needs to be another paragraph describing interaction with the existing rollForwardOnNoCadidateFx (even though we're deprecating it, we can't remove it). The interaction should be pretty simple I think - basically if rollForward is specified, then rollForwardOnNoCandidateFx is ignored. I would not try to combine the two in any way as it may be very confusing. And failing if both are set also feels too "dangerous".


We will introduce a new knob, called `RollForward`, with the following values:

* `None` -- Do not roll forward. Only bind to specified version.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this have a runtimeconfig equivalent today?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking applypatches. On that note, is the third example wrong on that page?

@vitek-karas was advocating for not adding None to RollForward. I find it hard to reason about --fx-version behavior in absence of a None option.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess None would mean rollForwardOnNoCandidateFx=0 and applyPatches=0 in current runtimeconfig.
I'm not sure we should "advocate" it by including it in the setting here. Basically we don't want anybody to use that setting unless there's no other way as it disables security patching.

I think that if we decide to have an option for this behavior in RollForward it should not be called None - mostly because people tend to think about None as the safe default - which this is not (neither default nor safe).
If we include it I would call it DisableAll or something like that - to make it more explicit and more "scary".

* `AlwaysLatestMinor` -- Roll forward to latest minor version, even if requested minor version is present. Intended for COM hosting scenario.
* `AlwaysLatestMajor` -- Roll forward to latest major and highest minor version, even if requested major is present. Intended for COM hosting scenario.

`Minor` is the default setting. See **Configuration Precedence** for more information.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Last time we spoke in a big group about this topic, there was a lot of openness to Major becoming the default. Is that now off the table? cc @DamianEdwards

I think that this proposal makes configuration easier and the whole process easier to understand, I was really hoping that global tools would start to just workin mismatched major case. cc @wli3

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was more openness to it at the time. I'm not proposing that, however, as the way to move forward. When we flip that lever, the journey to re-inventing the .NET Framework will be complete. Yeahh!

Another way of putting is asking which UX experience we want (since both will exist to some degree):

  • Why didn't think app start? Oh, I have to install something from Microsoft. Ain't got time for that. Screw this app!
  • Why did this app crash? I don't know. Screw this app!

I'm pushing choice 1 as the good default. The second one is too hard for everyone involved.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's also the argument about hosting environments (think ASP.NET hosting). That is running my app on a container/VM which is managed by somebody else. If in such case I try to run a 2.* app on a host which only has 3.0 and it silently works... everybody thinks (including the hosting provider), that it will work just fine. In reality there might be some slight behavioral difference which can't be tested for upfront before deployment. It makes it harder for both sides to have a solid experience.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. Next question: do you consider it OK if we recommend to global tool authors to put RollForward=Major in their csproj. We have a lot of feedback that folks don't like that their global tools do not work with only 3.0 installed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK if we recommend to global tool authors to put RollForward=Major

It is ok as long as the global tool authors understand the consequences. This setting increases probability that the tool won't work.

I think it is manageable for small tools that are unlikely to break. I would not recommend it for large complex tools.

Copy link
Member

@vitek-karas vitek-karas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.
There's one large missing peice - handling of previews. Basically define rules about rolling forward (and versions) from/to previews, in case non-previews are missing and so on.


We will introduce a new knob, called `RollForward`, with the following values:

* `None` -- Do not roll forward. Only bind to specified version.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess None would mean rollForwardOnNoCandidateFx=0 and applyPatches=0 in current runtimeconfig.
I'm not sure we should "advocate" it by including it in the setting here. Basically we don't want anybody to use that setting unless there's no other way as it disables security patching.

I think that if we decide to have an option for this behavior in RollForward it should not be called None - mostly because people tend to think about None as the safe default - which this is not (neither default nor safe).
If we include it I would call it DisableAll or something like that - to make it more explicit and more "scary".

* `AlwaysLatestMinor` -- Roll forward to latest minor version, even if requested minor version is present. Intended for COM hosting scenario.
* `AlwaysLatestMajor` -- Roll forward to latest major and highest minor version, even if requested major is present. Intended for COM hosting scenario.

`Minor` is the default setting. See **Configuration Precedence** for more information.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's also the argument about hosting environments (think ASP.NET hosting). That is running my app on a container/VM which is managed by somebody else. If in such case I try to run a 2.* app on a host which only has 3.0 and it silently works... everybody thinks (including the hosting provider), that it will work just fine. In reality there might be some slight behavioral difference which can't be tested for upfront before deployment. It makes it harder for both sides to have a solid experience.

The specified framework 'Microsoft.NETCore.App', version '2.2.0' was not found.
C:\testapps\twooneapp>dotnet --fx-version 2.2.0 --roll-forward Patch bin\Debug\netcoreapp2.1\twooneapp.dll
Hello World!
C:\Program Files\dotnet\shared\Microsoft.NETCore.App\2.2.3\System.Private.CoreLib.dll
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the overall outcome of this. The only confusion might arise when all the different ways to specify these settings is combined. If specified on command line together like this, it makes perfect sense. If the RollFoward is specified through an environment variable which is effectively invisible, it gets a bit more complicated (as compared to existing behavior of --fx-version).
Basically if I specify --fx-version today I get some relatively predictable behavior. After this change that behavior becomes less predictable since the ambient env. variable may modify it. Now I have to know about the env. variable (and the runtimeconfig setting to be able to decide the exact behavior.

I'm not sure if it's OK to leave it like that, or if there's something we should do... and I don't know what... just raising a concern.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a concern. The order of precedence for command line and ENV is really tough.

Here are two scenarios to consider (where both CLI and ENV config is used):

  • --fx-version 3.0.0 & ENV DOTNET_ROLL_FORWARD=Major
  • --fx-version 3.0.0 --roll-forward=Patch & ENV DOTNET_ROLL_FORWARD=Major

I now think I have it wrong. I think most people expect that the CLI would beat config in the second case. That said, I do think that the ENV needs to be honored in the first example to enable hosters to have control over a large number of apps.


1. `.runtimeconfig.json` properties (AKA "json")
2. CLI arguments
3. Environment variables (AKA "ENV")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add some reasoning why this order?
I understand why we want runtimeconfig to be the least important. I'm not sure about ENV winning over CLI. ENV may be "global" while CLI is always very specific to the app.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ya. I think you are right. I will switch.

* `.runtimeconfig.json` properties (AKA "json")
* CLI arguments
* Environment variables (AKA "ENV")
Note: The project file property is not listed because the project file is a build-time not runtime artifact. Runtime-oriented values in project files need to be written to `.runtimeconfig.json` as part of the build in order to influence runtime behavior.

The `version` and `roll-forward` settings compose in the following way:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to list the various ways to specify version. Is it the version of framework in runtimeconfig and --fx-version CLI, or is there something else as well?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am only aware of those two and cannot think of a useful other option. We could have an ENV to specify version, but I suspect you'd largely want that to test latest and we now have an ENV that does that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK - those two make sense - maybe we should list them here, as they are not explicitly mentioned together anywhere in this doc I think...

The host will consult these settings in order (later scopes take precendence over earlier ones):
The host will consult the various ways of setting `RollForward`, in order (later scopes take precendence over earlier ones):

1. `.runtimeconfig.json` properties (AKA "json")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that runtimeconfig has two levels. For example rollForwardOnNoCandidateFx can be specified globally (for all frameworks in the app) or locally per-framework.
I assume we would allow the same for rollForward, in which case there's a precedence to define.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would that look like? I don't think I'm familiar with this scenario.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.runtimeconfig.json can look like this for example:

{
  "runtimeOptions": {
    "tfm": "netcoreapp3.0",
    "applyPatches": false,
    "rollForwardOnNoCandidateFx": 0,
    "framework": {
      "name": "Microsoft.NETCore.App",
      "version": "3.0.0",
      "applyPatches": true,
      "rollForwardOnNoCandidateFx": 2
    }
  }
}

The settings per-framework win over those on the global level.

@@ -229,37 +225,80 @@ Can be specified via the following ways:
* Environment variable: `DOTNET_ROLL_FORWARD`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned below, runtimeconfig should probably support both global and per-framework setting.

Copy link
Member

@vitek-karas vitek-karas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

### RollForward

We will introduce a new knob, called `RollForward`, with the following values:
`RollForward` specifies the roll-forward policy for an application, either as a fallback in case a specific version
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably a copy paste error - the sentence seem to end too early.

* `LatestPatch` -- Roll forward to the highest patch verison. This disables minor version roll forward.
* `Minor` -- Roll forward to the lowest higher minor verison, if requested minor version is missing. If the requested minor version is present, then the `LatestPatch` policy is used.
* `Major` -- Roll forward to lowest higher major version, and lowest minor version, if requested major version is missing. If the requested major version is present, then the `Minor` policy is used.
* `LatestMinor` -- Roll forward to latest minor version, even if requested minor version is present. Intended for COM hosting scenario.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure we should explicitly state that this is for COM... it is useful in other cases as well. But if you think it makes sense, I'm fine with it.


Given a request for `x.y.z` version, the host will look for an `x` runtime version, as described for **Minor Version Selection** above. If it cannot find an `x` runtime, then the host will look for all runtimes that are higher than `x`. It will select the lowest major version within that set and then select the lowest minor and highest patch version within that set. This behavior is oriented on a best effort attempt at selecting a compatible installed product version. That's why the lowest minor version is selected, not the highest one.

**User Impact:** When a user runs an application on a machine that does not have `x` installed, but has a later runtime installed, then the app will transparently run on that version. The user does not need to configure their applications to use that later minor version. There is some risk in this scenario that the app will crash due to incompatibilities in the runtime relative to the runtime that the app targets and that it will be difficult to diagnose the cause (incompatibility)and the solution (installing a older runtime). If that app runs successfully, then the user will be happy. The tension between compatibility and deployment convenience is why this scenario is opt-in.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you meant "applications to use that later major version."

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will there be any info or warning text when the app starts to warn the user for the major version roll forward?

I see a developer just assuming there app will work for all future versions without knowing that version 4.0 changed his assumptions and the app crashes. If the app transparently rolls forward and crashes the user never realizes they can solve the problem them selves by installing the appropriate version of .NET Core and instead blame the original developer.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our philosophy is that the developer owns stdout and stderr. In part, one app's stdout in another app's stdin. stderr is more an option, but is also not perfect.

My belief is that it should be uncommon for apps to run in environments where major version rollforward is enabled. If apps crash in such an environment, it should be possible to connect the opt-in rollforward setting with the crash and consider that the setting is the cause of the crash if no other more obvious causes are identified.

Fair?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What other mechanism do you think should be used to notify about the roll-forward happening.
On Windows we were discussing that maybe we should write an information event into the event log.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's the only good option I have at the moment. We could also expose it via the new runtime events so that various in-proc event loggers could collect this info.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea of it getting put out as a runtime event.

proposed/runtime-binding.md Outdated Show resolved Hide resolved

### Configuration Precedence

The host will consult the various ways of setting `RollForward`, in order (later scopes take precendence over earlier ones):
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo precendence -> precedence

@normj
Copy link

normj commented Feb 6, 2019

LGTM

It requires a high bar of compatibility between minor versions and protects .NET Core's ability to evolve with breaking changes when they are important for major versions.

@richlander richlander merged commit 3f28add into master Feb 7, 2019
@jkotas jkotas deleted the rollforward branch April 29, 2020 15:56
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.

6 participants