Skip to content

Conversation

@chrisfromwork
Copy link
Contributor

Currently only managed plugins are referenced in MSBuildForUnity project generation. In some instances, we also need to include winmd references. This change introduces the concept of winmds and adds winmd references to MSBuildForUnity generated projects.

#93 is currently open and addresses all changes before 137fd4d "add winmd support"

@andreiborodin
Copy link
Contributor

This is an awesome change.

I would like to understand a couple of things in terms of Unity behavior regarding WinMD:

  • What happens when we say WinMD supports Android, and build for Android?
  • What happens when we say WinMD references a placeholder DLL, that DLL is set to support Android player, and try to build it for Android player?
  • What happens when we say WinMD references a placeholder DLL, both the DLL and WinMD support WSA player, we export the WSA solution and try building it?

We should mimick the Unity behaviour here with our setup.

Copy link
Contributor

@andreiborodin andreiborodin left a comment

Choose a reason for hiding this comment

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

Looked at the code, some comments. Need to wait on the answers to the Unity behaviour questions.

@chrisfromwork
Copy link
Contributor Author

So to start off, you can hit editor crashes if you have both the placeholder dll and winmd enabled for the editor and switch from the standalone player to the WSA player. It looks like this is a Unity exception compared to a MSBuildForUnity exception.

What happens when we say WinMD supports Android, and build for Android?

This generates an error when exporting/building stating that the "dll is not allowed to be included or could not be found"

What happens when we say WinMD references a placeholder DLL, that DLL is set to support Android player, and try to build it for Android player?
What happens when we say WinMD references a placeholder DLL, both the DLL and WinMD support WSA player, we export the WSA solution and try building it?

This may have just been quick/strange wording, but WinMD's don't reference placeholder dll's. WinMDs and placeholder dlls define the same types for the WSA vs other players.

If you include both a winmd reference and a placeholder reference for the WSA player an error is generated based on the colliding types.

If you include just a placeholder dll reference for Android, things successfully build. This should be fine because the placeholder dll is callable no-op .NET dll. Folks may get unexpected behavior at runtime but they have fundamentally setup their code in a strange manner to use WSA types for non-WSA unity players.

If you include a winmd and placeholder dll reference for Android you get the same error for including a winmd reference.

@andreiborodin
Copy link
Contributor

Thanks for looking into this. About specifying placeholder dlls, see this image:
image


foreach (string assetAssemblyPath in Directory.GetFiles(Utilities.AssetPath, "*.dll", SearchOption.AllDirectories))
IEnumerable<string> assetReferences = Directory.GetFiles(Utilities.AssetPath, "*.*", SearchOption.AllDirectories)
.Where(file => file.ToLower().EndsWith(".dll") || file.ToLower().EndsWith(".winmd"));
Copy link
Contributor

Choose a reason for hiding this comment

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

file.ToLower() [](start = 31, length = 14)

Could do .Select(t=>t.ToLower()).Where(t=> t.EndsWith() || ...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if you go this route you end up with all lower case file names which can cause issues with finding the asset relative path

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, yes, my bad. Was thinking of how best to cache. Could you pass StringCompareOptions to EndsWith?

@chrisfromwork
Copy link
Contributor Author

I've added more robust error logging and winmd reference exclusion when things are configured incorrectly.

Debug.LogError($"WinMDs should only be enabled for the WSA Player; however, {ReferencePath} was configured to support {platform.Key}.");
valid = false;
break;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to correct this by just saying "we will only use it for UWP?" and logging an error, instead of just failing to use this as dependency?

@andreiborodin andreiborodin merged commit 19f147d into microsoft:master Jan 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants