Skip to content

[libmodplug] [ffmpeg] fix libmodplug .pc file for static windows build#18867

Merged
vicroms merged 14 commits intomicrosoft:masterfrom
mcmtroffaes:feature/modplug-static-fix
Jul 22, 2021
Merged

[libmodplug] [ffmpeg] fix libmodplug .pc file for static windows build#18867
vicroms merged 14 commits intomicrosoft:masterfrom
mcmtroffaes:feature/modplug-static-fix

Conversation

@mcmtroffaes
Copy link
Copy Markdown
Contributor

@mcmtroffaes mcmtroffaes commented Jul 8, 2021

  • What does your PR fix?

    Fixes the libmodplug .pc file on windows (user32 is needed). Patch has been posted upstream here: Fix Libs.private in .pc file. Konstanty/libmodplug#59 I've enabled windows static builds for the ffmpeg modplug feature as well.

  • Which triplets are supported/not supported? Have you updated the CI baseline?

    Libmodplug now additionally supports all static windows triplets.

  • Does your PR follow the maintainer guide?

    Yes, to the best of my knowledge.

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

    Yes.

@NancyLi1013 NancyLi1013 self-assigned this Jul 9, 2021
Copy link
Copy Markdown
Contributor

@NancyLi1013 NancyLi1013 left a comment

Choose a reason for hiding this comment

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

Could you please update

file(COPY ${SOURCE_PATH}/COPYING DESTINATION ${CURRENT_PACKAGES_DIR}/share/libmodplug)
file(RENAME ${CURRENT_PACKAGES_DIR}/share/libmodplug/COPYING ${CURRENT_PACKAGES_DIR}/share/libmodplug/copyright)
as

file(INSTALL ${SOURCE_PATH}/COPYING DESTINATION ${CURRENT_PACKAGES_DIR}/share/{PORT} RENAME copyright)
?

@NancyLi1013 NancyLi1013 added requires:author-response category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist labels Jul 9, 2021
@dg0yt
Copy link
Copy Markdown
Contributor

dg0yt commented Jul 9, 2021

file(INSTALL ${SOURCE_PATH}/COPYING DESTINATION ${CURRENT_PACKAGES_DIR}/share/{PORT} RENAME copyright)

@NancyLi1013 Are you aware that the RENAME argument is not a documented feature? If it is meant to be used even more in vcpkg, a documentation request should be made at Kitware.

@mcmtroffaes
Copy link
Copy Markdown
Contributor Author

Could you please update

file(COPY ${SOURCE_PATH}/COPYING DESTINATION ${CURRENT_PACKAGES_DIR}/share/libmodplug)
file(RENAME ${CURRENT_PACKAGES_DIR}/share/libmodplug/COPYING ${CURRENT_PACKAGES_DIR}/share/libmodplug/copyright)

Done.

Are you aware that the RENAME argument is not a documented feature? If it is meant to be used even more in vcpkg, a documentation request should be made at Kitware.

Yes, that would be great.

@NancyLi1013
Copy link
Copy Markdown
Contributor

file(INSTALL ${SOURCE_PATH}/COPYING DESTINATION ${CURRENT_PACKAGES_DIR}/share/{PORT} RENAME copyright)

@NancyLi1013 Are you aware that the RENAME argument is not a documented feature? If it is meant to be used even more in vcpkg, a documentation request should be made at Kitware.

Thanks for pointing this out. @dg0yt Currently, there is no any documentation that declares how we should use RENAME in handling license. We suggest to handle like this is just to reduce codes.

You can also see this here.

@NancyLi1013 NancyLi1013 added info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. and removed requires:author-response labels Jul 9, 2021
@NancyLi1013
Copy link
Copy Markdown
Contributor

LGTM, thanks again for your fixes. @mcmtroffaes

@mcmtroffaes
Copy link
Copy Markdown
Contributor Author

Merged master & bumped port version.

@vicroms vicroms merged commit 5abd47d into microsoft:master Jul 22, 2021
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 info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants