icu4c: fix #20954: icu_54_1 package does not compile #20955
Conversation
|
@savannidgerinel, thanks for your PR! By analyzing the history of the files in this pull request, we identified @k0001, @grahamc and @chris-martin to be potential reviewers. |
|
Looks good to me. Whether keeping icu54 building is a good idea is another matter (last I looked it's known to be vulnerable). It's only used by the zoom.us client, however, so it doesn't make too much of a difference. |
|
For what it's worth, I was able to run zoom-us with icu-57.1 and could discern no difference in behaviour vs icu-54.1. |
|
Ah, cool, then I suggest removing icu54 instead of fixing it :) If you care to, you could open a second PR, leaving this one open in case somebody disagrees with me. |
|
I'm 👍 on dumping icu_54_1, seeing as it is vulnerable / out of date, and a maintenance burden. |
|
Should I create a different bug for dumping icu_54_1, or keep it in this branch under this particular bug? (Solution for icu_54_1 doesn't build: dump icu_54_1) |
|
In the interest of keeping things simple, let's just go ahead and remove icu54. Generally, if there are alternative solutions, separate PRs would make sense. (Note: it's fine to create PRs without having a corresponding issue). |
|
savannidgerinel <notifications@github.com> writes:
Should I create a different bug for dumping icu_54_1, or keep it in
this branch under this particular bug? (Solution for icu_54_1 doesn't
build: dump icu_54_1)
You can reuse this branch / PR I think, just make sure to squash your
commits in to a single commit.
|
Only zoom-us depends on icu4c-54.1. Since we know that version has some vulnerabilities, and zoom-us appears to work with icu4c-57.1, I remove the icu/54.1.nix file, remove icu_54_1 from all-packages.nix, and have zoom-us depend on icu (i.e., icu4c-57.1)
|
Thank you |
Motivation for this change
icu4c-54.1 doesn't actually need (and cannot accept) the patches that icu4c-57.1 needs. Instead of fixing it, since we know that icu4c-54.1 is vulnerable and tests show that only zoom-us depends on it and that zoom-us seems to work with icu4c-57.1, we simply remove that older version and make zoom-us depend on the newer one.
Things done
(nix.useSandbox on NixOS,
or option
build-use-sandboxinnix.confon non-NixOS)
nix-shell -p nox --run "nox-review wip"./result/bin/)