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

wap packaging: .xbf and .pri generation is not working for us #1816

Closed
DHowett-MSFT opened this issue Jul 4, 2019 · 13 comments · Fixed by #1900
Closed

wap packaging: .xbf and .pri generation is not working for us #1816

DHowett-MSFT opened this issue Jul 4, 2019 · 13 comments · Fixed by #1900
Assignees
Labels
Area-Build Issues pertaining to the build system, CI, infrastructure, meta Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. Severity-Blocking We won't ship a release like this! No-siree.
Milestone

Comments

@DHowett-MSFT
Copy link
Contributor

The WAP packaging project, CascadiaPackage, consumes most of its important dependencies transitively. It depends on WindowsTerminal and Host.EXE.

By default, wapproj puts all files collected from a project into a subdirectory named after that project, even if those files were originally from a nested dependency. This includes the .exes.

We've suppressed that behavior so that WindowsTerminal and conhost can live in the appx package root next to their DLLs.

We must also do this so that the .winmd -> AppX Manifest generator works right -- if we don't, it will insert references like this:

<InProcessServer>
  <Path>Whatever.dll</Path>
</InProcessServer>

when our directory structure looks like this:

WindowsTerminal/Whatever.dll
WindowsTerminal/Whatever.xbf

This will fail validation (and activation)! This cannot be fixed, so we stomp the directory for DLL and EXE files.

<Target Name="OpenConsoleStompSourceProjectForWapProject" BeforeTargets="_ConvertItems">
<ItemGroup>
<!-- Stomp all "SourceProject" values for all incoming dependencies to flatten the package. -->
<_TemporaryFilteredWapProjOutput Include="@(_FilteredNonWapProjProjectOutput)" />
<_FilteredNonWapProjProjectOutput Remove="@(_TemporaryFilteredWapProjOutput)" />
<_FilteredNonWapProjProjectOutput Include="@(_TemporaryFilteredWapProjOutput)">
<!-- Override the filename for OpenConsole.exe (only) -->
<TargetPath Condition="'%(Filename)' == 'OpenConsole' and '%(Extension)' == '.exe'">conhost.exe</TargetPath>
<!-- Blank the SourceProject here to vend all files into the root of the package. -->
<SourceProject></SourceProject>
</_FilteredNonWapProjProjectOutput>

Well, we're also stomping the directory for XBF files. Therefore, MinMaxCloseControl.xbf and App.xbf end up in the appx root.

This works, because we've overridden their resource locator paths:

const winrt::Windows::Foundation::Uri resourceLocator{ L"ms-appx:///MinMaxCloseControl.xaml" };
winrt::Windows::UI::Xaml::Application::LoadComponent(*this, resourceLocator, winrt::Windows::UI::Xaml::Controls::Primitives::ComponentResourceLocation::Nested);

That works great when deployed from Visual Studio. However, when the project is deployed by MSBuild (like in our CI), those loose XBF files are missing. If you look in one of the PRI files, it says that the path is TerminalApp/MinMaxCloseControl.xbf.

Well, that never existed on disk (see above: wapproj would put it in WindowsTerminal/MinMaxCloseControl.xbf because it was a transitive dependency.)

It gets worse. That path would actually work because the XBF file has been merged into the PRI file(!). However, the PRI files from wapproj transitive dependencies do not get merged into the global resources.pri.

If the transitive dependency PRI files can be merged into resources.pri, we can skip overriding the resource loader and remove the loose XBF files from our package.

/cc @metathinker, who originally reported a crash from the CI builds.

@DHowett-MSFT DHowett-MSFT added Issue-Bug It either shouldn't be doing this or needs an investigation. Area-Build Issues pertaining to the build system, CI, infrastructure, meta Product-Terminal The new Windows Terminal. Severity-Blocking We won't ship a release like this! No-siree. labels Jul 4, 2019
@DHowett-MSFT DHowett-MSFT added this to the Terminal v0.3 - Preview 2 milestone Jul 4, 2019
@ghost ghost added the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Jul 4, 2019
@DHowett-MSFT
Copy link
Contributor Author

Oh, also, this results in a crash when you use builds from Azure DevOps or MSBuild. Yeah.

@metathinker
Copy link
Contributor

metathinker commented Jul 4, 2019

Previous (and occasionally silly) discussion as part of another issue thread: #1364 (comment)

As Dustin notes above, .appx packages and loose-file app package layouts with this defect will cause Terminal to crash on startup; the precise nature of the crash is discussed in the other thread.

@DHowett-MSFT
Copy link
Contributor Author

