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

Use host model for apphost #3447

Merged
merged 9 commits into from
Jul 27, 2019
Merged

Conversation

wli3
Copy link

@wli3 wli3 commented Jul 23, 2019

part of #11300

@wli3
Copy link
Author

wli3 commented Jul 25, 2019

@nguerrera do you mind to review this PR now. I am still waiting for core-setup bit flow to make tests pass. But I hope to get that in before monday so I won't need to be in ask mode

@nguerrera
Copy link
Contributor

Looks good, now that CLI has host model, you will need to get it through core-sdk and back to stage 0, unfortunately, I think. Otherwise you will get the one from CLi loaded, I think. :( This is another data point to put together my repo consolidation proposal....

You can add ExcludeAssets=runtime for .net core to HostModel package reference now I think, like dependency model. This will save us an unnecesarry copy of the dll in the full SDK.

@wli3 wli3 force-pushed the use-HostModel-for-apphost branch 3 times, most recently from fcd794f to 1168706 Compare July 26, 2019 18:04
@wli3 wli3 closed this Jul 26, 2019
@wli3 wli3 reopened this Jul 26, 2019
William Li and others added 5 commits July 26, 2019 11:16
@wli3 wli3 force-pushed the use-HostModel-for-apphost branch from 93708ac to b6e12c7 Compare July 26, 2019 18:17
@wli3 wli3 marked this pull request as ready for review July 26, 2019 20:30
@wli3
Copy link
Author

wli3 commented Jul 26, 2019

@nguerrera tests should all pass now. It is good to review. I will squash merge it

From last time you review.

  1. I updated stage 0 to have the latest bits
  2. exclude hostmodel in runtime a0c6810
  3. manual update Hostmodel version, looks like
    a bad merge cause it cannot be updated automatically 3fcf529
  4. A test find that I issues the warning even when it is not a Windows dll. I need to condition the warning. And also I need to "forward" $(WindowsGraphicalUserInterface) to generate shim as well 3573711

@@ -113,20 +113,20 @@ protected override void ExecuteCore()

try
{
var windowsGraphicalUserInterface = runtimeIdentifier.StartsWith("win") && "WinExe".Equals(OutputType, StringComparison.OrdinalIgnoreCase);
Copy link
Author

@wli3 wli3 Jul 26, 2019

Choose a reason for hiding this comment

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

@nguerrera @peterhuene

I added this change since last review.

sorry for the churn. The feedback loop of tests is slow.

I realize I do need to recalculate WindowsGraphicalUserInterface, since WindowsGraphicalUserInterface depends on UseApphost which will always be false in dotnet tools.

@wli3 wli3 merged commit 1f061a6 into dotnet:master Jul 27, 2019
@wli3 wli3 deleted the use-HostModel-for-apphost branch July 27, 2019 01:24
dsplaisted pushed a commit to dsplaisted/sdk that referenced this pull request Feb 19, 2020
…0191103.2 (dotnet#3447)

- Microsoft.NETCore.App.Ref - 3.1.0-preview3.19553.2
- Microsoft.NETCore.App.Runtime.win-x64 - 3.1.0-preview3.19553.2

Dependency coherency updates

- Microsoft.NET.Sdk.WindowsDesktop - 3.1.0-preview2.19553.1 (parent: Microsoft.NETCore.App.Runtime.win-x64)
- System.CodeDom - 4.7.0-preview3.19551.4 (parent: Microsoft.NETCore.App.Runtime.win-x64)
- System.Security.Cryptography.ProtectedData - 4.7.0-preview3.19551.4 (parent: Microsoft.NETCore.App.Runtime.win-x64)
- System.Text.Encoding.CodePages - 4.7.0-preview3.19551.4 (parent: Microsoft.NETCore.App.Runtime.win-x64)
- System.Resources.Extensions - 4.7.0-preview3.19551.4 (parent: Microsoft.NETCore.App.Runtime.win-x64)
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.

2 participants