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

Add Converters property to directory props to build converters from source for sample app #152

Merged
merged 4 commits into from
Aug 2, 2023

Conversation

michael-hawker
Copy link
Member

Separating out from #138

Follow-on from CommunityToolkit/Tooling-Windows-Submodule#99

138 isn't building in linux for some reason related to converters, but everything works fine locally. Figured would try splitting this out first to see if that changes anything.

@michael-hawker
Copy link
Member Author

@Arlodotexe I think the crux of the errors is this one maybe?

/home/runner/.nuget/packages/msbuild.sdk.extras/3.0.23/Build/Workarounds.targets(27,5): error : If you are building projects that require targets from full MSBuild or MSBuildFrameworkToolsPath, you need to use desktop msbuild ('msbuild.exe') instead of 'dotnet build' or 'dotnet msbuild' [/home/runner/work/Windows/Windows/components/Converters/src/CommunityToolkit.WinUI.Converters.csproj::TargetFramework=uap10.0.17763]

I think it's getting tripped up by a uap target, but I don't understand why this is different than anything else we've done. It has to be something with the reference to the Converters project directly by the sample app? And this causing an issue on Uno/Linux with the build?

@michael-hawker
Copy link
Member Author

After installing the .NET 7 SDK in Codespaces (see #153), I can repro the build issue trying to run the sample app in Codespaces. Looks like the same message.

@michael-hawker
Copy link
Member Author

I tried to limit the TargetFrameworks using ./tooling/MultiTarget/UseTargetFrameworks.ps1 -targets wasm (which modified the targetframeworks.props files, but the issue still persist with the same errors... not sure why it'd still be trying to build uap target at that point for the converters package...

@Arlodotexe
Copy link
Member

Thanks @michael-hawker, found and fixed an issue with the UseTargetFramework.ps1 script in CommunityToolkit/Tooling-Windows-Submodule#109

We'll need this tooling fix, and we'll need to adjust our CI in this repo to run ./tooling/MultiTarget/UseTargetFrameworks.ps1 -targets wasm as you pointed out.

When we've resolved that, we'll hit this error instead:

CSC : error UXAML0001: The type {using:CommunityToolkit.WinUI.Converters}DoubleToVisibilityConverter could not be found [/workspaces/Windows/tooling/ProjectHeads/AllComponents/Wasm/CommunityToolkit.App.Wasm.csproj]
CSC : error UXAML0001: Processing failed for file /workspaces/Windows/tooling/CommunityToolkit.App.Shared/Renderers/ToolkitDocumentationRenderer.xaml (System.InvalidOperationException: The type {using:CommunityToolkit.WinUI.Converters}DoubleToVisibilityConverter could not be found [/workspaces/Windows/tooling/ProjectHeads/AllComponents/Wasm/CommunityToolkit.App.Wasm.csproj]

@michael-hawker
Copy link
Member Author

michael-hawker commented Jul 19, 2023

@Arlodotexe we don't do the TargetFrameworks ps1 call in the tooling submodule repo (where it builds fine), do we think we should be doing that here? (will try it though)

@michael-hawker
Copy link
Member Author

michael-hawker commented Jul 19, 2023

Edit: NVM - Apparently submodule wasn't updated all the way for some reason, repushed...

@michael-hawker
Copy link
Member Author

Looks like it's outputting something for the converters here:
https://github.com/CommunityToolkit/Windows/actions/runs/5603161081/jobs/10249385004?pr=152#step:9:397

CommunityToolkit.WinUI.Converters -> /home/runner/work/Windows/Windows/components/Converters/src/bin/Debug/netstandard2.0/CommunityToolkit.WinUI.Converters.dll

So they should be building. But not sure why it can't find the converter in there.

@michael-hawker
Copy link
Member Author

Rolling back a tiny bit on the submodule pointer to not include the new titlebar stuff, as that's adding even more issues I think?

@Arlodotexe
Copy link
Member

Found and fixed this one issue: 039638c

Though I'm still not sure why it's failing to build the samples 🤔. Removing the xamarinmac20 tfm somehow solves the problem completely, but it's still unclear what exactly the problem is and how it was introduced.

@Arlodotexe
Copy link
Member

Arlodotexe commented Jul 24, 2023

Seeing this in the logs:

CSC : error UXAML0001: The type {using:CommunityToolkit.WinUI.Converters}DoubleToVisibilityConverter could not be found [/home/runner/work/Windows/Windows/tooling/ProjectHeads/AllComponents/Wasm/CommunityToolkit.App.Wasm.csproj]

That shouldn't be affecting our ability to build individual sample projects, though. Something going on with the dependency?

@Arlodotexe
Copy link
Member

Arlodotexe commented Jul 25, 2023

Did a binary search on the git history to find where this started. The issue would have been showing earlier if not for the bug we eventually fixed in this PR.

The tfms just weren't getting enabled in CI because of this bug. I fixed it that commit, then it uncovered this issue we're having. I continued rewinding git in a binary search and manually applied the fix over it as I went to find where the problem was introduced.

This issue stops if you rewind before this PR and manually apply the bugfix linked above, meaning that PR is the cause of the issue.

From that PR:

Looks like this was originally added here to resolve MSB4011 warnings
A short time after we found out this was a bug in Roslyn, which appears to be resolved now. Let's get this cleaned up. Thanks @Sergio0694!

Well... the Roslyn ticket I linked was for .NET Core. We're still on xamarin for MacOS. We have a NET 7 branch ready to go, but we aren't ready to commit to dropping Xamarin just yet (maybe soon?).

For now, we'll just remove this ProjectReference on MacOS.

@michael-hawker
Copy link
Member Author

Rebased on main and pulled up the submodule to main there as well, though it does pull in the extra titlebar stuff, we'll see what this does...

@michael-hawker
Copy link
Member Author

Will update and see what happens after #160 has cleaned-up the tooling submodule

@michael-hawker
Copy link
Member Author

@Arlodotexe updated on top of #160, just has the change to the build to only build for wasm on linux and then the update for the converters package reference for #138

@michael-hawker
Copy link
Member Author

@Arlodotexe I think the crux of the errors is this one maybe?

/home/runner/.nuget/packages/msbuild.sdk.extras/3.0.23/Build/Workarounds.targets(27,5): error : If you are building projects that require targets from full MSBuild or MSBuildFrameworkToolsPath, you need to use desktop msbuild ('msbuild.exe') instead of 'dotnet build' or 'dotnet msbuild' [/home/runner/work/Windows/Windows/components/Converters/src/CommunityToolkit.WinUI.Converters.csproj::TargetFramework=uap10.0.17763]

I think it's getting tripped up by a uap target, but I don't understand why this is different than anything else we've done. It has to be something with the reference to the Converters project directly by the sample app? And this causing an issue on Uno/Linux with the build?

Right without the TFM change we were getting this original error:

/home/runner/.nuget/packages/msbuild.sdk.extras/3.0.23/Build/Workarounds.targets(27,5): error : If you are building projects that require targets from full MSBuild or MSBuildFrameworkToolsPath, you need to use desktop msbuild ('msbuild.exe') instead of 'dotnet build' or 'dotnet msbuild' [/home/runner/work/Windows/Windows/components/Converters/src/CommunityToolkit.WinUI.Converters.csproj::TargetFramework=uap10.0.17763]
/home/runner/.nuget/packages/microsoft.windowsappsdk/1.3.230331000/buildTransitive/MrtCore.PriGen.targets(911,5): error MSB4062: The "Microsoft.Build.Packaging.Pri.Tasks.ExpandPriContent" task could not be loaded from the assembly /usr/share/dotnet/sdk/7.0.306/Microsoft/VisualStudio/v17.0/AppxPackage/Microsoft.Build.Packaging.Pri.Tasks.dll. Could not load file or assembly '/usr/share/dotnet/sdk/7.0.306/Microsoft/VisualStudio/v17.0/AppxPackage/Microsoft.Build.Packaging.Pri.Tasks.dll'. The system cannot find the file specified. [/home/runner/work/Windows/Windows/components/Converters/src/CommunityToolkit.WinUI.Converters.csproj::TargetFramework=net6.0-windows10.0.19041.0]
/home/runner/.nuget/packages/microsoft.windowsappsdk/1.3.230331000/buildTransitive/MrtCore.PriGen.targets(911,5): error MSB4062:  Confirm that the <UsingTask> declaration is correct, that the assembly and all its dependencies are available, and that the task contains a public class that implements Microsoft.Build.Framework.ITask. [/home/runner/work/Windows/Windows/components/Converters/src/CommunityToolkit.WinUI.Converters.csproj::TargetFramework=net6.0-windows10.0.19041.0]

@michael-hawker
Copy link
Member Author

@nickrandolph said we should be on MSBuild.Sdk.Extras 3.0.44 so trying an update to that as well.

@michael-hawker
Copy link
Member Author

Not sure why GitHub isn't picking up the latest commit to run suddenly... going to try closing and re-opening

@michael-hawker
Copy link
Member Author

We should merge in CommunityToolkit/Tooling-Windows-Submodule#118 and then update this pointer for that before merging just to keep things clean, but otherwise we should be all set now... At least we've confirmed exactly what the issue was and that it's an issue in the underlying dotnet tooling on linux.

@michael-hawker
Copy link
Member Author

Cleaned-up this PR, tooling pointing to main with fix from 118, so we should be good to go here. FYI @Arlodotexe

@michael-hawker
Copy link
Member Author

Didn't add explicit comment here, but end reason for failure on linux was duplicated ProjectReference between app head and the generated includes, this worked fine on windows, but there appears to be bug with that scenario on linux (or at least Ubuntu) with the tooling, see: dotnet/msbuild#2688

@michael-hawker michael-hawker enabled auto-merge (rebase) August 2, 2023 21:05
@michael-hawker michael-hawker merged commit cbea35f into main Aug 2, 2023
@delete-merged-branch delete-merged-branch bot deleted the llama/converters-fix branch August 2, 2023 21:11
michael-hawker added a commit that referenced this pull request Aug 4, 2023
Helps with UWP project heads and old-project system now for some reason
after single-experiment heads broke with #152 #164
michael-hawker added a commit that referenced this pull request Aug 8, 2023
* Remove double-slashes from Project Paths

Helps with UWP project heads and old-project system now for some reason
after single-experiment heads broke with #152 #164

* Ensure RepositoryDirectory ends in Slash

* Don't double-add extensions source reference to test heads
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants