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

Reconsider implicitly using the latest patch version of the targeted version of .NET Core in the SDK #983

Closed
dsplaisted opened this issue Mar 13, 2017 · 34 comments
Assignees
Milestone

Comments

@dsplaisted
Copy link
Member

dsplaisted commented Mar 13, 2017

From @eerhardt:

The issue is someone upgrades their SDK on their developer machine, which brings in the latest patch version (1.1.1 in this case) and then publishes their app to their host machine. The host machines aren't updated to 1.1.1, because it just got released and these things take time. Now the app breaks on a host machine because 1.1.1 isn't installed.

See more comments/discussion on #860.

UPDATE: The current plan of record is now:

  • The implicit framework version is not updated with each new SDK (even if it includes newer runtimes (patch version)). This makes apps with newer SDKs compatible to a larger set of environments. The implicit framework version always stays as at the “.0” patch
  • Self-contained apps are published with the latest patch version (same as “run” case) unless specifically configured.

More details and discussion can be found here: https://github.com/dotnet/designs/issues/3

When targeting .NET Core 1.x, the 2.0 SDK should continue to roll forward to at least the latest patch that the 1.x SDK rolls forward to, in order to have consistent behavior between the two SDKs.

This means that there's not any concrete work needed to support the new policy until a patch version of 2.0 ships.

@dsplaisted
Copy link
Member Author

My suggestion would be that instead of using RuntimeFrameworkVersion to specify a patch version of the runtime to use, we just use the TargetFramework. IE, to target 1.1.1:

<TargetFramework>netcoreapp1.1.1</TargetFramework>

Then if we wanted to target 1.1.1 by default, we could just do that in the default template, and it's a lot more obvious to simply retarget to 1.1 than to have to set the RuntimeFrameworkVersion property.

@normj
Copy link
Contributor

normj commented Mar 14, 2017

Alternatively if the patch version is going to implicit to the latest whether the user is ready for it or not should the dotnet execution of the application be more lenient and look for the closet matching runtime and not just fail if the exact version isn't found?

@normj
Copy link
Contributor

normj commented Mar 22, 2017

Pinging on this. I see more work going on for the implicit versioning but I don't understand how this is sustainable. Unless every .NET Core hosting environment, whether on premise or in a Cloud provider, can instantly update as soon as a new patch version is release this will continue to cause deployment pain.

Are there plans to make the implicit versioning to latest patch version for netcoreapp frameworks and latest major/patch version for netstandard libraries more discover-able?

@dsplaisted
Copy link
Member Author

@normj I think the work you're probably seeing for implicit versioning is what we're doing for preview versions of the .NET Core Shared Framework. Would you expect hosting environments to provide a preview version of the shared framework?

The reason for the work we're doing for the preview versions of the shared framework is so that you don't have to hard code something like <RuntimeFrameworkVersion>2.0.0-beta-001507</RuntimeFrameworkVersion> in your project.

As for the non-preview policy of rolling forward to the latest patch version in the SDK, I don't have any update yet.

@Petermarcu
Copy link
Member

FWIW, I hope to not overload the TFM to include patch versions. I think apps running on a shared framework should only declare the major.minor and have a more advanced knob if they for some reason depend on a patch version.

For apps that pull the runtime app local, they should probably get the latest that came with the SDK they are using. @eerhardt is going to provide more input but it is definitely a scenario we need to cover. Devs should be able to get the latest SDK and still, by default, be able to publish apps and run against a major.minor.

@eerhardt
Copy link
Member

eerhardt commented Mar 30, 2017

I think the high-level strategy I am landing on here is:

  1. For applications running on the shared framework, I express my dependency on the platform through a TFM (netcoreapp1.1). By default, unless I specify otherwise, I should run on whichever patch version of that framework someone has installed. Ex. My app will run on any 1.1.x that is installed on the machine.
    1. The host will automatically roll-forward to the latest installed patch version, as it does today.
  2. For self-contained apps where the platform is with my app, by default I will get the latest runtime patch version that comes with the SDK I used to publish my app. This keeps my application up to date and secure when I use the latest SDK.

