icu icon indicating copy to clipboard operation
icu copied to clipboard

ICU-22787 Fix ClangCL compilation on Windows

Open StefanStojanovic opened this issue 1 year ago • 10 comments

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

StefanStojanovic avatar Jun 03 '24 14:06 StefanStojanovic

#3013

srl295 avatar Jun 03 '24 15:06 srl295

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

View Diff Across Force-Push

~ 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.

StefanStojanovic avatar Jun 07 '24 19:06 StefanStojanovic

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.

Please wait with squashing until after feedback is resolved.

markusicu avatar Jun 07 '24 19:06 markusicu

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.

StefanStojanovic avatar Jun 07 '24 19:06 StefanStojanovic

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.

StefanStojanovic avatar Jun 11 '24 17:06 StefanStojanovic

I see that in CI-ICU4C all MSVC builds 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).

roubert avatar Jun 12 '24 15:06 roubert

The CI is currently broken,

It has now been fixed and the tests pass.

roubert avatar Jun 13 '24 12:06 roubert

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

View Diff Across Force-Push

~ 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.

StefanStojanovic avatar Jun 20 '24 10:06 StefanStojanovic

@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)

rp9-next avatar Aug 08 '24 08:08 rp9-next

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

View Diff Across Force-Push

~ 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.

StefanStojanovic avatar Aug 08 '24 12:08 StefanStojanovic

@roubert , merging is blocked by a change requested check which I believe is outdated.. Could you dismiss it ?

Done.

roubert avatar Aug 08 '24 16:08 roubert