Skip to content

Commit

Permalink
Merge uwp-winmd-migration
Browse files Browse the repository at this point in the history
  • Loading branch information
@lances101
lances101 committed Dec 24, 2017
1 parent 9264ef6 commit d8beaee
Show file tree
Hide file tree
Showing 3 changed files with 6 additions and 9 deletions.
12 changes: 5 additions & 7 deletions Plugins/Wox.Plugin.Program/Wox.Plugin.Program.csproj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,11 @@
<Reference Include="System.Xaml" />
<Reference Include="System.Xml" />
<Reference Include="System.Xml.Linq" />
<Reference Include="Windows, Version=255.255.255.255, Culture=neutral, processorArchitecture=MSIL">
<SpecificVersion>False</SpecificVersion>
<HintPath>$(MSBuildProgramFiles32)\Windows Kits\10\UnionMetadata\Windows.winmd</HintPath>
<Private>False</Private>
</Reference>
<Reference Include="WindowsBase" />
</ItemGroup>
<ItemGroup>
Expand Down Expand Up @@ -150,13 +155,6 @@
</ProjectReference>
</ItemGroup>
<Import Project="$(MSBuildToolsPath)\Microsoft.CSharp.targets" />
<Import Project="..\..\packages\UwpDesktop.10.0.14393.3\build\portable-net45+uap\UwpDesktop.targets" Condition="Exists('..\..\packages\UwpDesktop.10.0.14393.3\build\portable-net45+uap\UwpDesktop.targets')" />
<Target Name="EnsureNuGetPackageBuildImports" BeforeTargets="PrepareForBuild">
<PropertyGroup>
<ErrorText>This project references NuGet package(s) that are missing on this computer. Use NuGet Package Restore to download them. For more information, see http://go.microsoft.com/fwlink/?LinkID=322105. The missing file is {0}.</ErrorText>
</PropertyGroup>
<Error Condition="!Exists('..\..\packages\UwpDesktop.10.0.14393.3\build\portable-net45+uap\UwpDesktop.targets')" Text="$([System.String]::Format('$(ErrorText)', '..\..\packages\UwpDesktop.10.0.14393.3\build\portable-net45+uap\UwpDesktop.targets'))" />
</Target>
<!-- To modify your build process, add your task inside one of the targets below and uncomment it.
Other similar extension points exist, see Microsoft.Common.targets.
<Target Name="BeforeBuild">
Expand Down
1 change: 0 additions & 1 deletion Plugins/Wox.Plugin.Program/packages.config
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,5 +3,4 @@
<package id="JetBrains.Annotations" version="10.3.0" targetFramework="net452" />
<package id="Newtonsoft.Json" version="9.0.1" targetFramework="net452" />
<package id="System.Runtime" version="4.0.0" targetFramework="net452" />
<package id="UwpDesktop" version="10.0.14393.3" targetFramework="net452" />
</packages>
2 changes: 1 addition & 1 deletion appveyor.yml
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
version: 1.3.{build}
os: Visual Studio 2015
image: Visual Studio 2017
configuration: Release
platform: Any CPU
assembly_info:
Expand Down

13 comments on commit d8beaee

@lances101
Copy link
Member Author

Choose a reason for hiding this comment

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

Removes the UwpDesktop package and relies directly on the Windows 10 SDK.

@bao-qian
Copy link
Member

Choose a reason for hiding this comment

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

why? what benefit will we get?

@lances101
Copy link
Member Author

Choose a reason for hiding this comment

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

The main idea was moving away from an unmaintained library. UwpDesktop latest supported Windows 10 SDK version is 14393 (released around one year ago) and requires one of the three specific versions of the SDK (10240, 10586 or 14393). This commit would allow Wox to not be dependent on a version of the SDK that UwpDesktop supports while not really losing anything.

The only thing that is missing in this commit is adding a reference to $(MSBuildProgramFiles32)\Reference Assemblies\Microsoft\Framework\.NETCore\v4.5\System.Runtime.WindowsRuntime.dll so that UWP calls can be awaited.

I do agree that this is a minor convenience change and could be reverted if there are any doubts on the project's stability because of this.
What do you think?

