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

v5: Update to latest winmd and land Dart 3 #697

Merged
merged 28 commits into from
May 10, 2023
Merged

v5: Update to latest winmd and land Dart 3 #697

merged 28 commits into from
May 10, 2023

Conversation

timsneath
Copy link
Contributor

This is a little complex to land, because of the cyclical dependency between win32gen and winmd, coupled with the breaking change (the latest Win32 metadata includes the COM classes that winmd depends on, so they've moved to win32).

Step 1: use git dependencies and publish_to: none to ensure that the pieces work together
Step 2: remove publish_to and replace git dependencies with actual versions
Step 3: publish both simultaneously
Step 4: ...
Step 5: profit :)

@timsneath
Copy link
Contributor Author

Well, I have no clue why this is failing with this error message. 😠 It works fine on my machine!

@timsneath timsneath changed the title Update to latest winmd v5: Update to latest winmd and land Dart 3 May 10, 2023
@halildurmus
Copy link
Owner

It appears that the interface IPropertySetSerializer is a recent addition, introduced in version 10.0.22000.0. However, as the test machines are running on older versions than 10.0.22000.0, the interface is not available to them.

We started encountering this error because the recently updated Windows.Win32.md file now includes a new namespace named Windows.Win32.System.WinRT.Metadata. Within this namespace, there is a function called RoCreatePropertySetSerializer which requires IPropertySetSerializer to be passed as a parameter:
Image

As a workaround, we could temporarily ignore the Windows.Win32.System.WinRT.Metadata.Apis in: https://github.com/dart-windows/win32/blob/db97d3c8e11236e40df12db840bd2bb7ec551a9c/tool/win32gen/test/input_test.dart#L19-L21

Once halildurmus/winmd#68 is resolved, we will no longer face such problems :)

@timsneath
Copy link
Contributor Author

timsneath commented May 10, 2023

Ah, that's brilliant. Thank you.

Although I don't think that PR will solve this: the aforementioned Windows.Win32.winmd came from NuGet!

@timsneath
Copy link
Contributor Author

I'll fix the test, merge this and publish this morning as v5 if you're Ok with that. We are nearly there!

@halildurmus
Copy link
Owner

halildurmus commented May 10, 2023

Although I don't think that PR will solve this: the aforementioned Windows.Win32.winmd came from NuGet!

Unless I'm missing something, wouldn't that PR resolve the issue because the winmd will use the bundled Windows.Win32.winmd file to read the metadata, making it accessible on all machines, regardless of their operating system version? I know that machines running on older versions cannot call these APIs, but they should be able to access the Metadata which should be enough for the failed test.

I'll fix the test, merge this and publish this morning as v5 if you're Ok with that. We are nearly there!

Sounds good! FYI, I plan to publish the windows_* packages on either Thursday or Friday as I want to write some more documentation for WinRT.

..where((typeDef) => typeDef.name.endsWith('Apis'))
// Temporarily disable because of:
// https://github.com/dart-windows/win32/pull/697#issuecomment-1541500049
..where((typeDef) =>
!typeDef.name.startsWith('Windows.Win32.System.WinRT.Metadata.Apis'));
..where((typeDef) => typeDef.name.endsWith('Apis'));
Copy link
Owner

@halildurmus halildurmus May 10, 2023

Choose a reason for hiding this comment

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

@timsneath I think ..where should be .where :). Pretty sure that's the reason it didn't work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doh. OK, I've disabled this test for right now so I can land this, but I'll re-enable it when we have a green build.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, ready for your approval @halildurmus!

Copy link
Owner

@halildurmus halildurmus May 10, 2023

Choose a reason for hiding this comment

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

Looks like GitHub is failing us this time :)

Edit: We seem to have a failing test as well :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, because it looks like there are other parts of metadata that depend on other classes in this namespace. I'm going to disable this test for GHA.

Copy link
Owner

@halildurmus halildurmus May 10, 2023

Choose a reason for hiding this comment

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

MetadataGetDispenser function is from the ignored namespace Windows.Win32.System.WinRT.Metadata.Apis.

Ignoring it should fix it (hopefully):

    ...
    // Gather metadata for all the functions in the JSON file
    for (final function in functionsToGenerate.values) {
      if (function.functionSymbol == 'MetaDataGetDispenser') continue;
      final method = methods.where((m) => m.name == function.functionSymbol);
      if (method.length != 1) {
        fail('${function.functionSymbol} did not match a unique item in '
            'the metadata. There were ${method.length} matching items.');
      }
    }

Edit: On second thought, there are probably more functions like this.

@timsneath
Copy link
Contributor Author

OK, just pulling this test. It's too painful to debug this right now, and I want to get this out. This is just a test flake on Windows 10 machines.

Copy link
Owner

@halildurmus halildurmus left a comment

Choose a reason for hiding this comment

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

LGTM

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.

None yet

2 participants