-
Notifications
You must be signed in to change notification settings - Fork 223
Check for min runtime version at application start #2675
Conversation
@@ -30,18 +30,20 @@ public class ApplicationHostContext | |||
|
|||
public bool SkipLockfileValidation { get; set; } | |||
|
|||
public bool SkipRuntimeMinVersionCheck { get; set; } | |||
|
|||
public FrameworkReferenceResolver FrameworkResolver { get; set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add the RuntimeVersion
to the ApplicationHostContext, so we don't have to flow this thing everywhere.
Is it possible to add tests? |
eb4c9e0
to
5f4829d
Compare
Updated with tests |
5f4829d
to
f228654
Compare
// Ignore projects references because in that case we are compiling from sources | ||
// and versions don't match | ||
if (lib.Type == LibraryTypes.Package && | ||
lib.Identity.Name.Equals("Microsoft.Dnx.Runtime.Abstractions", StringComparison.Ordinal)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we extract the string Microsoft.Dnx.Runtime.Abstractions
in way rather than hard code the string. For example reflecting a type name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd avoid reflection since this is in the critical runtime path. But putting it in a constant is a good idea :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm with @anurse on this one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, though if we make other name changes we'll have to catch this case as well.
Looking good. A few comments inline. |
|
||
if (abstractions != null) | ||
{ | ||
minRequiredRuntime = abstractions.Identity.Version.ToString(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you turning this into a string and then back into a version?
After giving this some thought, I think this change should be here: https://github.com/aspnet/dnx/blob/dev/src/Microsoft.Dnx.ApplicationHost/DefaultHost.cs#L172 It's only relevant at runtime, we're trying to protect people from themselves and since we only throw at runtime, why put it in ApplicationHostContext. |
@@ -13,5 +13,8 @@ | |||
<PropertyGroup> | |||
<SchemaVersion>2.0</SchemaVersion> | |||
</PropertyGroup> | |||
<PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Debug|AnyCPU'"> | |||
<ProduceOutputsOnBuild>True</ProduceOutputsOnBuild> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whoops, forgot these. Will revert
d1cf461
to
daf9993
Compare
updated |
// because when building from sources the versions are not ordered | ||
// correctly | ||
if (lib.Type == LibraryTypes.Package && | ||
string.Equals(lib.Identity.Name, "Microsoft.Dnx.Runtime.Abstractions", StringComparison.Ordinal) && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Constantify, as discussed earlier ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe typeof(SomethingInThatAssembly).GetTypeInfo().Assembly.GetName().Name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will change now
LGTM |
@@ -183,6 +185,22 @@ private void Initialize(RuntimeOptions options, IServiceProvider hostServices, I | |||
AddBreadcrumbs(libraries); | |||
} | |||
|
|||
private void ValidateMinRuntimeVersion(IEnumerable<LibraryDescription> libraries) | |||
{ | |||
foreach (var lib in libraries) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can avoid the 2 for loops and do the servicing and this in a single pass but that's not important right now.
|
b44bef2
to
d7299b9
Compare
d7299b9
to
82b1501
Compare
Fixes: #2197
please review @davidfowl @anurse