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

[24.05] backport gradle: 8.7 → 8.8 #338608

Conversation

siddarthkay
Copy link
Contributor

Description of changes

PR to backport #316849 to 24.05 release

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.11 Release Notes (or backporting 23.11 and 24.05 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.

(cherry picked from commit 48d567f)
Copy link
Member

@Atemu Atemu left a comment

Choose a reason for hiding this comment

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

I am not convinced this should be backported.

This appears to be quite a major version and it's not clear to me whether it's bug-compatible with 8.8 outside of the bugs that were explicitly fixed.

Could you explain why this would need to be backported? Are there any critical bugs or security issues fixed by this release that stable users cannot be expected to bear for another 2 months?

@siddarthkay
Copy link
Contributor Author

@Atemu : My motivations behind this backport is upgrading react-native to version 0.75 and react-native requires gradle 8.8
ref -> https://github.com/facebook/react-native/blob/a276df4d2a2c7c658bdf892d121b44886ba22202/packages/helloworld/android/gradle/wrapper/gradle-wrapper.properties#L3

We use nixpkgs release 23-11 and are currently planning to upgrade to nixpkgs release 23-04 release.
Hence the backprort PR.
ref -> https://github.com/status-im/status-mobile/blob/97d4edcf30a1b34f48a6bfae80488cf496638bea/nix/pkgs.nix#L9-L14

What are your reasons for blocking this version bump?

@Atemu
Copy link
Member

Atemu commented Sep 1, 2024

Thanks for explaining your needs.

The purpose of the stable channel is to not change significantly (outside of where it is necessary) for its entire support timeframe. If you require new versions of software in <6 months after they become available, you should therefore not use a stable channel.

If you are closer to the bleeding edge than a stable software distribution allows, I'd recommend you to pin a revision of nixpkgs-unstable and bump it frequently. It is the branch where all updates happen as soon as possible (on a best-effort basis) and is arguably better supported than the stable channel but may introduce breaking changes (such as this one) at any time.
For dev purposes, you can simply move your pin whenever it's convenient and, should something break, simply keep the previous pin until the issue is resolved.

The primary reason for blocking this bump is that, as a general purpose software distribution, we need to care for a lot more than react-native functioning; lots of our packages depend on gradle. Glancing over the original PR, there were mentions of at least one dependant packages breaking due to this change and many updates to dependant packages that make them compatible with gradle 8.8 may not have existed before branch-off in May.

The onus is now on you to convince me (or any other person with merge privileges) that this change will not break dependant packages.

On unstable, that bar is quite a bit lower as breaking changes are expected and it's often not logistically possible to figure out whether a change breaks anything.
On stable however, breaking changes should ideally never happen.

In case of doubt (current case), I opt to not let this pass.

As I said though, this backport shouldn't be necessary for your purposes.

If you really must have gradle 8.8 in the stable channel however, it would be permissible to add it as a separate top-level attribute (e.g. gradle_8_8) because the actual problem in this PR is that you're changing the definition of the gradle that other packages depend on. With a separate top-level attribute that no other package depends on, that problem would not exist.
(You could simply copy-paste the gradle package definition in this case. We don't really care about maintainability of stable-only changes as the stable channel maintanence will end in 3 months.)

@siddarthkay
Copy link
Contributor Author

@Atemu : Thanks for explaining your reasoning. I like the idea of gradle_8_8 To prod further on that direction is it necessary for this change to land in master first and then be backported to this release branch or is it okay if i just make that change in this PR?

@Atemu
Copy link
Member

Atemu commented Sep 2, 2024

Master currently carries gradle 8.8 and can generally be expected to carry the newest version quite soon after release at all times (again, best effort), so this doesn't really make sense to do on master. What does often make sense on is the other way around: keeping an older version available.

As I said though, please first consider using nixpkgs-unstable before dealing with backporting stuff to a channel that should change as little as possible.
Don't let the "unstable" scare you, it just means that breaking changes don't happen at coordinated points in time but rather simply as they come. Some unintended breakage can fall through the cracks (slightly more than on stable I'd say) and that can cause clear issues for a week or two at a time at worst but, as I said, you should pin specific revisions anyways and simply wait a few days to a week for such issues to be resolved before you move the pins for real. During that time, everything keeps working with the previous revision of course due to Nix' reproducibility guarantees.

@siddarthkay
Copy link
Contributor Author

Thanks for your inputs @Atemu , I'll try using nixpkgs from one of the unstable channels that have gradle 8.8

@siddarthkay siddarthkay closed this Sep 3, 2024
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.

4 participants