Skip to content

[icu] mingw support + [mp3lame] Fix VCPKG_TARGET_STATIC_LIBRARY_SUFFIX#14969

Merged
BillyONeal merged 10 commits intomicrosoft:masterfrom
longnguyen2004:icu-mingw
Dec 15, 2020
Merged

[icu] mingw support + [mp3lame] Fix VCPKG_TARGET_STATIC_LIBRARY_SUFFIX#14969
BillyONeal merged 10 commits intomicrosoft:masterfrom
longnguyen2004:icu-mingw

Conversation

@longnguyen2004
Copy link
Copy Markdown
Contributor

@longnguyen2004 longnguyen2004 commented Dec 6, 2020

Describe the pull request

  • What does your PR fix?
    Add support for icu on mingw triplets, and also add support for building boost-locale with icu
  • Which triplets are supported/not supported? Have you updated the CI baseline?
    All mingw triplets
    boost-locale[icu] tested on both x64-mingw-dynamic and x64-windows
  • Does your PR follow the maintainer guide?
    Yes

@PhoebeHui PhoebeHui added the category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist label Dec 7, 2020
@PhoebeHui
Copy link
Copy Markdown
Contributor

PhoebeHui commented Dec 7, 2020

@longnguyen2004, the icu:x64-windows-static failed with following issue in CI testing, could you please confirm if this issue reprod locally?

Failures:

-- Package x64-windows-static-dbg
-- Package x64-windows-static-dbg done
CMake Error at ports/icu/portfile.cmake:237 (file):
  file RENAME failed to rename
    D:/packages/icu_x64-windows-static/lib/sicudt..lib
  to
    D:/packages/icu_x64-windows-static/lib/icudt..lib

  because: No such file or directory

Call Stack (most recent call first):
  scripts/ports.cmake:136 (include)
Error: Building package icu:x64-windows-static failed with: BUILD_FAILED

@longnguyen2004
Copy link
Copy Markdown
Contributor Author

longnguyen2004 commented Dec 7, 2020

Guess I'll have to fix mp3lame as well, since there was a similar problem.

@longnguyen2004 longnguyen2004 changed the title [icu] mingw support [icu] mingw support + [mp3lame] Fix VCPKG_TARGET_STATIC_LIBRARY_SUFFIX Dec 7, 2020
PhoebeHui
PhoebeHui approved these changes Dec 8, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@PhoebeHui PhoebeHui left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the PR!

@PhoebeHui PhoebeHui added the info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. label Dec 8, 2020
ras0219-msft
ras0219-msft suggested changes Dec 8, 2020
View reviewed changes
@BillyONeal
Copy link
Copy Markdown
Member

BillyONeal commented Dec 8, 2020

Is the -Bsymbolic change submitted upstream to ICU or is there some reason it should not be?

@BillyONeal BillyONeal added requires:author-response and removed info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. labels Dec 8, 2020
@longnguyen2004
Copy link
Copy Markdown
Contributor Author

longnguyen2004 commented Dec 9, 2020

I haven't submitted a patch to ICU yet, but I'll look into it.
FWIW, -Bsymbolic is only meaningful for ELF platforms, and lld also doesn't support it in the mingw driver, so I think upstream will accept it.

@BillyONeal
Copy link
Copy Markdown
Member

BillyONeal commented Dec 9, 2020

I haven't submitted a patch to ICU yet, but I'll look into it.
FWIW, -Bsymbolic is only meaningful for ELF platforms, and lld also doesn't support it in the mingw driver, so I think upstream will accept it.

Can you actually submit it to them?

Thanks!

@longnguyen2004
Copy link
Copy Markdown
Contributor Author

longnguyen2004 commented Dec 9, 2020

Submitted:
Jira ticket: https://unicode-org.atlassian.net/browse/ICU-21435
unicode-org/icu#1505

@longnguyen2004
Copy link
Copy Markdown
Contributor Author

longnguyen2004 commented Dec 14, 2020 •
edited
Loading

@BillyONeal Seems like the fix will be left for 69.1, which is a long time away, can you check this PR again and make sure it's good?

@BillyONeal
Copy link
Copy Markdown
Member

BillyONeal commented Dec 14, 2020

Absolutely: backports are fair game in patches! We just want to make sure upstream knows a bug exists etc. before we backport them in patches.

ras0219-msft
ras0219-msft suggested changes Dec 14, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@ras0219-msft ras0219-msft left a comment

Choose a reason for hiding this comment

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

The generated port version of boost-locale must also be changed:
https://github.com/microsoft/vcpkg/pull/14969/files#diff-1fc3b5ebed025c0311a4625fa496b6db0580e6a7e97a88d0f9fbf402cd83d571R28

@BillyONeal
Copy link
Copy Markdown
Member

BillyONeal commented Dec 14, 2020

I'm taking care of that, stand by

@BillyONeal BillyONeal merged commit 7944ed3 into microsoft:master Dec 15, 2020
@BillyONeal
Copy link
Copy Markdown
Member

BillyONeal commented Dec 15, 2020

Thanks for your contribution!

@longnguyen2004 longnguyen2004 deleted the icu-mingw branch December 16, 2020 01:40
strega-nil pushed a commit to strega-nil/vcpkg that referenced this pull request May 5, 2021
@longnguyen2004
[icu] mingw support + [mp3lame] Fix VCPKG_TARGET_STATIC_LIBRARY_SUFFIX (
2065fef 
microsoft#14969)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@BillyONeal BillyONeal BillyONeal approved these changes

+2 more reviewers

@ras0219-msft ras0219-msft ras0219-msft approved these changes

@PhoebeHui PhoebeHui PhoebeHui approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@PhoebeHui PhoebeHui

Labels

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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@longnguyen2004 @PhoebeHui @BillyONeal @ras0219-msft