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.

Sign up for GitHub

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

Jump to bottom

Add logic to roll forward to patch versions of .NET Core for self-contained apps but not shared framework apps or libraries #1222

Merged
merged 2 commits into from
May 24, 2017
Merged

Add logic to roll forward to patch versions of .NET Core for self-contained apps but not shared framework apps or libraries #1222

merged 2 commits into from
May 24, 2017
+116 −36

Conversation

dsplaisted
Copy link
Member

Fixes #983

@dasMulli
Copy link
Contributor

Am I correct in assuming the change will affect the version of the NuGet package being referenced? This would then require a restore using the same properties when called through msbuild directly.. (breaking change?).
Are there any downsides to just downgrading the version written to the runtimeconfig.json to a .0 patch and using the latest NuGet?

@dsplaisted
Copy link
Member Author

By itself this doesn't change anything, as we only want to apply the new logic to patches that we ship in the future.

Previously we were rolling forward to the latest patch version (that existed at the time the SDK ships) for all apps. This changes the behavior so that in the future, we will only roll forward to patch versions for self-contained apps, while shared framework apps will continue to target version x.y.0.

There was some discussion about referencing newer NuGet packages but downgrading the version written to the runtimeconfig.json. However, that feels a bit dangerous to me. In general we don't support compiling against a later version of something and running on a previous version of it. It's likely that we won't add any APIs in patch versions or make any other changes that would break this, but I think it's simpler and safer to keep them in sync.

Does that help explain things?

@dasMulli
Copy link
Contributor

Yes, thanks!
So it comes down to "protect against APIs that won't be available" vs "make sure fixes in the ref package are consumed" 😂

So this likely requires /t:Restore;Publish /p:[PropsForSCD] when publishing with msbuild or it might use outdated packages silently?

@dsplaisted
Copy link
Member Author

@dotnet-bot test Windows_NT_FullFramework Debug

@dsplaisted
Copy link
Member Author

Yeah, if we have another issue where there's an issue we want to fix with the package but not the runtime, then we might need to think about a split between the package version and the version that goes in runtimeconfig.json. I hope it doesn't come to that :-)

So this likely requires /t:Restore;Publish /p:[PropsForSCD] when publishing with msbuild or it might use outdated packages silently?

  • Combining the Restore target with pretty much any other target in the same msbuild invocation isn't supported. .props and .targets files that are written by the restore target won't be imported when it runs subsequent targets.
  • The properties added in this PR are implementation details and test hooks. You shouldn't need to set them. If you want to target a specific patch, then you do so by setting the RuntimeFrameworkVersion property (and this PR doesn't change that).

@dsplaisted
Copy link
Member Author

@dotnet-bot test Windows_NT_FullFramework Debug

@dsplaisted
Copy link
Member Author

@MattGertz for approval

@MattGertz
Copy link

@dsplaisted Template?

eerhardt
eerhardt reviewed May 19, 2017
View reviewed changes
src/Tasks/Microsoft.NET.Build.Tasks/build/Microsoft.NET.Sdk.DefaultItems.targets Outdated
<!-- If not covered by the previous cases, use the target framework version for the implicit RuntimeFrameworkVersions -->
<Otherwise>
<PropertyGroup>
<ImplicitRuntimeFrameworkVersionForSharedApp>$(_TargetFrameworkVersionWithoutV)</ImplicitRuntimeFrameworkVersionForSharedApp>
Copy link
Member

Choose a reason for hiding this comment

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

I'd use the term FrameworkDependent in code. This is the term we use in documentation: https://docs.microsoft.com/en-us/dotnet/articles/core/deploying/

And also in the rest of the SDK code: https://github.com/dotnet/sdk/blob/master/src/Tasks/Microsoft.NET.Build.Tasks/ProjectContext.cs#L19-L23

eerhardt
eerhardt reviewed May 19, 2017
View reviewed changes
test/Microsoft.NET.Build.Tests/GivenThatWeWantToBuildANetCoreApp.cs Outdated

private void It_targets_the_right_framework(
string testIdentifier,
string targetFramework, string runtimeFrameworkVersion,
Copy link
Member

Choose a reason for hiding this comment

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

NIT: I think putting some args on new lines, and some inline makes this hard to read.

eerhardt
eerhardt reviewed May 19, 2017
View reviewed changes
test/Microsoft.NET.Build.Tests/GivenThatWeWantToBuildANetCoreApp.cs Outdated
@@ -30,37 +30,75 @@ public class GivenThatWeWantToBuildANetCoreApp : SdkTest
[InlineData("netcoreapp1.1", null, "1.1.2", "1.1.2")]
[InlineData("netcoreapp1.1", "1.1.0", "1.1.0", "1.1.0")]
[InlineData("netcoreapp1.1.1", null, "1.1.1", "1.1.1")]
public void It_targets_the_right_shared_framework(string targetFramework, string runtimeFrameworkVersion,
private void It_targets_the_right_shared_framework(string targetFramework, string runtimeFrameworkVersion,
Copy link
Member

Choose a reason for hiding this comment

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

Why did this change to private?

@dsplaisted
Add logic to roll forward to patch versions of .NET Core for self-con…
d5b0fe6 
…tained apps but not shared framework apps or libraries

Fixes dotnet#983
@dsplaisted dsplaisted changed the base branch from master to release/2.0.0 May 24, 2017 20:55
@dsplaisted dsplaisted force-pushed the 983-implicit-patch branch from daac244 to c817abd Compare May 24, 2017 20:56
@dsplaisted
Copy link
Member Author

@MattGertz for approval

Customer scenario

Targeting and running an app. This change means we will default to targeting the latest patch version of .NET Core for self-contained apps (so they get the latest updates), but for framework-dependent apps, we will target the Major.Minor.0 version, and rely on the runtime roll-forward policy. This means framework-dependent apps won't fail to run when deployed to hosts that haven't updated the shared framework.

Bugs this fixes:

#983

Workarounds, if any

Explicitly set RuntimeFrameworkVersion.

Risk

Low.

Performance impact

Low - this is an update to logic which is statically evaluated by MSBuild

Is this a regression from a previous update?

Yes. We had policy (first introduced in #878) which would cause regressions in SDK updates.

Root cause analysis:

Hosts that provide a shared framework aren't necessarily immediately updated to a new runtime when we release that runtime together with an SDK.

How was the bug found?

Hosters and developers deploying to hosts reported the issue when they found that apps were broken when publishing to the hosts.

@dsplaisted dsplaisted force-pushed the 983-implicit-patch branch from c817abd to 325c345 Compare May 24, 2017 21:16
@dsplaisted dsplaisted merged commit 935c5a1 into dotnet:release/2.0.0 May 24, 2017
@dsplaisted dsplaisted deleted the 983-implicit-patch branch May 24, 2017 22:38
@dsplaisted dsplaisted mentioned this pull request May 25, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eerhardt eerhardt eerhardt approved these changes

@nguerrera nguerrera nguerrera approved these changes

@livarcocc livarcocc Awaiting requested review from livarcocc

Assignees
No one assigned
Labels
Approved to merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@dsplaisted @dasMulli @MattGertz @nguerrera @eerhardt @dnfclas