@jhdxr
Copy link
Member

@jhdxr jhdxr commented on d8beaee Dec 26, 2017

Choose a reason for hiding this comment

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

Although I'm not professional in c# and its libraries, I think it's better to depend on the system SDK instead of a third party one. In addition, it's good to reduce dependency without feature losing.

@bao-qian
Copy link
Member

Choose a reason for hiding this comment

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

from what I remember, I use this package because call uwp api from .net is really confusing.
if we want to remove uwpdesktop, we need answer these questions:

  1. System.Runtime.WindowsRuntime.dll, can be found in following location, what's the difference?

    1. $(MSBuildProgramFiles32)\Reference Assemblies\Microsoft\Framework\.NETCore
    2. $(MSBuildProgramFiles32)\Reference Assemblies\Microsoft\Framework\.NETFramework
    3. $(MSBuildProgramFiles32)\Reference Assemblies\Microsoft\Framework\.NETPortable
    4. nuget https://www.nuget.org/packages/System.Runtime.WindowsRuntime/

  2. windows.winmd is too large. 5mb. should we add it into release package?

  3. there are 4 windows.winmd in $(MSBuildProgramFiles32)\Windows Kits\10\UnionMetadata, what's the difference?

  4. from this article https://blogs.msdn.microsoft.com/lucian/2015/10/23/how-to-call-uwp-apis-from-a-desktop-vbc-app/ , this windows.winmd is used for win8 not win10. Will we really need it? Will our app crash on win8 if we don't have it?

  5. uwp api separate into different contract, each contract has multiple version, which one should we use?
    e.g. Windows.Foundation.FoundationContract has 1.0.0.0 2.0.0.0 3.0.0.0

  6. what api from latest sdk will we need?

@lances101
Copy link
Member Author

Choose a reason for hiding this comment

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

@bao-qian, if you want to skip the big read, jump to the last paragraph where I explain my stance.

All of the articles I've been able to find on this matter seem to be written by people related to Microsoft and I have not found any formal documentation as to "why", but only "do it this way". UwpDesktop itself was created by two Microsoft employees, and I suppose that being able to communicate on different aspects of the platform in the same company helped a lot.

  1. The UwpDesktop package readme touches the subject of importing it, but does not really explain why that exact version. System.Runtime.WindowsRuntime indeed does contain extension methods for converting between tasks and windows runtime asynchronous operations. I am no expert on this matter, but as far as I understand, for System.Runtime.WindowsRuntime.dll we rely on the version compiled for .NET Core, due to the latter being the implementation of the .NET Standard on which the interoperability between WinRT (UWP) calls and .NET Framework relies upon.

  2. Windows.winmd is not added to the release, nor it is added to git. It's only referenced during compilation. (correct me if you've noticed a different behavior). UwpDesktop does actually copy all files it seems. This screenshot of the is from build 1.3.424 (files added by UwpDesktop):
    default
    Latest dev branch build comes without those extra 4.64 megabytes (extracted). The difference is not that big when compressed:
    default

  3. Again, there is close to zero documentation on this matter. I have the following files in my Windows Kits/10/UnionMetadata folder:

    • Windows.winmd
    • Facade/Windows.winmd
    • 10.0.16299.0/Windows.winmd
    • 10.0.16299.0/Facade/Windows.winmd
    • and other entries like the last two for different versions of the SDK

The Windows.winmd files contained in the Facade folders and the ones outside seem to be referencing the same types, the only difference is how:

Again, I could not find any information on this as to why and what should be used when. Microsoft themselves seem to be referencing UnionMetadata/Windows.winmd directly as can be seen in Microsoft's own bridging tutorials (for example this one).

