Skip to content

Conversation

@fcharlie
Copy link
Contributor

Summary of the Pull Request

This PR is mainly to fix the conhost.exe not using the application manifest correctly and the operating system version cannot be detected correctly. Related documents can be viewed #2053.

This update also brings support for longPath to conhost.exe.

References

PR Checklist

  • Closes UseDx option no longer works in conhost #2053
  • CLA signed. If not, go over here and sign the CLA
  • Tests added/passed
  • Requires documentation to be updated
  • I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

When DirectWrite rendering is turned on with UseDx, this application cannot display emoji on Windows 10 before applying this PR. After applying this PR, it can be displayed normally.

屏幕截图(7)

@DHowett
Copy link
Member

DHowett commented Jul 22, 2019

@miniksa do you know whether the windows inbox build automatically stamps us with the right sort of manifest, or if we need to make sure this change works over there as well?

I'm all for it. This is great, and it moves our comctl dependency out of our project file 😄

@DHowett-MSFT
Copy link
Contributor

Wait, the conhost.exe on my 18362 machine has two manifests. That can't be right ???

@miniksa
Copy link
Member

miniksa commented Jul 22, 2019

@miniksa Michael Niksa FTE do you know whether the windows inbox build automatically stamps us with the right sort of manifest, or if we need to make sure this change works over there as well?

I'm all for it. This is great, and it moves our comctl dependency out of our project file 😄

The Windows build automagically stamps us with a lot of manifest stuff. We cannot take this change until we ensure that it won't screw up the Windows build when this integrates back.

@miniksa
Copy link
Member

miniksa commented Jul 22, 2019

OK @fcharlie, I've got two potential solutions for you here to unblock this so I'd feel OK taking it:

  1. Instead of using conhost.exe.manifest inside the .vcxproj (which will force me to make sure it still works properly inside the Windows build too since the sources file references it), instead, copy/paste this file into a new openconsole.exe.manifest, correct it how you did, and put THAT into the .vcxproj. That way there is an "inside windows build" manifest and an "outside windows build" manifest.
    --OR--
  2. Go change the version detect checks in the DX renderer to instead use QI to detect whether the interface is supported or not (instead of OS version). If the QI fails, just skip that feature. Feature detection is always preferred to version checking.

Or even better... Do both!

Copy link
Contributor

@DHowett-MSFT DHowett-MSFT left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@fcharlie fcharlie closed this Jul 22, 2019
@fcharlie fcharlie reopened this Jul 22, 2019
@fcharlie
Copy link
Contributor Author

Sorry, I missed the close button on my phone

@fcharlie
Copy link
Contributor Author

@miniksa #2065 Fixed conhost.exe built in Windows internal

@miniksa miniksa merged commit 3b96a84 into microsoft:master Jul 23, 2019
@miniksa
Copy link
Member

miniksa commented Jul 23, 2019

@miniksa #2065 Fixed conhost.exe built in Windows internal

@fcharlie thank you very much!

@fcharlie fcharlie deleted the fix_conhost_osver_detect branch July 23, 2019 01:02
mcpiroman pushed a commit to mcpiroman/terminal that referenced this pull request Jul 23, 2019
* add openconsole.exe.manifest to fix detecting of os version
mcpiroman pushed a commit to mcpiroman/terminal that referenced this pull request Jul 25, 2019
* add openconsole.exe.manifest to fix detecting of os version
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.

UseDx option no longer works in conhost

4 participants