Find winmd files without reading PATH nor copying to target/

[MarijnS95]

Fixes #979

Non-Windows platforms - which are supported for cross-compiling - do not set the output directory in PATH nor use semicolons to separate this variable, resulting in errors Split out winmd "discovery" by emitting the path to these files in a build step, that is subsequently compiled into windows_gen. This is more efficient than include_bytes! which was used prior to the PATH system as no copies or binary includes are required at all. Copying of DLL targets to the target/ dir for easy consumption and running of crates is still performed, but only on Windows for the same PATH reason.

In the future, if a chain of crates is required to export winmd files for downstream crates to consume, this can be extended to also export their $CARGO_MANIFEST_DIR/.winmd/windows in such a way that the next crate can pick it up, again without copying any files around.

TODO

@riverar Linked to the copy function on an older commit, but I think he intended to link to the way the target directory for those copies was derived:

windows-rs/crates/macros/src/lib.rs

Line 121 in 49ecfbc

let mut destination : ::std::path::PathBuf = ::std::env::var("OUT_DIR").expect("No `OUT_DIR` env variable set").into();

Should be migrate back to this and drop any and all PATH handling entirely?

[riverar]

My link was just meant to show how we copy artifacts out of the .windows subfolders. No PATH handling should remain.

[MarijnS95]

@riverar I interpreted it as pointing to a specific commit, conveniently right before PATH handling was introduced. Will remove!

[MarijnS95]

windows-rs/crates/macros/src/lib.rs

Lines 121 to 132 in 49ecfbc

	let mut destination : ::std::path::PathBuf = ::std::env::var("OUT_DIR").expect("No `OUT_DIR` env variable set").into();
	loop {
	destination.pop();
	destination.push("Cargo.toml");
	if destination.exists() {
	break;
	}
	destination.pop();
	}

I don't understand this loop. That's wrong if someone builds with their target folder ( CARGO_TARGET_DIR) in a different location.

[kennykerr]

Yep, that loop should not be needed anymore. It was an attempt to find dirs across crates which we no longer need.

[MarijnS95]

Yep, that loop should not be needed anymore. It was an attempt to find dirs across crates which we no longer need.

It's gone now, let me know if this is okay. I'm happy because this has now unblocked building on Linux (I'm not cross-compiling at all 😳), but it should work for your Windows (and rustdoc) purposes as well.

[kennykerr]

@MarijnS95 🤣 I've actually had some internal interest for cross-platform support, so you never know.

[MarijnS95]

@MarijnS95 I've actually had some internal interest for cross-platform support, so you never know.

Just shoot and I'll throw the cross-platform working DirectXShaderCompiler example back online 😁

Joking aside, really glad to hear this.

[riverar]

Looks OK to me. I defer to @kennykerr due to high impact. 😂

[kennykerr]

Haven't had a chance to dig in, but a quick smoke testing compiling https://github.com/microsoft/windows-app-rs fails unfortunately.

[kennykerr]

For a change as invasive as this, it may also help to smoke out issues by building https://github.com/microsoft/windows-samples-rs

[MarijnS95]

@kennykerr Can you clarify what exact issue you're seeing? For windows-app-rs, after fixing capitalization issues in cargo.toml, I'm left with the following:

error: `Microsoft.UI.Xaml.Application` not extendable
  --> src/main.rs:27:41
   |
27 | #[implement(extend Microsoft::UI::Xaml::Application, override OnLaunched)]
   |          

Is this a result of building with the wrong metadata?

Same for the samples; what errors should I be looking out for? Do I need to be on Windows to test specific things (need to revive a Windows install first that decided to corrupt itself for no apparent reason)?

[kennykerr]

Yes, that's the error I saw. I think its misleading because the type is not actually found in metadata at all and because its not found its not extendable.

[MarijnS95]

Yes, that's the error I saw. I think its misleading because the type is not actually found in metadata at all and because its not found its not extendable.

And that error isn't present when compiling against windows-rs master? This is hard to test for me currently due to the original "Path doesn't contain ;" errors.

[kennykerr]

Yes that error is not present in master.

[MarijnS95]

@kennykerr It's pretty obvious what happens: bindings/ has the .windows/winmd directory used to generate bindings, but #[implement] in the root crate needs to use these files as well. We have various ways to solve this including ln -s .windows/winmd from the crate root, or "exporting" the winmd path from bindings as suggested in #979 (comment) and pointing windows-rs to that one way or another, so that it uses the winmds directed by the bindings crate.

Let's discuss before jumping on an implementation detail.

(EDIT: This requirement is pretty much your "scenario 3" we were trying to avoid 😛)

[riverar]

ln -s .windows/winmd from the crate root

Tip: Symlinks in Windows require administrative privileges in normal scenarios, so that's out.

[MarijnS95]

@riverar I'm unfortunately aware, and it nauseates me. IMO this should be fully solved from Rust, no messing around with paths and links/copies by the user.

[kennykerr]

(EDIT: This requirement is pretty much your "scenario 3" we were trying to avoid 😛)

This worked before because the .windows folder was relative to the workspace rather than the crate so both crates shared the same .windows/winmd folder.

[kennykerr]

Basically, the TypeReader should

1. look in <workspace>/.windows/winmd and
2. add the gen crate's default winmd files if the previous step didn't have any windows.* winmd files.

[MarijnS95]

This worked before because the .windows folder was relative to the workspace rather than the crate so both crates shared the same .windows/winmd folder.

"Before" referring to the situation before, PATH changes, correct? Now on master it works because of unregulated copies to the target folder.

Basically, the TypeReader should

1. look in <workspace>/.windows/winmd and
2. add the gen crate's default winmd files if the previous step didn't have any windows.* winmd files.

That's what it's doing already, but not relative to <workspace> but the <crate> (CARGO_MANIFEST_DIR), as you said in your previous comment. This <crate> changes between building a "toplevel" package and its bindings crate.

Should we use cargo metadata to point windows-rs to a different directory, so that we can just parse $CARGO_MANIFEST_DIR/Cargo.toml?

[kennykerr]

Yes, that's the way it worked before the binary packaging support was added.

[kennykerr]

I need to get this wrapped up. It sounds like you still have some open questions about how to proceed. Do you mind terribly if I just take this over and get it done. I'd like to get this fixed up and publish a new version of the crate.

[kennykerr]

Closing in favor of #1067

[MarijnS95]

It sounds like you still have some open questions about how to proceed. Do you mind terribly if I just take this over and get it done. I'd like to get this fixed up and publish a new version of the crate.

I don't have time for this back-and-forth so feel free. Looks like #1067 is a simple "yes" to my question in #1047 (comment), but in code form.

[MarijnS95]

@kennykerr Do you mind awfully if I rebase this branch and open a new PR after yours is merged, so that we at least keep some cleanups/simplifications?

[kennykerr]

That would be great.