refactor: remove redundant ICU version-detection mechanism#364
Open
clydegerber wants to merge 2 commits into
Open
refactor: remove redundant ICU version-detection mechanism#364clydegerber wants to merge 2 commits into
clydegerber wants to merge 2 commits into
Conversation
Contributor
Author
|
This is the alternative to PR 363. |
Every ICU version this workspace targets in CI (74, 76, 77) is >= the boundary versions referenced by the version-gated cfg features (`icu_version_64_plus`, `icu_version_67_plus`, `icu_version_68_plus`). The gates are always active in practice, so the conditional-compilation mechanism — gates in source code, feature declarations in Cargo.toml, and cfg emissions from `build.rs` — is dead infrastructure. Removes: - Redundant `#[cfg(feature = "icu_version_##_plus")]` annotations and their companion `not(...)` dead-code branches from rust_icu_ecma402, rust_icu_ulistformatter, rust_icu_unumberformatter, and rust_icu_uloc (including the pre-ICU-67 buggy-behavior test variant). - The `icu_version_64_plus`, `icu_version_67_plus`, `icu_version_68_plus`, and `icu_version_69_max` Cargo feature declarations across 27 manifests. - The cfg-emission blocks in `rust_icu_sys/build.rs` and `rust_icu_release/src/lib.rs`. - The `_plus` features from the `test-with-features` CI matrix and the `macos-test` Makefile target. This is a breaking change to the public Cargo feature surface; the next release will already require a major version bump for other reasons. Rewrites the CONTRIBUTING.md section as forward-looking guidance for re-introducing version-dependent code if it becomes necessary. This commit was created by an automated coding assistant, with human supervision.
This was referenced Jun 1, 2026
filmil
requested changes
Jun 1, 2026
Member
filmil
left a comment
filmil
left a comment
There was a problem hiding this comment.
The changes here are mostly removal of now-unneeded settings. Nice!
|
|
||
| ## Adding ICU-version-dependent code | ||
|
|
||
| The workspace currently targets ICU versions for which all wrapped APIs are |
Member
There was a problem hiding this comment.
This assertion is incorrect, the main version issues are differences in test outcomes.
| ### 1. The Cargo feature | ||
|
|
||
| Declare a feature in each affected crate's `Cargo.toml`, named after the | ||
| boundary version (e.g. `icu_version_80_plus`). Propagate it to dependent |
Member
There was a problem hiding this comment.
Consider using a version number that exists.
Per review feedback: version gating in this workspace has historically been about behavior differences between ICU versions (locale data updates, behavioral bug fixes that change output), not about API availability. Updates the example to use a real ICU version (`icu_version_78_plus`) instead of a hypothetical one. This commit was created by an automated coding assistant, with human supervision.
clydegerber
force-pushed
the
refactor/remove-redundant-version-gates
branch
from
992ab55 to
5f91005
Compare
Contributor
Author
|
Thanks for the review! On the first comment — you're right, version gating has historically been about behavior differences (locale data updates, bug fixes that change output) rather than API availability. Reworded the introduction to reflect that. On the second — changed the example from Pushed in 5f91005. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The workspace's minimum-supported ICU version is 74 (per the CI matrix: 74, 76, 77), so the
icu_version_64_plus,icu_version_67_plus, andicu_version_68_plusgates are always active in practice. This PR removes them entirely along with the supporting infrastructure.Removes:
#[cfg(feature = \"icu_version_##_plus\")]annotations and their companionnot(...)dead-code branches fromrust_icu_ecma402,rust_icu_ulistformatter,rust_icu_unumberformatter, andrust_icu_uloc, including the pre-ICU-67 buggy-behavior test variant.icu_version_64_plus,icu_version_67_plus,icu_version_68_plus, andicu_version_69_maxCargo feature declarations across 27 manifests. (icu_version_69_maxwas already a ghost — declared widely but never consumed by any source code.)rust_icu_sys/build.rsandrust_icu_release/src/lib.rs._plusfeatures from thetest-with-featuresCI matrix and themacos-testMakefile target.Rewrites the CONTRIBUTING.md section as forward-looking guidance for re-introducing version-dependent code if it becomes necessary again.
Alternative approach considered
An alternative would be to keep the version-gated code intact and add
build.rsfiles to the few crates that lack auto-detection of the ICU version (rust_icu_ecma402,rust_icu_ulistformatter,rust_icu_unumberformatter). That would preserve the public Cargo feature surface but leaves dead conditional-compilation in place — every supported ICU version satisfies the gates, so thenot(...)branches are unreachable and the_plusfeatures are activated 100% of the time. Removing the mechanism outright eliminates the dead code and simplifies the build graph; if a future ICU version ever introduces a real boundary, the pattern can be re-introduced (see CONTRIBUTING.md).Breaking change
This removes Cargo features from the public surface of every wrapper crate. The next release already requires a major version bump for unrelated reasons (the recent
PartialEqderive removals from bindgen-generated types and theText::try_fromownership-behavior change), so this fits the breaking-changes batch.Test plan
cargo testpasses on default features (217 tests)test-with-featuresmatrix passes with the updated feature listThis commit was created by an automated coding assistant, with human supervision.