refactor: remove redundant ICU version-detection mechanism#364
refactor: remove redundant ICU version-detection mechanism#364clydegerber wants to merge 2 commits into
Conversation
|
This is the alternative to PR 363. |
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!
clydegerber
force-pushed
the
refactor/remove-redundant-version-gates
branch
from
992ab55 to
5f91005
Compare
|
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. |
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.
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
5f91005 to
78b6758
Compare
|
@filmil - can this be merged? I believe I've addressed all comments... |
Noted - please resolve the open comments explicitly. I check the top level PR table, which shows unresolved comments, and I skip them. I apologize, but I can't always give attention to a bug unless I see a top level change. |
@filmil - my apologies, I've resolved the open comments. |
2 participants
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.