Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

Add os supported versions to host #4700

Merged
merged 6 commits into from
Oct 31, 2018

Conversation

vitek-karas
Copy link
Member

This change is Windows only.

  • Add explicit manifest to dotnet.exe which specifies the supported OS versions.
  • Remove any manifest from apphost.exe as it should get the manifest from the app when it's customized.

Fixes #3638

Also remove any manifest from apphost.exe as it will get the one from the app when it's used
@vitek-karas
Copy link
Member Author

@ericstj Any idea on how to test this? I thought about an app similar to your sample in the issue, but in order to validate it the test would somehow have to be able to determine the correct version of the OS it runs on - without any compatibility magic. Not sure if there's a way to do that.

Copy link
Member

@sbomer sbomer left a comment

Choose a reason for hiding this comment

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

LGTM, thank you for fixing this!

Copy link
Member

@ericstj ericstj left a comment

Choose a reason for hiding this comment

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

Make sure you test some set of apps with this change. Probably the built in apps plus the templates. We don’t want to break any of the basic scenarios and this does have that potential (due to turning off OS compatibility shims).

src/corehost/cli/dotnet/dotnet.manifest Show resolved Hide resolved
@vitek-karas
Copy link
Member Author

by built-in apps you mean all the "dotnet something" tools?
I was thinking about adding some automated tests for this... but I can't figure out how to write the test verification (as it would see the same potentially wrong version of the OS or shims) - any ideas?

@ericstj
Copy link
Member

ericstj commented Oct 25, 2018

Yeah, run dotnet tools. MSBuild is a good one, you could drop this version into a corefx repo and make sure it can still build, that'd be a nice test. For templates I was primarily thinking about dotnet new mvc, webapi, etc. Make sure none of those fall on the floor.

WRT to an automated test: you can call RtlGetVersion and it won't lie. This is what we do in System.Runtime.InteropServices.RuntimeInformation, but in that case we only expose it as a string:
https://github.com/dotnet/corefx/blob/1b16ca1c4bf1ea1a817bc4070362c53341e0ca10/src/Common/src/Interop/Windows/NtDll/Interop.RtlGetVersion.cs#L11-L34

So I imagine an automated test could fetch the current version from RtlGetVersion then ask VerifyVersionInfo to see if we're greater than or equal to that version. That'd have the nice characteristic of automatically failing when running on a new OS when we need to make a change.

@vitek-karas
Copy link
Member Author

Added automated test for dotnet.exe OS support. Unfortunately adding the same for apphost.exe is just too expensive right now as we don't have the infra to test with freshly built apphost. For now I'll leave it out and I'm discussing options with the SDK team to overcome this limitation.

@vitek-karas
Copy link
Member Author

I verified this with the new dotnet.exe:

  • I can dotnet new and then dotnet run project types console,winforms,wpf,web,mvc,webapi - everything seems to work just fine
  • I can build the CoreFx repo with the new dotnet.exe

I manually exported and double checked the manifest embeded in the new dotnet.exe - it contains the original manifest plus the supported OSes.

@ericstj
Copy link
Member

ericstj commented Oct 29, 2018

test OSX x64 Release Build

Console.WriteLine(string.Join(Environment.NewLine, args));
Console.WriteLine($"Framework Version:{GetFrameworkVersionFromAppDomain()}");

#if WINDOWS
Copy link
Member

Choose a reason for hiding this comment

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

Do you need the #ifdef? It looks like you only ever run this on Windows.

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 not needed, but I personally would prefer to keep it - makes it more explicit. I don't like code which doesn't run, but also doesn't work at all.

@vitek-karas
Copy link
Member Author

test OSX x64 Release Build

1 similar comment
@ericstj
Copy link
Member

ericstj commented Oct 29, 2018

test OSX x64 Release Build

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

Successfully merging this pull request may close these issues.

4 participants