I have a terrible idea. I'm going to use my holiday and try to replace the WAP packaging project with a native UWP project. Instead of going from EXE to Centennial (which is apparently very hard), why not go from UWP to Centennial (which will probably expose hundreds of other issues, but solve this set of a hundred issues 😁)

@DHowett-MSFT DHowett-MSFT self-assigned this Jul 4, 2019
@DHowett-MSFT
Copy link
Contributor Author

DHowett-MSFT commented Jul 4, 2019

Everything is terrible! 😄

feature WAP UAP
transitive dependencies
(this issue)
✔️
PRI merging
(this issue)
✔️
dependency resources
(this issue)
✔️
simpler project structure
(one extra project)
✔️
isn't a weird .NET project type
(this makes debugging annoying)
✔️
partially works today ✔️
(new uncharted unknown)
depends on VCLibs.Desktop ✔️
(this seems hard)
intended for this purpose ✔️
(throws a warning)
can be debugged
(with some hoops)
✔️
(application thinks it isn't deployed)
Project not selected to build for this solution configuration, even after it builds.

@mdtauk
Copy link

mdtauk commented Jul 4, 2019

WinUI and XAML Desktop apps - may change all this. I hope you are sharing your findings and thoughts with that team.

@DHowett-MSFT
Copy link
Contributor Author

@mdtauk not only are we sharing our findings and thoughts with that team, we report to the same engineering manager as that team. 😄
Unfortunately, that team doesn't own the packaging rules shipped with Visual Studio.

@mdtauk
Copy link

mdtauk commented Jul 4, 2019

@DHowett-MSFT I can only imagine that the VS team, would be developing new compilation and packaging processes, to match the requirements of the frameworks which are to come

@zadjii-msft
Copy link
Member

zadjii-msft commented Jul 4, 2019 via email

@DHowett-MSFT
Copy link
Contributor Author

feature WAP
If you add the dependencies directly to the wap project,
they fail with new and interesting linker errors
✔️

Yeah, if you add TerminalConnection as a direct dependency to wapproj, it has to re-link it during deployment, and it refuses to link in ConTypes.lib.

@DHowett-MSFT DHowett-MSFT removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Jul 8, 2019
@ghost ghost added the In-PR This issue has a related PR label Jul 10, 2019
@DHowett-MSFT DHowett-MSFT changed the title wap packaging: .xbf and .pri generation is a house of horrors wap packaging: .xbf and .pri generation is not working for us Jul 10, 2019
@DHowett-MSFT
Copy link
Contributor Author

/cc @ocalvo for info

@ocalvo
Copy link
Contributor

ocalvo commented Jul 12, 2019

/cc @azchohfi for info

@ghost ghost added Needs-Tag-Fix Doesn't match tag requirements Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed In-PR This issue has a related PR labels Jul 12, 2019
DHowett-MSFT pushed a commit that referenced this issue Jul 12, 2019
* Fix the WAP packaging project

This commits fixes the centennial package by:
* Forcing XBF (XAML binary format) files to be embedded in project
  PRI files.
* Moving package content generation to before PRI generation
* Collecting all of the package's PRI files to merge into resources.pri
* Fixing the hardcoded resource paths to reflect the new reality.

It also includes a magic value that fixes the bug where the project is
autodetected as a Mixed (CLR + Native) project.

Fixes #1816.
@metathinker
Copy link
Contributor

metathinker commented Jul 18, 2019

I can't reopen issues after they have been closed, so paging @DHowett-MSFT:

It looks like the issue wasn't fixed or was reintroduced - I built the current master commit 988fe0b and installed the loose-file AppX layout. The crash on startup is back, and occurs at the same place. This time, the resource that failed to load is: ms-appx:///TerminalApp/TerminalPage.xaml

@DHowett-MSFT
Copy link
Contributor Author

Yuuuup! #2018 to fix :)

mcpiroman pushed a commit to mcpiroman/terminal that referenced this issue Jul 23, 2019
* Fix the WAP packaging project

This commits fixes the centennial package by:
* Forcing XBF (XAML binary format) files to be embedded in project
  PRI files.
* Moving package content generation to before PRI generation
* Collecting all of the package's PRI files to merge into resources.pri
* Fixing the hardcoded resource paths to reflect the new reality.

It also includes a magic value that fixes the bug where the project is
autodetected as a Mixed (CLR + Native) project.

Fixes microsoft#1816.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Build Issues pertaining to the build system, CI, infrastructure, meta Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. Severity-Blocking We won't ship a release like this! No-siree.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants