Skip to content

Conversation

KaylinFinke
Copy link
Contributor

@KaylinFinke KaylinFinke commented Jul 2, 2025

This change intends to improve/enable generating a static library using visual studio on Windows.

Due to the circular dependency on icudt.lib it remains necessary to build the project twice.

First build the static data. You can do this with:

set ENABLE_STATIC=YES&&set CL=/DU_STATIC_IMPLEMENTATION&& set ICU_PACKAGE_MODE=-m static&& msbuild icu4c/source/allinone/allinone.sln /p:Configuration=debug /p:Platform=x64

Save off icudt.lib (for this configuration it's in icu4c/lib64) and clean the project; Restore the .lib and re-run the solution this time indicating you already have static data. It will still be overwritten by makedata.mak, but the data will be identical:

set WITH_STATIC_DATA=YES&& set ENABLE_STATIC=YES&&set CL=/DU_STATIC_IMPLEMENTATION&& set ICU_PACKAGE_MODE=-m static&& msbuild icu4c/source/allinone/allinone.sln /p:Configuration=debug /p:Platform=x64

Then run the tests to confirm:
icu4c\source\allinone\icucheck.bat x64 Debug

Checklist

  • Required: Issue filed: ICU-23154
  • Required: The PR title must be prefixed with a JIRA Issue number.
  • 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

@markusicu markusicu self-assigned this Jul 10, 2025
Due to the circular dependency on icudt.lib it remains necessary to build the project twice.

First build the static data. You can do this with:

set ENABLE_STATIC=YES&&set CL=/DU_STATIC_IMPLEMENTATION&& set ICU_PACKAGE_MODE=-m static&& msbuild icu4c/source/allinone/allinone.sln /p:Configuration=debug /p:Platform=x64

Save off icudt.lib (for this configuration it's in icu4c/lib64) and clean the project; Restore the .lib and re-run the solution this time indicating you already have static data. It will still be overwritten by makedata.mak, but the data will be identical:

set WITH_STATIC_DATA=YES&& set ENABLE_STATIC=YES&&set CL=/DU_STATIC_IMPLEMENTATION&& set ICU_PACKAGE_MODE=-m static&& msbuild icu4c/source/allinone/allinone.sln /p:Configuration=debug /p:Platform=x64

Then run the tests to confirm:
icu4c\source\allinone\icucheck.bat x64 Debug
@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • icu4c/source/data/makedata.mak is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

Copy link
Contributor

@richgillam richgillam left a comment

Choose a reason for hiding this comment

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

I looked at this, but it's all Visual Studio project-file changes, and I'm unqualified to pass judgment on whether the changes are sensible and harmless. I'm hoping one of the other reviewers has a better sense of this than I do...

@KaylinFinke
Copy link
Contributor Author

KaylinFinke commented Jul 12, 2025

That's understandable. Thanks for taking a look. If it helps the idea is:
<Lib> sections are used when building a StaticLibrary ConfigurationType. <Link> sections are for DynamicLibrary. The settings in <Link> have been duplicated in <Lib> for the DynamicLibrary projects except there's no <ImportLibrary> section and the <OutputFile> section is named the same as the <ImportLibrary> was for the DynamicLibrary. Also, when we link other StaticLibrary libraries, we don't list those libraries in <AdditionalDependencies> to avoid duplicate symbols.

We then override DynamicLIbrary with StaticLibrary based on an environment variable.

For stubdata, when we want to build the project with static data we already have, we override it to Utility instead of StaticLibrary, preventing the project from building stubdata since we already have data.

@richgillam
Copy link
Contributor

Fredrik summoned @rp9-next and @StefanStojanovic in your other PR; it'd be helpful if they could weigh in here too. Without any comments from actual Microsoft or Windows people, I'm inclined to just rubber-stamp this...

@rp9-next
Copy link
Contributor

tagging @Mamtadhaka to take a look

@Mamtadhaka
Copy link

tagging @Mamtadhaka to take a look

@KaylinFinke can you please let us know if these changes are tested, it would be helpful if the description is updated with the testing details. Thanks

@KaylinFinke
Copy link
Contributor Author

KaylinFinke commented Jul 21, 2025

tagging @Mamtadhaka to take a look

@KaylinFinke can you please let us know if these changes are tested, it would be helpful if the description is updated with the testing details. Thanks

Yes, this is outlined in the ticket but I can elaborate here. Compilation and tests https://unicode-org.github.io/icu/userguide/icu4c/build.html#running-the-tests-from-the-windows-command-line-cmd were performed for both Clang-CL and CL using steps similar to what is documented in the description for debug x64 CL. (that is, replace the compilation line for debug x64 CL with another configuration) for debug and release against Win32 and x64. I had some compiler stability issues when building Win32 Clang-CL on my laptop and switched to my desktop so it didn't need multiple attempts but once I did so I did not experience these problems. Runs were done from different visual studio command prompts using clean git repositories (git clean -xfd)

The Clang-CL tests were done using ICU-23151 with this change, while the CL tests were this change alone. The CL tests were repeated as a baseline without these changes to compilation passed there as well. I developed this off 88db995 and tested against 77.1 with these changes as well. I used Windows 11 (26100) using Visual Studio 2022 17.14.7 Preview 1.0. I did compile the project using the corresponding latest non-preview release at the time, both with and without my changes, but I didn't test all configurations I could against the non-preview build.

I did not test ARM nor ARM64 and do not have access to these platforms.

Example commands to compile the project for the above 8 configurations are documented in ICU-23151

@KaylinFinke
Copy link
Contributor Author

Is there something I can do to help testing or make this easier to understand? If this is not a desirable solution I'm happy to rework it.
-Kaylin

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants