icu icon indicating copy to clipboard operation
icu copied to clipboard

Published 20 hours ago • verified publisher icon unicode-org

ICU-22062 Fixes for building on MinGW, from the conan CCI patch set

Open paulharris opened this issue 2 years ago • 3 comments

https://github.com/conan-io/conan-center-index/pull/2868 This was the PR on conan’s CCI that introduced a patch for building on MinGW. I’m submitting a PR here for your consideration. I’m looking to be helpful and upstream patches where possible.

Checklist
  • [x] Required: Issue filed: https://unicode-org.atlassian.net/browse/ICU-22062
  • [x] Required: The PR title must be prefixed with a JIRA Issue number.
  • [x] Required: The PR description must include the link to the Jira Issue, for example by completing the URL in the first checklist item
  • [x] Required: Each commit message must be prefixed with a JIRA Issue number.
  • [ ] Issue accepted (done by Technical Committee after discussion)
  • [ ] Tests included, if applicable
  • [ ] API docs and/or User Guide docs changed or added, if applicable

paulharris avatar Jun 16 '22 03:06 paulharris

Thanks for making this PR and the JIRA ticket!

Two quick questions:

  • Why are these defines not needed? If they aren't needed, then could they be removed rather than commented out?
  • Why no changes to the x86 architecture? i.e.: Why no changes in the file https://github.com/unicode-org/icu/blob/d5f278843f2b3d4eb90c26f00e680eaeee75f076/icu4c/source/config/mh-mingw

jefgen avatar Jun 16 '22 16:06 jefgen

Apologies for the slow response to your quick questions, I'm trying to check with the original author. I'll set this to Draft for now and open it back up for review when I have something further.

paulharris avatar Jun 21 '22 03:06 paulharris

Hi there, I talked to the patch author, the patch was limited to what was required to make things work for the platforms that was to be used and tested. There are other patches for ICU, you can find them here: https://github.com/conan-io/conan-center-index/tree/master/recipes/icu/all/patches Each patch may not be applied to all of the versions, you can see which version for which patch here: https://github.com/conan-io/conan-center-index/blob/master/recipes/icu/all/conandata.yml

I'm not expert on ICU, would you be able to take the ideas from this patch and make it work better for your project?

paulharris avatar Oct 27 '22 13:10 paulharris