Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Select libraries using the ICU build time version #79259

Merged
merged 2 commits into from
Dec 11, 2022

Conversation

wscho77
Copy link
Contributor

@wscho77 wscho77 commented Dec 6, 2022

if CLR_USE_ICU_BUILD_TIME_VERSION environment variable is set, select libraries using ICU build time version.

if CLR_USE_ICU_BUILD_TIME_VERSION environment variable is set,
select libraries using ICU build time version.
@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Dec 6, 2022
@ghost
Copy link

ghost commented Dec 6, 2022

Tagging subscribers to this area: @dotnet/area-system-globalization
See info in area-owners.md if you want to be subscribed.

Issue Details

if CLR_USE_ICU_BUILD_TIME_VERSION environment variable is set, select libraries using ICU build time version.

Author: wscho77
Assignees: -
Labels:

area-System.Globalization, community-contribution

Milestone: -

@wscho77
Copy link
Contributor Author

wscho77 commented Dec 6, 2022

##79251 (comment)

@am11
Copy link
Member

am11 commented Dec 6, 2022

This is very niche use case. If you know that you have the exact ICU version installed as the runtime build (which is not very likely, as the official build takes place on a CentOS7 machine for compat), you can already use CLR_ICU_VERSION_OVERRIDE=<that version> without introducing another environment variable.

@wscho77
Copy link
Contributor Author

wscho77 commented Dec 6, 2022

This is very niche use case. If you know that you have the exact ICU version installed as the runtime build (which is not very likely, as the official build takes place on a CentOS7 machine for compat), you can already use CLR_ICU_VERSION_OVERRIDE=<that version> without introducing another environment variable.

One source code can be built on the multiple build environment, and the ICU version can be different for each build environment.
As you can see in this patch, build time ICU version can be obtain with "U_ICU_VERSION_MAJOR_NUM" value in the source code.
This is a different case against "CLR_ICU_VERSION_OVERRIDE".

If you set environment "CLR_USE_ICU_BUILD_TIME_VERSION=true", this patch works.

@am11
Copy link
Member

am11 commented Dec 6, 2022

It is the case when we know that the system has the same version installed as the runtime build, right? If so, then we should also be able to find out which version it was (e.g. in a distro's manifest file) and we can set the existing CLR_ICU_VERSION_OVERRIDE to that version for quick lookup.

@wscho77
Copy link
Contributor Author

wscho77 commented Dec 6, 2022

the case when we know that the system has the same version installed as the runtime build, right? If so, then we should also be able to find out which version it was (e.g. in a distro's manifest file) and we can set the existing CLR_ICU_VERSION_OVERRIDE to that version for quick lookup.

Mistakes can be occur if we manually specify the ICU version every time when the build environment changes.
I just want to simple and automatical way to set ICU version :)

How about follow Android case? It try to load ICU without version number. (

static int FindICULibs(char* symbolName, char* symbolVersion)
).
Linux also provides unversioned ibrary (symlink).

@tarekgh
Copy link
Member

tarekgh commented Dec 6, 2022

@janvorli @jkotas the changes here look reasonable to me. Do you have any feedback for that?

@tarekgh tarekgh added this to the 8.0.0 milestone Dec 6, 2022
@janvorli
Copy link
Member

janvorli commented Dec 6, 2022

Linux also provides unversioned ibrary

This is only the build time library (from a dev package), not the runtime one. The runtime libraries are always versioned.

As for this PR, how about allowing the existing CLR_ICU_VERSION_OVERRIDE to be set also to "build" string, check for it in the FindLibUsingOverride before we start parsing the env var value for the version components and use the build time version in this case? That would not require introducing yet another env var and it seems it would logically make sense, as it is an override too.

@wscho77
Copy link
Contributor Author

wscho77 commented Dec 6, 2022

check for it in the FindLibUsingOverride before we start parsing the env var value for the version components and use the build time version in th

It would be good to use "CLR_ICU_VERSION_OVERRIDE" environment variable. I'll make a patch for it soon.

If "build" is set to CLR_ICU_VERSION_OVERRIDE environment variable,
use build time ICU version.
@wscho77
Copy link
Contributor Author

wscho77 commented Dec 6, 2022

@janvorli, @tarekgh I add a patch to add "build" value for CRL_ICU_VERSION_OVERRIDE".
Please review this patch.

Copy link
Member

@tarekgh tarekgh left a comment

Choose a reason for hiding this comment

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

LGTM.

@wscho77
Copy link
Contributor Author

wscho77 commented Dec 11, 2022

@tarekgh The failed checks are not related to this patch.
What should I do to pass checks?

@tarekgh tarekgh merged commit cfdf0f6 into dotnet:main Dec 11, 2022
@tarekgh
Copy link
Member

tarekgh commented Dec 11, 2022

@wscho77 thanks for your contribution here.

@ghost ghost locked as resolved and limited conversation to collaborators Jan 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Globalization community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants