ICU-22787 Fix ClangCL compilation on Windows
I'm working on supporting Node.js on Windows. There is an ongoing effort to enable it to be compiled with ClangCL. While working on it I noticed an issue when creating .obj file in ICU because of the *pCPU = IMAGE_FILE_MACHINE_UNKNOWN; line, and the code I made fixes that by adding a new option. Although this is a broader problem that should be fixed for multiple compilers and platforms, my change is aimed toward ClangCL and Windows, but can be used as a good foundation to work on fixes for other platforms and compilers and make corrections in future iterations.
This issue is currently our main blocker for enabling ClangCL compilation for Node.js and it'd be greatly appreciated if it could be reviewed soon.
Refs: https://github.com/nodejs/node/pull/53003
Checklist
- [x] Required: Issue filed: https://unicode-org.atlassian.net/browse/ICU-22787
- [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.
- [x] Issue accepted (done by Technical Committee after discussion)
- [ ] Tests included, if applicable
- [ ] API docs and/or User Guide docs changed or added, if applicable
#3013
Notice: the branch changed across the force-push!
- icu4c/source/common/unicode/platform.h is now changed in the branch
- icu4c/source/tools/genccode/genccode.c is different
- icu4c/source/tools/toolutil/pkg_genc.cpp is different
~ Your Friendly Jira-GitHub PR Checker Bot
Thanks for the review @roubert, I tried to address all your comments in a new commit.
I see 2 checks failed: jira-ticket and single-commit. I'll squash commits and keep only the original commit message once all is ready to land, but I kept 2 commits now for easier reviewing.
I see 2 checks failed:
jira-ticketandsingle-commit. I'll squash commits and keep only the original commit message once all is ready to land, but I kept 2 commits now for easier reviewing.
Please wait with squashing until after feedback is resolved.
Please wait with squashing until after feedback is resolved.
Will do.
By the way, I see that in CI-ICU4C all MSVC builds failed. I'm not sure why that happened, but I built it locally before pushing (allinone.sln -> Build Solution) and it worked.
Hey everyone, I wanted to check if there is anything else for me to do after my initial changes? If you think it is better to leave the new option I can every my latest changes in genccode.c. Other than that, are the other changes good? Regards.
I see that in
CI-ICU4CallMSVCbuilds failed. I'm not sure why that happened,
The CI is currently broken, you'll just have to wait for it to get fixed (and then those tests should pass again).
The CI is currently broken,
It has now been fixed and the tests pass.
Notice: the branch changed across the force-push!
- icu4c/source/common/unicode/platform.h is different
- icu4c/source/tools/genccode/genccode.c is different
- icu4c/source/tools/toolutil/pkg_genc.cpp is different
- icu4c/source/tools/toolutil/pkg_genc.h is different
~ Your Friendly Jira-GitHub PR Checker Bot
@roubert I pushed the changes in a new commit. I addressed your comments and made changes based on this comment. Please let me know if there is something else I should change. Regards.
@roubert , merging is blocked by a change requested check which I believe is outdated.. Could you dismiss it ?
@StefanStojanovic, please squash all the commits to a single commit for merging (you could use the squash branch tool as well)
Notice: the branch changed across the force-push!
- icu4c/source/tools/pkgdata/pkgdata.cpp is different
- icu4c/source/tools/toolutil/pkg_genc.cpp is different
- icu4c/source/tools/toolutil/pkg_genc.h is different
~ Your Friendly Jira-GitHub PR Checker Bot
@StefanStojanovic, please squash all the commits to a single commit for merging (you could use the squash branch tool as well)
Thanks for the review @rp9-next. I rebased to the latest main branch and squashed commits afterward.
@roubert , merging is blocked by a change requested check which I believe is outdated.. Could you dismiss it ?
Done.