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

grpc: 1.47.0 -> 1.48.0 #182157

Merged
merged 3 commits into from
Jul 30, 2022
Merged

grpc: 1.47.0 -> 1.48.0 #182157

merged 3 commits into from
Jul 30, 2022

Conversation

marsam
Copy link
Contributor

@marsam marsam commented Jul 20, 2022

Description of changes

https://github.com/grpc/grpc/releases/tag/v1.48.0

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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/)
  • 22.11 Release Notes (or backporting 22.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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@marsam
Copy link
Contributor Author

marsam commented Jul 20, 2022

@GrahamcOfBorg build bear sysdig libtensorflow

@marsam marsam requested a review from smancill July 27, 2022 00:00
@smancill
Copy link
Contributor

@ofborg build google-cloud-cpp

@marsam marsam merged commit 63a717a into NixOS:master Jul 30, 2022
@marsam marsam deleted the update-grpc branch July 30, 2022 23:15
@vcunat
Copy link
Member

vcunat commented Aug 2, 2022

This broke some tests in arrow-cpp (and transitively relatively many package builds). I don't know why. https://hydra.nixos.org/build/185903212

@vcunat
Copy link
Member

vcunat commented Aug 2, 2022

/cc arrow-cpp maintainers: @tobim, @veprbl, @cpcloud.

@samuela
Copy link
Member

samuela commented Aug 4, 2022

Created #185082 prior to seeing @vcunat's comments.

@marsam Could you submit a fix for arrow-cpp or revert this PR? In the future, please test all PRs with nixpkgs-review prior to merging.

@cpcloud
Copy link
Contributor

cpcloud commented Aug 4, 2022

I'm working on updating arrow-cpp to 9.0.0 and it looks like the tests pass with grpc 1.48.0. I'm still trying to work out some new dependency discovery failures, but we should be able avoid a revert here.

@cpcloud
Copy link
Contributor

cpcloud commented Aug 4, 2022

#185124

@SuperSandro2000
Copy link
Member

First of all we should consider adding arrow-cpp to passthru tests of grpc if it easily breaks by updates and is an important consumer.

Also the changelog does not mention any breaking changes I could find.

Could you submit a fix for arrow-cpp or revert this PR?

First of all: We are not going to start to hastily revert update because of single downstream failures. The fallout must be big/catastrophically or fixes require unjustifiable amounts of work on our side. If we can fix it by adding a patch from upstream then this is the preferred solution. In any way we should consider multiple solutions before reverting. If that means a package does not build for 5 days then so be it, that's okay and we will manage. master is not a branch where no (temporary) breakages are allowed to exist.

When updating packages that are dependencies of many other packages then we must be careful if there are breaking changes. If we notice those early we should already include fixes in the update PR. If we notice those later we should do follow ups. If we immediately notice that the fallout is very big than a revert can be considered especially if the update is not very important.

But it is not your responsibility to fix all (last percent(s)) downstream packages. Otherwise no one could or would want to maintain core packages because it also draws in partly maintenance of 100 or sometimes 1000 downstream packages.

@jonringer
Copy link
Contributor

First of all we should consider adding arrow-cpp to passthru tests of grpc if it easily breaks by updates and is an important consumer.

I think that's the right path moving forward.

@risicle
Copy link
Contributor

risicle commented Aug 5, 2022

because of single downstream failures

arrow-cpp has around 500 reverse dependencies of its own, so a "single failure" isn't really a single failure.

But it is not your responsibility to fix all (last percent(s)) downstream packages. Otherwise no one could or would want to maintain core packages because it also draws in partly maintenance of 100 or sometimes 1000 downstream packages.

Core packages with this number of rebuilds should be merged to staging, not master. The lack of complete testing is compensated for by the opportunity to make fixes before breakage affects development of the >90% of PRs that target master.

(packages with ~500 rebuilds live in an uncomfortable hinterland of course)

@samuela
Copy link
Member

samuela commented Aug 5, 2022

arrow-cpp has around 500 reverse dependencies of its own, so a "single failure" isn't really a single failure.

Agreed, if a PR targets master it shouldn't break any downstream packages. That should be uncontroversial.

risicle added a commit to risicle/nixpkgs that referenced this pull request Aug 5, 2022
reverse dependency is sensitive to changes as exhibited by
NixOS#182157
@veprbl
Copy link
Member

veprbl commented Aug 6, 2022

because of single downstream failures

arrow-cpp has around 500 reverse dependencies of its own, so a "single failure" isn't really a single failure.

Thankfully, it doesn't!

@samuela
Copy link
Member

samuela commented Aug 6, 2022

How many reverse transitive dependencies does grpc have? 500 seems realistic to me, but I don't know the exact number

@smancill
Copy link
Contributor

smancill commented Aug 6, 2022

What happens when a new version of gRPC breaks Arrow again, if #185329 is merged? What's the process to follow? gRPC would not be able to be updated until Arrow is updated? What if the Arrow update still doesn't work with that gRPC version?

@marsam
Copy link
Contributor Author

marsam commented Aug 6, 2022

Could you submit a fix for arrow-cpp or revert this PR? In the future, please test all PRs with nixpkgs-review prior to merging.

Sorry about that. I think #185124 is going to be merged in the next couple of days, so I'll wait that instead of reverting this PR.

I maintain grpc because I use it in a hobby project, I'm not getting paid for it.
I know grpc is a core and I do try to fix any accidental breakage. See:
#146580 (comment)
#137055 (comment)
but a couple of months ago my health deteriorated and I'm still recovering.
Apologies if I'm a bit slow to respond.

@SuperSandro2000
Copy link
Member

Don't worry. We'll figure it out and the next time it breaks we just take a look what can be done to fix it.

@veprbl
Copy link
Member

veprbl commented Aug 6, 2022

@marsam Please take care of yourself first. For better or for worse, Nixpkgs can't ensure that for you.

@samuela
Copy link
Member

samuela commented Aug 6, 2022

@marsam Thanks for all the work that you do! I'm sorry to hear about your health, and best wishes for a swift recovery!

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.

9 participants