From an implementation perspective, what this means to me is:
  • The implicit PackageReference to Microsoft.NETCore.App uses the latest patch version that comes with the SDK. <PackageReference Include=”Microsoft.NETCore.App” Version="1.1.1" />
  • For self-contained apps, it will use the runtime assemblies from the referenced version (i.e. 1.1.1).
  • For shared framework apps, we will generate a runtimeconfig.json to say version=1.1.0 by default.

This means is we will use the latest NuGet package to build the app in all cases. But shared fx apps can run on any runtime that supports the specified TFM (netcoreapp1.1 in this case).


A place where this gets tricky is "How do we give the dev all the right knobs, but still keep it simple?"

I could see having 2 separate .csproj properties:

  1. Which version of Microsoft.NETCore.App do you want to reference?
  2. What is the minimum patch version of the shared fx do you want your app to run on?
    a. In the case where we ship a behavior change from 1.1.0 to 1.1.1, and the dev wants to say “this app cannot run on 1.1.0 because of the behavior change”

Or we could continue using the 1 property we have today: <RuntimeFrameworkVersion>. If the project explicitly has it set, use that version for both the PackageReference and to put in the runtimeconfig.json. Otherwise, use the defaults as outlined above.

@dsplaisted
Copy link
Member Author

Having a different version in the runtimeconfig.json for shared framework apps than for the NuGet package or for the runtimeconfig.json for a self-contained app worries me a bit. However, it does seem like it might be the best trade-off.

As far as having the right knobs, I really hope that a developer having to think about which version of the Microsoft.NETCore.App package should be referenced would be an extremely rare occurrence. I'm also not really happy with the way the RuntimeFrameworkVersion property works today. Someone may set it to 1.1.0 today so that their app will run on AWS, for example, and then when 2.0 is released they could retarget to netcoreapp2.0, but if they don't delete the RuntimeFrameworkVersion property then they will end up with a broken app that doesn't run. That's why I suggested just using the TargetFramework property to specify the patch version. Another option could be to have a property that only specifies the patch version, and is appended to the version which comes from the TargetFramework. So to target 1.1.0 instead of 1.1.1, you would set the TargetFramework to netcoreapp1.0, and the new property (RuntimeFrameworkPatchVersion or something) to just 0.

@eerhardt
Copy link
Member

or for the runtimeconfig.json for a self-contained app

There is no version in the runtimeconfig.json for self-contained apps. The presence of a name and version in this file is what tells the host if the app is self-contained or not.

So to target 1.1.0 instead of 1.1.1, you would set the TargetFramework to netcoreapp1.0, and the new property (RuntimeFrameworkPatchVersion or something) to just 0.

This doesn't solve the same problem you list for RuntimeFrameworkVersion. If I'm on netcoreapp1.0, and I want to target 1.0.3, I would set RuntimeFrameworkPatchVersion = 3. Then when I moved to netcoreapp1.1 and didn't delete this property, my app breaks.

@dsplaisted
Copy link
Member Author

This doesn't solve the same problem you list for RuntimeFrameworkVersion. If I'm on netcoreapp1.0, and I want to target 1.0.3, I would set RuntimeFrameworkPatchVersion = 3. Then when I moved to netcoreapp1.1 and didn't delete this property, my app breaks.

Yes, but my thought is that it probably breaks with a better error experience where you can understand what's happening and fix it.

@richlander
Copy link
Member

Here's an idea:

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework targetRuntime="2.0.3" minRuntime="2.0.0">netcoreapp2.0</TargetFramework>
  </PropertyGroup>

</Project>

There are a few ideas here:

  • Carry these extra declarations with the target framework since they are coupled, as suggested.
  • The declarations don't have to be pretty since they are intended to be atypical.
  • targetRuntime is the runtime you are targeting that satisfies the target framework. By default, this is the runtime that the SDK implicitly specifies but can be overridden, as we see above.
  • minRuntime is the minimum runtime that the app will accept on the production machine. By default this is the .0 patch. We would only update this to >0 if we find something bad in .0 (or higher).

@Petermarcu
Copy link
Member

Petermarcu commented Mar 31, 2017

I like the coupling with the TargetFramework. I have a few suggestions/comments:

  • targetRuntime would need to be able to be resolved to a full package including prerelease versions of packages. So it can't be just a normal version string. It needs to support the 2.0.0-preview2-004353.00 type strings.
  • I'm not sure about minRuntime. The first two parts need to match. We don't roll forward across major or minor. I think only overriding the 3rd part here is interesting and should probably just be called minPatchVersion or something like that.
  • This should only be done for 2.0+