All facade files are equal by checksum and seem to get rewritten each time a new API is installed or uninstalled. Whether as to they are the ones to be referenced in the project - who knows. No info on that.

  1. As you have seen in the files linked in point 3, Windows.winmd is what contains the actual references to the API. Therefore we definitely need them on Windows 10. As to whether this brakes compatibility with Windows 8.1, I will spin up a Windows 8 VM to try it.

  2. So this seems to be something that Microsoft has not really documented. Their own samples reference UnionMetadata/Windows.winmd which contains contract version 1, which (of course) does not contain methods from UnionMetadata/10.0.16299.0/Windows.winmd (contract version 3). As far as I've seen, the contracts are backwards compatible (version 3 implements 3, 2 and 1) and there shouldn't be any problem. If a user is on a older build, the API call is still handled if it was implemented in that build. Take a look here at PackageCatalog , it implements all three interfaces of the different API versions. Each subsequent API interface has only the changes relative to the previous one.

  3. There is no specific need in a specific new API right now. But this commit does set the tendency of not depending on the UwpDesktop package for updates if we ever need it.

To summarize: the main change is between referencing a specific SDK version directly through UnionMetadata\{sdk_version}\Windows.winmd (Microsoft's recommended approach) versus using UwpDesktop for a specific version of the SDK to automatically add references to the project. I think it's important to explain what UwpDesktop does - it navigates through the SDK files and manually adds API references to the project based on what it found. Both approaches require that the developer has a specific version of the SDK installed (albeit in my personal opinion, using Windows.winmd does seem less messy while providing access to the same APIs). Both approaches will work on user machines with older builds as long as that specific API is supported. Again, it's not like Wox hugely relies on UWP right now, so this does not have a huge impact on functionality.

@bao-qian
Copy link
Member

@bao-qian bao-qian commented on d8beaee Dec 27, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Great explanation.

  1. I agree with you to remove uwpdesktop dependency now. plus the author moved to facebook
    https://www.linkedin.com/in/lucian-wischik/

  2. but from what I found, reference only Windows.winmd is not enough. I doubt referencing only Windows.winmd is Microsoft's recommended approach.

    2.1 sample you provided https://github.com/Microsoft/DesktopBridgeToUWP-Samples/blob/6e82c0a6d24aaa3d3ee9a1d708ec0b37463d007d/Samples/NorthwindSample/NorthwindCent/DesktopServer/DesktopServer.csproj#L52-L63

    2.2 uwpdesktop readme: To interop with DLLs or WINMDs built for Win8.1 or WindowsPhone8.1, the issue is that those old DLLs and WinMDs used to think that all WinRT types were in a file called "windows.winmd". But they're not anymore! https://github.com/ljw1004/uwp-desktop/blob/master/README.md

    2.3 a desktop bridge doc added this year Then, add a reference to these files. https://docs.microsoft.com/en-us/windows/uwp/porting/desktop-to-uwp-enhance#first-set-up-your-project

@lances101
Copy link
Member Author

@lances101 lances101 commented on d8beaee Dec 27, 2017 •
edited by bao-qian
Loading

Choose a reason for hiding this comment

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

As for 2 and 2.1: Microsoft themselves don't seem to stick to one approach.
The example you linked is the only one in that repo where they use the Facade version, in the rest of them its UnionMetadata\Windows.winmd directly. So its really hard to understand what is the right approach based on their own examples and contradictory documentation.

The desktop bridge doc seems to be the latest recommended approach by Microsoft, and although I still have my doubts, I do think that that seems like the way we should go.

Anyway, I've asked the same thing on StackOverflow (https://stackoverflow.com/questions/47999722/windows-10-sdk-desktop-to-uwp-bridging), just in case somebody who actually has any idea of why things are the way they are replies. I'll reply to this commit once there is an answer (if there is one that is).

If you feel that that desktop bridging guide is the way to go, just say so and I will revert this commit and push one with that.

@bao-qian
Copy link
Member

Choose a reason for hiding this comment

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

I found some new info.

  1. we need .net core version of System.Runtime.WindowsRuntime.dll because windows.winmd referencing the .net core version.

image

  1. the new uwp api has been separated into different contract. windows.winmd contains only old api. e.g.
    this https://docs.microsoft.com/en-us/uwp/api/windows.applicationmodel.packagecontentgroupstate can't be found in windows.winmd

  2. now come to the Facade. I guess if we want to referencing contract dll (desktop bridge doc version), we must use Facade\windows.winmd version to maintain backward compatibility in win8 instead of windows.winmd. As explained in https://github.com/ljw1004/uwp-desktop To interop with DLLs or WINMDs built for Win8.1 or WindowsPhone8.1

  3. could you do a test?

build1: ref Facade\windows.winmd and universal contract api directly
build2: ref windows.winmd and universal contract api directly
vm: new win8 vm without sdk installed

run1: copy build 1 without *.winmd files in wox directory to vm, will wox startup?
run2: if run1 failed, copy build 1 with *.winmd files in wox directory to vm, will wox startup?
run3: copy build 2 without *.winmd files in wox directory to vm, will wox startup?
run4: if run 3 failed, copy build 3 with *.winmd files in wox directory to vm, will wox startup?

@lances101
Copy link
Member Author

@lances101 lances101 commented on d8beaee Dec 29, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

  1. Yep, I checked that too, should've written about it. Good on you for checking it!
  2. Oh, it is in the normal Windows.winmd, but you need to have the 10.0.15063.0 SDK installed and reference the Windows.winmd at UnionMetadata\10.0.15063.0\Windows.winmd. Here it is:

2017-12-29 21 49 02

That's what I meant when I said that both approaches still require targeting a specific API version.
  1. and 4. Yea, I've read that too. That's why I want to make some tests. I will be travelling until January, 2nd, and for some unforeseen circumstances I lost remote access to my main dev machine. Once I am back I will run the tests and post the results (around the evening of January 2nd).

If I manage to gain access earlier, I will comment here.

@lances101
Copy link
Member Author

Choose a reason for hiding this comment

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

All tests were run on a fresh Windows 8.1 x64 install from MSDN ISO with English (US) locale via the Hyper-V manager.

System Type: x64-based PC
OS Name: Microsoft Windows 8.1
OS Version: 6.3.9600 N/A Build 9600
System Locale: en-us; English (United States)

Machine state is reverted to initial clean snapshot before each Wox run.

Windows 10 SDK version used - 10.0.16299.0
Same version was used for both Facade and "root" versions:

  • UnionMetadata\10.0.16299.0\Windows.winmd
  • UnionMetadata\10.0.16299.0\Facade\Windows.winmd

For both builds $(MSBuildProgramFiles32)\Reference Assemblies\Microsoft\Framework\.NETCore\v4.5\System.Runtime.WindowsRuntime.dll was also referenced.

Build 1 (Facade) also had the *.winmd files listed here added as a reference to the project.

Queries tested:

  • calc to invoke Calculator
  • sett to invoke Wox Settings

Test results

Run 1 (build 1, facade, no *.winmd files)

  • Wox starts fine, no crashes. UWP apps are not available (as usual).
  • Listed queries work.
  • Notable entries in log file: none. Only the usual errors when Shell.IShellLinkW.Resolve is trying to resolve Desktop.lnk and Everything Service not being found errors.

Run 3 (build 2, facade, without *.winmd files)

  • Wox starts fine, no crashes. UWP apps are not available (as usual).
  • Listed queries work.
  • Notable entries in log file: none. Only the usual errors when Shell.IShellLinkW.Resolve is trying to resolve Desktop.lnk and Everything Service not being found errors.

It seems like both approaches work fine with Windows 8.1 and do not cause any problems upon initialization. Programs plugin still works as expected (Calculator result was present).

Sadly, I do not see a difference. I tried to keep my environment as clean as possible and yet it does not seem like there is a difference as to what approach we go for.

@bao-qian
Copy link
Member

@bao-qian bao-qian commented on d8beaee Jan 9, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

sorry for late reply.

I just recall we don't support windows 8 application in win8.
https://github.com/Wox-launcher/Wox/blob/master/Plugins/Wox.Plugin.Program/Programs/UWP.cs#L145

let's keep your approach, but reference an sdk specific api, like
UnionMetadata\10.0.16299.0\Windows.winmd

it seems like they are backwards compatible.

thanks for you tremendous help in this discussion, I document this discussion in https://github.com/Wox-launcher/Wox/wiki/UWP-api in case we need it in the future.

@lances101
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's fine.

I will push the changes in the upcoming days and reference this commit.

Please sign in to comment.