Skip to content

[soundtouch] update to 2.3.1#20017

Merged
BillyONeal merged 1 commit intomicrosoft:masterfrom
Be-ing:soundtouch_2.3.1
Sep 29, 2021
Merged

[soundtouch] update to 2.3.1#20017
BillyONeal merged 1 commit intomicrosoft:masterfrom
Be-ing:soundtouch_2.3.1

Conversation

@Be-ing
Copy link
Contributor

@Be-ing Be-ing commented Sep 7, 2021

Describe the pull request
I worked with upstream to implement a CMake build system, so I
removed the CMakeLists.txt from this port.

  • What does your PR fix?

    • Headers were not installed by the port before.
    • The port did not allow dynamic linking before.
    • pkgconfig and CMake config files were not installed before.
  • Which triplets are supported/not supported? Have you updated the CI baseline?

    all but UWP

  • 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?

    yes

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This is a new experimental fast check for PR issues. Please let us know if this bot is helpful!

After committing all other changes, the version database must be updated
git add -u && git commit
git checkout 71422c627264daedcbcd46f01f1ed0dcd8460f1b -- versions
./vcpkg x-add-version --all
Diff
diff --git a/versions/s-/soundtouch.json b/versions/s-/soundtouch.json
index a6919da..21e2b9c 100644
--- a/versions/s-/soundtouch.json
+++ b/versions/s-/soundtouch.json
@@ -1,7 +1,7 @@
 {
   "versions": [
     {
-      "git-tree": "ce0caddead031b61d55d090755f9b18925a9d51e",
+      "git-tree": "1543b3d35dc78c0660da5aa34023d25187975741",
       "version-string": "2.3.1",
       "port-version": 0
     },

@JonLiu1993 JonLiu1993 self-assigned this Sep 7, 2021
@JonLiu1993 JonLiu1993 added category:port-update The issue is with a library, which is requesting update new revision category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist labels Sep 7, 2021
@JonLiu1993
Copy link
Contributor

@Be-ing ,Have you tested the features locally? I tested the features in x86-windows and x64-windows but all failed:
install-x86-windows-dbg-out.log

@Be-ing
Copy link
Contributor Author

Be-ing commented Sep 7, 2021

It seems the soundtouchdll feature does not work on Windows...

@Be-ing
Copy link
Contributor Author

Be-ing commented Sep 7, 2021

Upstream merge request to fix SoundTouchDLL build with MSVC: https://gitlab.com/soundtouch/soundtouch/-/merge_requests/18

@Be-ing
Copy link
Contributor Author

Be-ing commented Sep 7, 2021

Upstream merged my fix for the soundtouchdll feature and moved the 2.3.1 tag to include it. I updated the hash.

@JonLiu1993
Copy link
Contributor

@Be-ing ,All feature are tetsed successfully in the following triplet:

  • x86-windows
  • x64-windows
  • x64-linux
    but in x64-windows-static triplet failed:
DLLs should not be present in a static build, but the following DLLs were found:

    F:/test-pr/1/vcpkg/packages/soundtouch_x64-windows-static/bin/SoundTouchDLL.dll
    F:/test-pr/1/vcpkg/packages/soundtouch_x64-windows-static/debug/bin/SoundTouchDLL.dll

There should be no bin\ directory in a static build, but F:\test-pr\1\vcpkg\packages\soundtouch_x64-windows-static\bin is present.
There should be no debug\bin\ directory in a static build, but F:\test-pr\1\vcpkg\packages\soundtouch_x64-windows-static\debug/bin is present.
If the creation of bin\ and/or debug\bin\ cannot be disabled, use this in the portfile to remove them

    if(VCPKG_LIBRARY_LINKAGE STREQUAL "static")
        file(REMOVE_RECURSE "${CURRENT_PACKAGES_DIR}/bin" "${CURRENT_PACKAGES_DIR}/debug/bin")
    endif()

Found 2 error(s). Please correct the portfile:

@Be-ing
Copy link
Contributor Author

Be-ing commented Sep 8, 2021

SoundTouchDLL is only meant to be built as a DLL, hence the name. It is hardcoded in the upstream build system as a shared library. So if that warning is a problem, the only solution I think would be to disable the soundtouchdll feature for static builds.

@JonLiu1993
Copy link
Contributor

@Be-ing ,Could you please fix the x64-windows-static triplet error and handle the Conflicting files?

@Be-ing
Copy link
Contributor Author

Be-ing commented Sep 10, 2021

I need an answer how to handle the interaction between the soundtouchdll feature and static linking. I can think of a few options:

  1. Fatal error if soundtouchdll is enabled with static linking.
  2. Nonfatal warning if soundtouchdll is enabled with static linking.

I don't particularly care what the answer is; I don't have a need for SoundTouchDLL. It was made to implement Delphi and Pascal bindings for SoundTouch, but SoundTouch is primarily intended to be used as a C++ library.

@JonLiu1993
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@JonLiu1993
Copy link
Contributor

JonLiu1993 commented Sep 22, 2021

@Be-ing ,Please confirm If upstream follows semantic versioning. and if no please change version-semver to version.I don't have permission to help you fix it. By the way, merge master

@Be-ing
Copy link
Contributor Author

Be-ing commented Sep 26, 2021

Yes upstream follows semver.

@JonLiu1993
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@JonLiu1993
Copy link
Contributor

@Be-ing ,Please merge master,

Copy link
Contributor

@JackBoosY JackBoosY left a comment

Choose a reason for hiding this comment

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

Did you test the usage?

I worked with upstream to implement a CMake build system, so I
removed the CMakeLists.txt from this port.

This fixes multiple issues:
  * Headers were not installed by the port before.
  * The port did not allow dynamic linking before.
  * pkgconfig and CMake config files were not installed before.
@BillyONeal BillyONeal merged commit 33fb841 into microsoft:master Sep 29, 2021
@BillyONeal
Copy link
Member

Thanks for the help working with upstream!

@Be-ing
Copy link
Contributor Author

Be-ing commented Sep 29, 2021

One less port in the overlays! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist category:port-update The issue is with a library, which is requesting update new revision

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants