Skip to content

[release/8.0-staging] Add support for more libicu versions #115378

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

Conversation

NikolaMilosavljevic
Copy link
Member

Fixes: #115274

Debian Trixie package repository was updated to include just libicu76. We require libicu72.

Besides version 76, this PR also adds versions 77 and 78, to limit the need to change this file often.

Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds support for additional ICU library versions to accommodate changes in the Debian Trixie package repository.

  • Introduces ICU versions 78, 77, and 76 into the KnownLibIcuVersion list
  • Extends the dependency matching to reduce future maintenance effort

@NikolaMilosavljevic NikolaMilosavljevic added the Servicing-consider Issue for next servicing release review label May 7, 2025
@am11
Copy link
Member

am11 commented May 7, 2025

It would be nice to devise a more flexible scheme matching its native counterpart:

#define MinICUVersion 60
#define MaxICUVersion (U_ICU_VERSION_MAJOR_NUM + 30)

Max version is relative to the libicu version available on build machine.

@NikolaMilosavljevic NikolaMilosavljevic changed the title Add support for more libicu versions [release/8.0-staging] Add support for more libicu versions May 7, 2025
@NikolaMilosavljevic
Copy link
Member Author

It would be nice to devise a more flexible scheme matching its native counterpart:

#define MinICUVersion 60
#define MaxICUVersion (U_ICU_VERSION_MAJOR_NUM + 30)

Max version is relative to the libicu version available on build machine.

Yeah, it would be great to not touch this again. I'll create an issue to track the long-term fix.

@ericstj ericstj requested a review from tarekgh May 8, 2025 17:27
@@ -6,7 +6,7 @@
<ItemGroup>
<LinuxPackageDependency Include="libc6;libgcc1;libgssapi-krb5-2;libstdc++6;zlib1g"/>
<LinuxPackageDependency Include="libssl1.0.0 | libssl1.0.2 | libssl1.1 | libssl3" />
<KnownLibIcuVersion Include="74;72;71;70;69;68;67;66;65;63;60;57;55;52" />
<KnownLibIcuVersion Include="78;77;76;74;72;71;70;69;68;67;66;65;63;60;57;55;52" />
Copy link
Member

Choose a reason for hiding this comment

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

78

If we already adding non -released versions, why can't add more version like 79, 80, 81, 81 ax example?

Copy link
Member

Choose a reason for hiding this comment

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

I gave a similar suggestion above, @NikolaMilosavljevic will track it separately. I think there we could also revisit if we need the version guard any longer, and consider switching to feature detection.

Copy link
Member Author

Choose a reason for hiding this comment

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

Future improvement is now tracked in #115407

@NikolaMilosavljevic
Copy link
Member Author

This was approved for servicing - updating the label.

@NikolaMilosavljevic NikolaMilosavljevic added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels May 8, 2025
@NikolaMilosavljevic NikolaMilosavljevic merged commit 4a0fef3 into dotnet:release/8.0-staging May 8, 2025
100 of 103 checks passed
@NikolaMilosavljevic NikolaMilosavljevic added this to the 8.0.x milestone May 8, 2025
@github-actions github-actions bot locked and limited conversation to collaborators Jun 8, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants