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

[vcpkg] Fix vcpkg.targets incorrect output #39216

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

JonLiu1993
Copy link
Member

Fixes #38502

When building using scripts\buildsystems\msbuild\vcpkg.targets and using Windows PowerShell (rather than PowerShell Core), the build outputs an error message when it actually succeeds.

'pwsh.exe' is not recognized as an internal or external command,

As per @berryzplus' comment on line 229 it incorrectly calls pwsh.exe without checking if pwsh.exe is included in %PATH%

<!-- Search %PATH% for pwsh.exe if it is available. -->
<Exec
Condition="'$(VcpkgXUseBuiltInApplocalDeps)' != 'true'"
Command="pwsh.exe $(_ZVcpkgAppLocalPowerShellCommonArguments)"
IgnoreExitCode="true"
UseCommandProcessor="false">

Using WHERE command to check if pwsh.exe is in a specific %PATH% fixes this bug

  • Changes comply with the maintainer guide.
  • SHA512s are updated for each updated download.
  • The "supports" clause reflects platforms that may be fixed by this new version.
  • Any fixed CI baseline entries are removed from that file.
  • Any patches that are no longer applied are deleted from the port's directory.
  • The version database is fixed by rerunning ./vcpkg x-add-version --all and committing the result.
  • Only one version is added to each modified port's versions file.
@JonLiu1993 JonLiu1993 added category:vcpkg-bug The issue is with the vcpkg system (including helper scripts in `scripts/cmake/`) info:internal This PR or Issue was filed by the vcpkg team. labels Jun 11, 2024
@JonLiu1993 JonLiu1993 marked this pull request as ready for review June 11, 2024 08:03
@jimwang118 jimwang118 marked this pull request as draft June 11, 2024 08:24
@BillyONeal
Copy link
Member

BillyONeal commented Jun 11, 2024

I don't believe the output is 'incorrect'. I believe the correct fix is to deploy z-applocal and get rid of the powershell dependency completely. I asked Victor to see if our metrics can tell us whether enough folks are trying that option yet.

If the answer is 'not enough folks are using that yet' we should consider asking anyone complaining about this to enable that instead.

@@ -226,6 +226,15 @@
<!-- Search %PATH% for pwsh.exe if it is available. -->
<Exec
Condition="'$(VcpkgXUseBuiltInApplocalDeps)' != 'true'"
Command="where.exe pwsh.exe &gt;NUL 2&gt;&amp;1"
IgnoreExitCode="true"
UseCommandProcessor="true">

Choose a reason for hiding this comment

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

Sorry for the inconvenience. It seems better to set this to false.

Suggested change
UseCommandProcessor="true">
UseCommandProcessor="false">
@berryzplus
Copy link

@BillyONeal

I don't believe the output is 'incorrect'. I believe the correct fix is to deploy z-applocal and get rid of the powershell dependency completely. I asked Victor to see if our metrics can tell us whether enough folks are trying that option yet.

If the attached comment, "Search %PATH% for pwsh.exe if it is available." is correct, then I think the command is wrong.

As you know, vcpkg.exe downloads the latest pwsh.exe at runtime.
It is necessary to investigate why the Exec task tries to search the installed pwsh.exe instead of the downloaded one.

In my scenario, pwsh.exe is not involved, so I don't want to see "unrecognized command" errors.

@JonLiu1993 JonLiu1993 added category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly and removed category:vcpkg-bug The issue is with the vcpkg system (including helper scripts in `scripts/cmake/`) labels Jun 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly info:internal This PR or Issue was filed by the vcpkg team.
4 participants