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] Revise appdeploy and copy_tool_dependencies #21092

Merged
merged 8 commits into from
Nov 14, 2021

Conversation

dg0yt
Copy link
Contributor

@dg0yt dg0yt commented Oct 31, 2021

  • What does your PR fix?

    • [appdeploy] Fixes deployment for mingw on linux by
      • making path construction portable,
      • catching failing fixing mutex creation.
    • [copy_tool_dependencies] Enables deployment for debug config.
    • [copy_tool_dependencies] Fixes overwriting of log files within a single port build.
    • [copy_tool_dependencies] Enables actual logging at least when DEBUG_PORT is set (cf. debug_message in ports.cmake).
  • Which triplets are supported/not supported? Have you updated the CI baseline?

    all, no

  • Does your PR follow the maintainer guide?

    yes

  • If you have added/updated a port: Have you run ./vcpkg x-add-version --all and committed the result?

    --

  • For discussion

    • Logging (-verbose) might always be enabled. It doesn't clutter output.
    • If there is a better idea how to deal with the mutex creation issue...
    • To correctly handle some port being set to a single config (via triplet file), it would be necessary to also search the alternative bin directory.
    • It is not efficient to call appdeploy.ps1 twice. It should better take a list of directories as search path instead of a single directory.
      And it should also take a list of executables (or just a single directory).

@dg0yt
Copy link
Contributor Author

dg0yt commented Oct 31, 2021

The original mutex error on linux:

New-Object: /home/kpa/Projekte/vcpkg/vcpkg/scripts/buildsystems/msbuild/applocal.ps1:26:16
Line |
  26 |          return New-Object System.Threading.Mutex($false, $mtxName)
     |                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     | Exception calling ".ctor" with "2" argument(s): "The filename, directory name, or volume label syntax is incorrect. :
     | 'VcpkgAppLocalDeployBinary-/67pRTB1Ups2rRbHaFf9MmG5+3JTeHXhgLDIy6xHo3Wsrcl2RLAMD0XgKTwuKrwUcHqwWB5xF+iuO1frfY0xZQ=='"

@dg0yt
Copy link
Contributor Author

dg0yt commented Oct 31, 2021

The original mutex error on linux

... is caused by the / in the mutex name.

@dg0yt dg0yt marked this pull request as draft October 31, 2021 17:41
@JackBoosY JackBoosY added category:vcpkg-feature The issue is a new capability of the tool that doesn’t already exist and we haven’t committed category:vcpkg-bug The issue is with the vcpkg system (including helper scripts in `scripts/cmake/`) labels Nov 1, 2021
@dg0yt dg0yt marked this pull request as ready for review November 1, 2021 11:29
@JackBoosY JackBoosY added the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Nov 3, 2021
@JackBoosY
Copy link
Contributor

@BillyONeal Can you please review this PR?
Thanks.

if ($destModTime -lt $sourceModTime) {
Write-Verbose " ${targetBinaryName}: Updating $SourceDir\$targetBinaryName"
Copy-Item "$SourceDir\$targetBinaryName" $targetBinaryDir
Write-Verbose " ${targetBinaryName}: Updating from $sourceBinaryFilePath"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Minor output change here.

@ras0219-msft
Copy link
Contributor

It is not efficient to call appdeploy.ps1 twice. It should better take a list of directories as search path instead of a single directory.
And it should also take a list of executables (or just a single directory).

110% agreed. Additionally, using powershell for this has shown to be no longer sufficient. Both @BillyONeal and I have medium term desires to nuke this in favor of a small portable C++ utility (maybe separate, maybe built-in to vcpkg.exe), however the need for extensibility for plugin producers (Qt, magnum, etc) has made that non-trivial. I hope to eventually find a simple text/json format that can capture all needed information for those cases in a declarative way (and we can add a small amount of hardcoded logic for backwards compatibility to older port versions).

@dg0yt
Copy link
Contributor Author

dg0yt commented Nov 13, 2021

It is not efficient to call appdeploy.ps1 twice. It should better take a list of directories as search path instead of a single directory.
And it should also take a list of executables (or just a single directory).

110% agreed.

I just can't put too much in a single PR.

@ras0219-msft ras0219-msft merged commit 7dff5e8 into microsoft:master Nov 14, 2021
@ras0219-msft
Copy link
Contributor

Thanks!

@dg0yt dg0yt deleted the appdeploy branch November 14, 2021 17:05
Jimmy-Hu added a commit to Jimmy-Hu/vcpkg that referenced this pull request Nov 14, 2021
[vcpkg] Revise appdeploy and copy_tool_dependencies (microsoft#21092)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:vcpkg-bug The issue is with the vcpkg system (including helper scripts in `scripts/cmake/`) category:vcpkg-feature The issue is a new capability of the tool that doesn’t already exist and we haven’t committed requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants