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

Refactor icu and init 74.2 #290266

Merged
merged 2 commits into from
Feb 21, 2024
Merged

Refactor icu and init 74.2 #290266

merged 2 commits into from
Feb 21, 2024

Conversation

afh
Copy link
Member

@afh afh commented Feb 20, 2024

ℹ️ This draft PR is my understanding of how the icu package could be improved in a similar vein as #289690.

TODO

If this PR is headed in the right direction and people see value in pursuing this approach close the following PRs

Description of changes

  • Add icu/default.nix containing the icu version specific packages
  • Remove icu/[0-9]*.nix
  • Use hash instead of sha256 in icu/base.nix
  • ❗Includes changes proposed in icu73: use buildPackages.icu73 for nativeBuildRoot #289782 icu73: use buildPackages.icu73 for nativeBuildRoot
  • Update all-packages.nix accordingly using callPackages

I'm uncertain whether the changes proposed here properly affect buildPackages.icu[0-9]{2} or whether more work is needed.
I'd appreciated if someone could chime in on this.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@afh afh mentioned this pull request Feb 20, 2024
15 tasks
@afh afh requested a review from 7c6f434c February 20, 2024 20:42
@7c6f434c
Copy link
Member

OK, speaking about checking for rebuilds: nix-instantiate producing the same derivation before and after means that there is no need to rebuild the derivation, or any of its dependants (also known as reverse-dependencies or rev-deps). Which is quite a bit cheaper to check than doing a rebuild.

Actual icu-originating rebuild would definitely go to staging, you are absolutely correct here. But expression refactoring should typically target zero rebuilds as a goal, and then have some kind of an answer why this goal is not achievable (if it is not achievable). Often any reasonable answer is good enough, though!

(Adding a not-yet-used version of ICU very similar to the previous ones counts separately, and it has low impact )

@7c6f434c 7c6f434c merged commit 9d29d2d into NixOS:master Feb 21, 2024
32 checks passed
@afh
Copy link
Member Author

afh commented Feb 21, 2024

Thanks for elaborating in this, @7c6f434c, seems I have a bit more reading to do 😅

Do you see value in switching the icu package to icu74 for 24.05?

@afh afh deleted the refactor-init-icu branch February 21, 2024 13:21
@afh
Copy link
Member Author

afh commented Feb 21, 2024

Also thanks for merging! 😃

@7c6f434c
Copy link
Member

In general, I ended up maintainer of ICU because too much stuff needs it… so I don't have strong opinions to be honest.

Trying to switch to icu74 and pinning only what needs to be pinned sounds reasonable (also based on CVE track record and on the changelog, we don't want to stay behind and the upgrade has a chance of being smooth). That's obviously a staging-scale thing, and I am not sure how long it would take to test so much stuff…

@afh
Copy link
Member Author

afh commented Feb 21, 2024

I'd be happy to draft a PR; the change is trivial, but as you said the challenge is figuring out what breaks with the update and fixing that.
Anything that needs to happen to prepare for a draft PR? Should more folks be involved to lend their expertise on "scaling staging"?

@7c6f434c
Copy link
Member

I am tempted to say that once the PR against staging is open, we might ask ofBorg to build at least something that has tests and depends on ICU, then ask people most recently having merged staging-next about the recommended procedure here.

The total number of packages caring to pin an ICU version right now looks around 25; direct rev-deps are above 200, and then transitive rev-deps include Qt and GTK so basically all the GUI stuff is dependent.

Usually either the impact is more narrow, or it is deeper (e.g. glibc updates)…

@afh
Copy link
Member Author

afh commented Feb 22, 2024

That sounds reasonable, @7c6f434c. Here goes nothin' #290509 🤞

@7c6f434c
Copy link
Member

… and conveniently it looks like calling the relevant people is happening even before we got around to it ourselves!

@kirillrdy
Copy link
Member

I had to deal with some icu firefox issues myself before :-)

@seanybaggins seanybaggins mentioned this pull request Feb 22, 2024
13 tasks
@uninsane uninsane mentioned this pull request Feb 23, 2024
13 tasks
@afh afh mentioned this pull request Mar 1, 2024
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants