Skip to content

Replace winapi and webview2 crates with bindings from windows-rs#133

Merged
wusyong merged 16 commits intotauri-apps:devfrom
wravery:windows-rs
Apr 1, 2021
Merged

Replace winapi and webview2 crates with bindings from windows-rs#133
wusyong merged 16 commits intotauri-apps:devfrom
wravery:windows-rs

Conversation

@wravery
Copy link
Copy Markdown
Contributor

@wravery wravery commented Mar 29, 2021

This is a follow-up to microsoft/windows-rs#538. I'm attempting to replace the winapi and webview2 crates on Windows with windows-rs in WRY. Generating the bindings for both from windows-rs makes them a little more consistent, and I think overall they're easier and safer to use.

I tested all of the examples in this repo, and they all seem to work fully (aside from transparent which does not seem to be supported on Windows yet). The dragndrop was definitely the hardest to get working, because I needed to get the COM v-table support for DropTarget to work with the windows-rs generated code. There's WIP on windows-rs to make implementing COM objects easier in the future (microsoft/windows-rs#81), and there's an experimental API on WebView2 which won't even require implementing IDragDrop once it is stabilized. So when either of those is complete, all of that COM interop code in file_drop.rs can go.

I've automated pulling down a new version of the WebView2 NuGet package as part of the bindings/build.rs script. My goal is to be able to just change the version string in bindings/build.rs and re-run cargo to be able to rev the version of WebView2. No more manually placing files, windows-rs will pull the DLL for the target architecture from the .windows directory as part of the build and copy it to the build directory.

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature
  • Code style update
  • Refactor
  • Documentation
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change? (check one)

  • Yes. Issue #___
  • No

The PR fulfills these requirements:

  • When resolving a specific issue, it's referenced in the PR's title (e.g. fix: #xxx[,#xxx], where "xxx" is the issue number)
  • A change file is added if any packages will require a version bump due to this PR per the instructions in the readme.

If adding a new feature, the PR's description includes:

  • A convincing reason for adding this feature (to avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it)

Other information:

This switches from using the released Win32 APIs for WebView2 to the still pre-release WinRT APIs for WebView2. But the tradeoff for using the pre-release APIs is that windows-rs is able to generate more complete bindings that are not marked as unsafe.

@lucasfernog
Copy link
Copy Markdown
Member

Impressive 😲

@nothingismagick
Copy link
Copy Markdown
Member

nothingismagick commented Mar 29, 2021

Really interesting work @wravery

This is another one of those reasons why I think we should really consider having a configurable bleeding edge approach that we can recommend to people. In rust its easy for us to have a branch structure like:

  • main (this is the canonical version available as a release and at crates.io)
  • next (experimental stuff that is shown to mostly work)
  • dev (where we actually work on things and break them)

@jbolda - how hard would it be for us to have 2 workflows that track dev and upon merging either push to next or main respectively?

Then there is the documentation about how to actually use wry "normally" via crates.io and then for those on the bleeding edge via git revisions. Obviously we will have to have something similar for tauri downstream. @Laegel - what do you think?

@nothingismagick
Copy link
Copy Markdown
Member

FYI - we have forked winit and published it to crates.io until they release a new version:

wry/Cargo.toml

Line 49 in 531f2a7

tauri-winit = "0.24"

@wravery wravery marked this pull request as ready for review March 29, 2021 16:51
@wravery wravery requested a review from a team as a code owner March 29, 2021 16:51
@wravery wravery requested a review from a team March 29, 2021 16:51
Comment thread src/webview/win/mod.rs
@wravery
Copy link
Copy Markdown
Contributor Author

wravery commented Mar 29, 2021

I was able to snap to the windows-rs 0.7.0 release which just came out earlier today. 🎉 So no more pre-release commit ID in Cargo.toml for that (or reason to fork like winit) pending another release.

@jbolda
Copy link
Copy Markdown
Member

jbolda commented Mar 29, 2021

Really interesting work @wravery

This is another one of those reasons why I think we should really consider having a configurable bleeding edge approach that we can recommend to people. In rust its easy for us to have a branch structure like:

  • main (this is the canonical version available as a release and at crates.io)
  • next (experimental stuff that is shown to mostly work)
  • dev (where we actually work on things and break them)

@jbolda - how hard would it be for us to have 2 workflows that track dev and upon merging either push to next or main respectively?

Then there is the documentation about how to actually use wry "normally" via crates.io and then for those on the bleeding edge via git revisions. Obviously we will have to have something similar for tauri downstream. @Laegel - what do you think?

I wonder if bleeding edge might be better as a feature flag? Long running experimental features end up being difficult to decide when to move those into the main publish flow, and then they never end up "finishing" (at least from what I have seen). I think it would be helpful to somehow have prerelease versions published on the Version Packages PR, but it is surprisingly difficult to get it working in a cross-language way without relying on the filesystem at all.

Copy link
Copy Markdown
Member

@wusyong wusyong left a comment

Choose a reason for hiding this comment

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

@wravery Thanks for the PR! You are doing god's work! Just some nits and I think it's good to merge.

@jbolda @nothingismagick Since we are pushing beta on tauri, and the only thing left to be published is #92.
I guess we can publish another version first and then merge this PR.
We will have enough time to settle down windows-rs as dependency.

Comment thread bindings/build.rs Outdated
Comment thread .gitignore Outdated
Comment thread bindings/build.rs
use std::{convert::From, env, fs, io, path::PathBuf, process::Command};

const WEBVIEW2_NAME: &str = "Microsoft.Web.WebView2";
const WEBVIEW2_VERSION: &str = "1.0.824-prerelease";
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks like the library structure is different from the previous stable release (1.0.774.44).
Will the future stable release be the same as the current one?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I don't know for sure, but I expect that when the WinRT APIs are released that they'll still use this structure. The Win32 files seem to be in the same place between versions, and some of this structure is deterministic for interop with other tools and Visual Studio.

This does mean using the prerelease packages until the WinRT APIs are released, though.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I see. It should be fine we use the prerelease package.

Copy link
Copy Markdown
Member

@nothingismagick nothingismagick Mar 30, 2021

Choose a reason for hiding this comment

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

Do we have to ship the dlls forever? Or just until the current prerelease becomes the evergreen stable? Because I really don't want to ship that by default forever

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

So I think the prerelease packages are a superset of what will eventually show up in the stable/release packages. I expect the paths/structure of the WinRT APIs to match at that point.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Do we have to ship the dlls forever? Or just until the current prerelease becomes the evergreen stable? Because I really don't want to ship that by default forever

These take the place of WebView2Loader.dll in the Win32 APIs, which has a corresponding WebView2Loader.dll.lib static import lib. There's also a WebView2LoaderStatic.lib which would statically link them into the binary with no need for a separate DLL. Whichever mechanism it used, it was probably coming from the webview2 crate. At some point, one or both of those files had to be included in the crates, upstream and/or downstream from wry to enable the static link step and potentially to include a DLL.

The WinRT API doesn't seem to have the option of linking a static implementation though, so this PR may be introducing the requirement to ship the DLLs with the package. I'll double check what webview2 was doing here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It looks like webview2's behavior depends on the toolchain: https://docs.rs/webview2/0.1.0-beta.1/webview2/#webview2loader. For gnu it dynamically links and you need to include the DLL, for msvc it statically links.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ok - the real reason why i am asking is because we absolutely do not want to be in charge of shipping standalone webviews. They must be managed by the OS. I am particularly concerned about the licensing implications of doing this. I am also worried about bundle size just exploding too...

MicrosoftEdge/WebView2Feedback#907

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ok - the real reason why i am asking is because we absolutely do not want to be in charge of shipping standalone webviews. They must be managed by the OS. I am particularly concerned about the licensing implications of doing this.

Ah, I see. Here's the license for that: https://www.nuget.org/packages/Microsoft.Web.WebView2/1.0.824-prerelease/License. I don't see a conflict with MIT, but it's not exactly MIT so the license text would need to be included with the binaries. Perhaps instead of including the contents in .windows in the repo, the build.rs should just always dynamically place them there, and it ought to copy/rename the LICENSE.txt file from the NuGet package with them to the target directory so it can be included with the binaries in a downstream consumer.

I am also worried about bundle size just exploding too...

This is not the WebView2 runtime itself, just the client libraries which know how to talk to it (or to a matching browser installation). The same (separately installed) runtime requirements as the webview2 crate apply: https://docs.rs/webview2/0.1.0-beta.1/webview2/#runtime. Plus if I do switch to always building the .windows directory dynamically, there should not be much change in binary size.

@wravery
Copy link
Copy Markdown
Contributor Author

wravery commented Mar 30, 2021

Since we are pushing beta on tauri, and the only thing left to be published is #92.

No rush on my end, I don't want to derail any release plans. I'll be just as happy if this goes in to a dev branch later.

@wravery
Copy link
Copy Markdown
Contributor Author

wravery commented Mar 30, 2021

I'll be just as happy if this goes in to a dev branch later.

D'oh! I just realized, a side-effect of switching to the WinRT APIs for WebView2 is that it would probably cut off support for Windows 7 and Windows 8. 😞

Between that and the other concerns about licensing and redistributing binaries, I think this should definitely be considered bleeding edge. It's promising as a way to simplify the dependency on WebView2, but there are a lot of caveats to depending on it as the only way to do that.

@nothingismagick
Copy link
Copy Markdown
Member

@wravery

Between that and the other concerns about licensing and redistributing binaries, I think this should definitely be considered bleeding edge. It's promising as a way to simplify the dependency on WebView2, but there are a lot of caveats to depending on it as the only way to do that.

Yeah, there is one other pretty well camouflaged elephant in the room, and that is our desire to work together with the PureOS ecosystem and enable "official" applications to ship to their app store. From a high level, that will require us mirroring tauri & wry source code on their gitlab - and then we have to comply with the letter and intent of strict copyleft. This includes license headers in every file and absolute clarity to the license and provenance of all "first-party" code. If you like, please feel free to start a discussion on our discord in the #wry channel so we can have a bit more of a real-time chat about this.

Copy link
Copy Markdown
Member

@wusyong wusyong left a comment

Choose a reason for hiding this comment

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

Published a new patch version and main branch to track it.
I think it's safe to merge this to dev branch.
We will have plenty amount of time to settle down until next version.

@wravery Thanks again for the help! I will be sure to visit window-rs, if there are other issues in the future.

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.

5 participants