@richlander
Copy link
Member

On targetRuntime, yes. The value should just be a string. That doesn't preclude your scenario..

On minRuntime, I'm less sure. What's the "third part" in your preview2 example? I think that fully-qualified version numbers are easier to understand. You are correct that there is an inherent relationship between the two. That said, one could argue that there is a similar relationship with the TFM.

There is a question of what should happen if targetRuntime is missing. Is this also a min version?

@Petermarcu
Copy link
Member

Correct on targetRuntime not precluding my scenario. I just wanted it to be clear that it had to be a string.

Good point about the string and its relationship to minRuntime. I would think it would then be an error if:

  1. minRuntime didn't match for major.minor with the targetRuntime that is implied or explicit.
    OR
  2. minRuntime > targetRuntime whether it's implied or explicit.

In the end, minRuntime may make even more sense for pre-releases where breaks are more likely to occur and people will want to say you have to have at least a certain version of them.

@richlander
Copy link
Member

richlander commented Mar 31, 2017

That makes sense. Agree on the pre-release scenario.

Here are some rules:

  • The targetRuntime runtime must exist (it's an override!), or error
  • minRuntime == targetRuntime on major.minor if both are specified
  • minRuntime <= targetRuntime whether implied or explicit.
  • It is OK to specify either minRuntime or targetRuntime w/o the other option present

Effect:

  • targetRuntime is the runtime used for compile-time assets and the source of runtime assets for standalone apps. It is not necessarily the runtime that an app runs on for the run dotnet run or dotnet path-to-dll scenarios, which could be later. By default, the targetRuntime value will be set to the runtime that was included with the SDK for the given target framework.
  • minRuntime is used as the required lower-bound .NET Core runtime (a higher patch version is fine), only for dotnet run and dotnet path-to-dll scenarios. It doesn't affect dotnet publish, which always using targetRuntime. By default, the minRuntime value will be the ".0" patch version of the major.minor version being targeting (this is the improvement!).

Any reason that this is 2.0+? Certainly, for the 2.0 tools. Any reason why this wouldn't work when targeting 1.x runtimes?

@Petermarcu
Copy link
Member

I think for minRuntime you need to pivot on Self-contained vs Shared. For self-contained, you get the runtime from the SDK, not the ".0" version.

For 1.x runtimes, I believe there was other logic. I'll say the requirement less strictly. We can't break 1.X apps when they install and start using the 2.0 SDK.

@richlander
Copy link
Member

@Petermarcu I revised the text above (no need to repeat it). Can you check?

@Petermarcu
Copy link
Member

I don't fully understand what you mean by targetRuntime must exist. Do you mean the attribute or that something has to infer it if its not specified? I didn't expect that someone would have to set it and that it would be inferred from the TFM.

Maybe we should write out the scenarios and what happens? I'm honestly having a hard time parsing it out of the words. Not sure if expanding it is better but it at least forces me to think through the matrix:


All scenarios are described in the context of a published application. The default is running on a shared .NET Core 2.0 runtime. Self-contained means that you have decided to publish the runtime with your application. Let's assume the SDK that is being used is the .NET Core 2.0.3 SDK.

I'll give it a stab but I will probably not be exhaustive.

Neither targetRuntime or minRuntime are specified:

<TargetFramework>netcoreapp2.0</TargetFramework>

  • Result: Application must be run in a configuration where at least 2.0.0 is present. A self-contained application will get 2.0.3.

Both targetRuntime and minRuntime are specified:

<TargetFramework targetRuntime="2.0.3" minRuntime="2.0.0">netcoreapp2.0</TargetFramework>

  • Result: Application must be run in a configuration where at least 2.0.0 is present. A self-contained application will get 2.0.3.

<TargetFramework targetRuntime="2.0.3" minRuntime="1.0.0">netcoreapp2.0</TargetFramework>

  • Result: Error because minRuntime doesn't match major.minor.

<TargetFramework targetRuntime="2.0.3" minRuntime="2.0.4">netcoreapp2.0</TargetFramework>

  • Result: Error because minRuntime is greater than targetRuntime.

<TargetFramework targetRuntime="2.0.4" minRuntime="2.0.4">netcoreapp2.0</TargetFramework>

  • Result: Application will compile but not run unless the the 2.0.4 runtime is made available. Self-contained application will get 2.0.4.

Just targetRuntime is specified:

<TargetFramework targetRuntime="2.0.3">netcoreapp2.0</TargetFramework>

  • Result: Application must be run in a configuration where at least 2.0.0 is present. A self-contained application will get 2.0.3.

<TargetFramework targetRuntime="1.0.0">netcoreapp2.0</TargetFramework>

  • Result: Error because targetRuntime doesn't match major.minor of inferred 2.0.0.

<TargetFramework targetRuntime="2.0.4">netcoreapp2.0</TargetFramework>

  • Result: Application will work and run on 2.0.3 because that's what came with the SDK and will have a minVersion of 2.0.0 which will allow it to run anywhere at least 2.0.0 is present. A self-contained application will get 2.0.4.

Just minRuntime is specified:

<TargetFramework minRuntime="2.0.3">netcoreapp2.0</TargetFramework>

  • Result: Application must be run in a configuration where at least 2.0.3 is present. A self-contained application will get 2.0.3.

<TargetFramework minRuntime="1.0.0">netcoreapp2.0</TargetFramework>

  • Result: Error because minRuntime doesn't match major.minor of inferred 2.0.0 for targetRuntime.

<TargetFramework minRuntime="2.0.4">netcoreapp2.0</TargetFramework>

  • Result: Application will compile but will only run if 2.0.4 is made available to it. A self-contained application will get 2.0.4 with it.

I'll stop there for now. It's not 100%. This can probably be simplified by breaking it up by "shared" and "self-contained". Unfortunately, I don't think github is the best medium for this to be spec'd out. Once we land on something, we should put it somewhere to get feedback.

@richlander
Copy link
Member

I meant this case: <TargetFramework targetRuntime="2.0.9999999">netcoreapp2.0</TargetFramework>

What happens in that case, if 2.0.3 is installed?

All your cases look good.

@Petermarcu
Copy link
Member

If that version doesn't exist, error. If it does, it would be just like my 2.0.4 example I think.

@richlander
Copy link
Member

richlander commented Apr 1, 2017

Correct. That's all I was trying to say initially, referring to your question. We agree.

This now needs to get converted to a design doc given the scope of the issue. We should also handle minor version roll-forward (in the case that your version is missing) in that doc. I'd like to post the final result at dotnet/designs (doesn't exist yet). Should we use an approach kinda like this: dotnet/csharplang#386 ?

