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

Fix build break where Microsoft.Terminal.Control.dll is empty #9537

Merged
3 commits merged into from
Mar 18, 2021

Conversation

zadjii-msft
Copy link
Member

@zadjii-msft zadjii-msft commented Mar 18, 2021

TIL that the <None Include="Foo.def" /> line in our projects is
actually totally meaningless. The important line is the one that's in
cppwinrt.build.pre.props, where we declare

<ModuleDefinitionFile Condition="Exists('$(ProjectName).def')">$(ProjectName).def</ModuleDefinitionFile>

So if you change a project's name, and not the .def file, then the
linker will just not use the .def file at all.

More importantly, this seemingly doesn't matter in debug builds. In a
Debug build, the linker will happily still include WINRT_CanUnloadNow
and WINRT_GetActivationFactory in the exports from the dll, even
without the .def. But in a Release build, the linker is much more
agressive about pruning symbols that aren't referenced, and without
those two, NONE of the symbols are eventually referenced.

This PR fixes Microsoft.Terminal.Control by renaming the .def, and
makes it marginally harder for someone to make the same mistake in the
future.

References

PR Checklist

@ghost ghost added Priority-0 Bugs that we consider release-blocking/recall-class (P0) Product-Terminal The new Windows Terminal. labels Mar 18, 2021
@zadjii-msft zadjii-msft changed the title Fix build break there Microsoft.Terminal.Control.dll is empty Fix build break where Microsoft.Terminal.Control.dll is empty Mar 18, 2021
@zadjii-msft zadjii-msft requested a review from DHowett March 18, 2021 15:11
@zadjii-msft
Copy link
Member Author

@msftbot merge this in 1 minute

@ghost ghost added the AutoMerge Marked for automatic merge by the bot when requirements are met label Mar 18, 2021
@ghost
Copy link

ghost commented Mar 18, 2021

Hello @zadjii-msft!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

  • I won't merge this pull request until after the UTC date Thu, 18 Mar 2021 15:20:56 GMT, which is in 1 minute

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

@zadjii-msft zadjii-msft added the Needs-Second It's a PR that needs another sign-off label Mar 18, 2021
@ghost ghost merged commit 2ed367f into main Mar 18, 2021
@ghost ghost deleted the dev/migrie/b/9529-build-break branch March 18, 2021 16:14
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AutoMerge Marked for automatic merge by the bot when requirements are met Needs-Second It's a PR that needs another sign-off Priority-0 Bugs that we consider release-blocking/recall-class (P0) Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

main is horked, bought farm, etc.
3 participants