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

Package id as assembly name #404

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Conversation

Arlodotexe
Copy link
Member

This PR updates our tooling to use the PackageId as the AssemblyName for components.

It required changes in both Tooling and any components with a custom PackageId due to the single-import. Hopefully this will get cleaner when we have nested components (CommunityToolkit/Tooling-Windows-Submodule#187).

Note that any components using a custom PackageId will need to be updated to define their PackageId before importing ToolkitComponent.SourceProject.props, which requires importing the PackageIdVariant prop from MultiTarget\WinUI.TargetVersion.props first.

Prerequisite PR CommunityToolkit/Tooling-Windows-Submodule#188

@Arlodotexe Arlodotexe self-assigned this Apr 18, 2024
@Arlodotexe
Copy link
Member Author

Arlodotexe commented Apr 18, 2024

Strange, looks like this change caused new nullability warnings to appear. Investigating.

image

@Arlodotexe
Copy link
Member Author

@michael-hawker Looks like the issue goes away if you use ./tooling/MultiTarget/UseUnoWinUI.ps1 3 to switch to WinUI 3, which means the internal helper is going missing when the assembly name doesn't match the root namespace.

@michael-hawker
Copy link
Member

@Arlodotexe was just thinking, is this an internal helper from another assembly or something? Did we miss an 'InternalsVisibleTo' somewhere with this change?

@Arlodotexe
Copy link
Member Author

Arlodotexe commented May 2, 2024

This is an internal helper, though I'm not sure where the helper is coming from. We need to find that before we can continue.

@michael-hawker
Copy link
Member

It looks from a search, that the helper is here (or in this file):

public static void ThrowIfNull(this ArgumentNullException? _, [NotNull] object? argument, [CallerArgumentExpression(nameof(argument))] string? parameterName = null)

And shared with the animation package here:

<!-- TODO: We should figure out a better way to share internal helpers without duplication -->
<!-- e.g. Animations is using ThrowIfNull helper... -->
<InternalsVisibleTo Include="CommunityToolkit.WinUI.Animations" />
<InternalsVisibleTo Include="CommunityToolkit.WinUI.Media" />

According to the docs on InternalsVisibleTo here. this sounds like it based on assembly vs. namespace, so we probably need to have a flag/choose block here to flip?

@michael-hawker
Copy link
Member

Oddly looks like the same messages still, hmmm...

@Arlodotexe Arlodotexe added the blocked Blocked label May 17, 2024
@Arlodotexe
Copy link
Member Author

Arlodotexe commented May 17, 2024

I've been trying to pinpoint why these errors are still showing.

Local repro setup
Since the CI / build (WinUI2) job runs more than one TFM at a time, I opted to test Wasm since that job was also failing, but it only builds wasm.

Unfortunately, I've been unable to reproduce this error message locally building Wasm. I've tried variations of our build commands:

.\tooling\Build-Toolkit-Gallery.ps1 -Head wasm -MultiTargets wasm -Components All -Release -WinUIMajorVersion 2

and

.\tooling\Build-Toolkit-Gallery.ps1 -Head wasm -MultiTargets wasm -Components All -Release -WinUIMajorVersion 3

as well as just invoking msbuild directly, adding what's present in our build.yml.

What's happening?

My hunch is that this is still a misconfiguration with our tooling, as we've confirmed this is declared in an internal helper and not a polyfill or platform API. Checking the binlogs, the fixes in 6b54da1 seem to be working as intended, so it's not that. Until we can repro the issue locally, what's happening here is a mystery.

A note on build times

The build time for local wasm builds has been climbing as well. It can take 15-25 minutes for a wasm release build to fully complete. However, the CI is erroring in only 2 minutes. I've been running it to completion for posterity, but for local builds we might be able to cancel the build and assume no repro after at least 5 minutes of build time.

@Arlodotexe
Copy link
Member Author

Possible solution: #441

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked Blocked
Projects
Status: 🛑 Blocked
Development

Successfully merging this pull request may close these issues.

2 participants