@Petermarcu
Copy link
Member

Petermarcu commented Apr 2, 2017

Sounds like we're on the same page. I personally think iterating on a design on github is a really unproductive experience. Wish there was a better way to collaborate and iterate on a design. Some type of shared workspace or document. Once everyone is close to the same page on the whole thing, putting it in a more static form like github md makes sense. I'd love to hear how @gafter and @MadsTorgersen to tight iteration on the design docs as they come to a conclusion.

@richlander
Copy link
Member

I think iterating on GitHub issues is the unproductive experience. I'd like to think working on a shared document is better. A PR on a shared branch with room for comments seems like it might work well. I'm hoping that there is something that works well beyond a closed conference room w/a whiteboard.

@Petermarcu
Copy link
Member

I agree with that. Just wish there was something better than what we can do with GitHub.

@dasMulli
Copy link
Contributor

dasMulli commented Apr 3, 2017

Is there an issue / proposal yet for metadata on MSBuild properties?
Right now minRuntime and targetRuntime would have to be separate properties..

I'd more like to see properties that express the user's intent rather than to add 2 more version properties.
For example:

  1. A user just targets netcoreapp1.1 and publishes:
    • M.N.App reference is 1.1.1, runtimeconfig is 1.1.0.
    • Maybe the SDK emits a 1.1.* reference here so NuGet will pick up new releases automatically.
  2. (1) + publishes for a self-contained runtime:
    • Since the pkg ref is 1.1.1, he gets the latest runtime automatically.
  3. (1) + the user wants to ensure that all production machines have the latest patch version insalled that is known at deploy time.
    • User can set sth like <TargetLatestRuntimePatchVersion>true</TargetLatestRuntimePatchVersion >.
    • => Runtimeconfig will have 1.1.1
  4. User wants to use a prerelease version:
  5. User wants to try out prerelease tooling:
    • User just writes <TargetFramework>netcoreapp2.0</TargetFramework>.
    • Preview CLI includes props files describing the version of the runtime installed with the CLI and SDK picks it up for runtimeconfig and NuGet reference (i think this is what has been implemented recently)

@Petermarcu
Copy link
Member

This is good feedback. Let's make sure its incorporated into the design discussion wherever @richlander decides to put it.

@dsplaisted
Copy link
Member Author

Is there an issue / proposal yet for metadata on MSBuild properties?

@richlander @Petermarcu I just want to highlight that "coupling" additional information to the TargetFramework property isn't something that MSBuild supports. MSBuild items can have metadata which can now be expressed as attributes (for example the Version in <PackageReference Include="System.Collections.Immutable" Version="1.0.0" />), but there's no corresponding concept for properties.

@livarcocc livarcocc added this to the 2.0 milestone Apr 20, 2017
@livarcocc livarcocc removed this from the 2.0 M1 milestone Apr 20, 2017
@normj
Copy link
Contributor

normj commented Apr 20, 2017

Just adding my vote for @Petermarcu design rules. I'm assuming in the case of <TargetFramework>netcoreapp2.0</TargetFramework> and the machine has both 2.0.0 and 2.0.3 installed that 2.0.3 would be used.

Is there ever a time targetRuntime affects shared runtime apps? If it is only useful for self-contained apps it would be more clear if the name indicated that. The name targetRuntime is pretty generic and could be confused with controlling other packaging aspects.

I'm confused about the supported versions. Is the proposal that this runtime determination will only affect netcoreapp2.0 apps? So developers with netcoreapp1.0 will be left in the difficult position they are in now where every patch breaks deployments till their deployment environment is updated with the patch.

@richlander
Copy link
Member

@dsplaisted and I met on this today. I'm in the process of writing up our conclusions.

@richlander
Copy link
Member

FYI: The proposal is close to done. Just wanted folks to know that this is still in the works.

@richlander
Copy link
Member

richlander commented May 4, 2017

The proposal has just been posted. I would greatly appreciate your feedback.

See https://github.com/dotnet/designs/issues/3.

/cc @normj

@dsplaisted
Copy link
Member Author

I've updated the original issue with the updated plan of record based on https://github.com/dotnet/designs/issues/3

@NikolaosWakem
Copy link

as of VS2017 15.2 everyone needs to edit their csproj file with
netcoreapp1.1.1

if they don't want 502.5 Processs Failure Error on Azure as 1.1.2 isn't on Azure - this definitely needs to be added to VS2017 interface and/or the ability to ship a self contained website (including framework)

dsplaisted added a commit to dsplaisted/sdk that referenced this issue May 17, 2017
…tained apps but not shared framework apps or libraries

Fixes dotnet#983
dsplaisted added a commit to dsplaisted/sdk that referenced this issue May 24, 2017
…tained apps but not shared framework apps or libraries

Fixes dotnet#983
@dsplaisted
Copy link
Member Author

Fixed by #1222

@aguinaldok4
Copy link

From @eerhardt:

The issue is someone upgrades their SDK on their developer machine, which brings in the latest patch version (1.1.1 in this case) and then publishes their app to their host machine. The host machines aren't updated to 1.1.1, because it just got released and these things take time. Now the app breaks on a host machine because 1.1.1 isn't installed.

See more comments/discussion on #860.

UPDATE: The current plan of record is now:

  • The implicit framework version is not updated with each new SDK (even if it includes newer runtimes (patch version)). This makes apps with newer SDKs compatible to a larger set of environments. The implicit framework version always stays as at the “.0” patch
  • Self-contained apps are published with the latest patch version (same as “run” case) unless specifically configured.

More details and discussion can be found here: https://github.com/dotnet/designs/issues/3

When targeting .NET Core 1.x, the 2.0 SDK should continue to roll forward to at least the latest patch that the 1.x SDK rolls forward to, in order to have consistent behavior between the two SDKs.

This means that there's not any concrete work needed to support the new policy until a patch version of 2.0 ships.

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

No branches or pull requests

9 participants