https://pantsbuild.org/ logo
Join Slack
Powered by
I'm working a c-compilation issue in the core deep...
# development
e
I'm working a c-compilation issue in the core deep down in grpc (https://github.com/pantsbuild/pants/issues/8472) and I found we have a fork of grpc/grpc but don't actually use it - afaict. OK to delete our fork? I expect you know the answer to this most intimately @average-vr-56795.
a
Yeah, we managed to get back onto upstream 
grpc/grpc
, feel free to delete 🙂
e
Yay! Thanks!
🎉 1
a
FWIW we’re now pretty far behind pingcap/grpc-rs 😞
e
Although - fwict we did not go back upstream. We use forked pingcap/grpc-rs which uses its own forked grpc/grpc
a
Maybe we should try switching over to Tower again…
Yeah, we’re still on pingcap/grpc-rs because I couldn’t find a reasonable testcase for https://github.com/pingcap/grpc-rs/pull/211
e
OK - thanks for all the context. Since grpc-rs is way behind on its own fork of grpc/grpc I may try my hand at re-towering.
a
Let me share my lessons from the last towering!
1. It’s a much nicer API, oh my so much nicer 2. The only really annoying bit about doing it is mechanically updating all the tests which have protos in them. If you ignore the tests, the changes to get the code compiling are pretty small. 3. I initially did an 
Into
impl for each proto type so I could just sprinkle around 
.into()
calls in tests, and then casually do actual re-writing separately. I got bored because nested protos made it more hassle than it was worth. 4. The reason we reverted it was we saw the channel to the execution server occasionally drop and then further attempts to use it unconditionally fail. I don’t know whether grpc-rs has some internal retries or something, or tower needs keepalives, or just to retry connections or whatever. I couldn’t repro locally or in tests, only in production 😞
w
and also, it's called Tonic now
a
… Is that why I have an urge for gin right now?
e
has something been done here. Getting this error in the CI lint shard:
Copy code
error: failed to load source for a dependency on `grpcio`
Caused by:
  Unable to update <https://github.com/pantsbuild/grpc-rs.git?rev=9dd357da5a2f231ce254d8abdd46068198637beb#9dd357da>
Caused by:
  failed to update submodule `grpc-sys/grpc`
Caused by:
  failed to fetch submodule `grpc-sys/grpc` from <https://github.com/pantsbuild/grpc.git>
Caused by:
  failed to authenticate when downloading repository
attempted to find username/password via git's `credential.helper` support, but failed
Caused by:
  failed to acquire username/password from local configuration
Error during execution of `cargo metadata`: error: failed to load source for a dependency on `grpcio`
Caused by:
  Unable to update <https://github.com/pantsbuild/grpc-rs.git?rev=9dd357da5a2f231ce254d8abdd46068198637beb#9dd357da>
Caused by:
  failed to update submodule `grpc-sys/grpc`
Caused by:
  failed to fetch submodule `grpc-sys/grpc` from <https://github.com/pantsbuild/grpc.git>
Caused by:
  failed to authenticate when downloading repository
attempted to find username/password via git's `credential.helper` support, but failed
Caused by:
  failed to acquire username/password from local configuration
What I mean is has something about this repo been changed permissions wise? This error does not seem transient
e
Nope - nothing yet and this error looks unrelated to the error I was seeing which is detailed in https://github.com/pantsbuild/pants/issues/8472 but is a dup-conflicting-function declaration.
Aha both @average-vr-56795 and I were wrong, somehow our grpc fork is used:
Copy code
Caused by:
  failed to fetch submodule `grpc-sys/grpc` from <https://github.com/pantsbuild/grpc.git>
I deleted that fork today! Looking...
e
I'm trying to figure out why exactly cargo-fmt is pulling that lib
e
Thanks. That's definitely the problem. I was browsing our grpc-rs fork at master HEAD, we use https://github.com/pantsbuild/grpc-rs/tree/9dd357da5a2f231ce254d8abdd46068198637beb/grpc-sys
That version does point the submodule at our fork 😕 ... gimme a bit.
e
alright thanks
e
Tracking here: https://github.com/pantsbuild/pants/issues/8473 I'll inform general since this is a non-revertable master break.
w
So, I think that clones of these repositories end up in the cargo cache
Do we need a copy of the repo?
e
Yes
I wiped my cargo cache in pursuit of tonic / using rust beta in pants.
I guess I can probably grab one from the travis cache.
Trying that...
w
Ok, looking
can look in 
~/.cache/pants/rust/cargo/git/checkouts/
i've got some that i can push places
Copy code
$ ls -1 ~/.cache/pants/rust/cargo/git/checkouts/
futures-timer-657d237f95a485bf
grpc-rs-b50c79747a664460
h2-5627e19d2c95dbee
lmdb-rs-369bfd26153a2575
rust-protobuf-8e8a2cd87e934b3d
string-8a0320b15d60e666
termion-994101b84ecd1aab
tokio-connect-0fb9e2e939118ab0
tokio-connect-35101edd25624c88
tower-20b7cbb22dd35c99
tower-b098c32cf5a1bcca
tower-grpc-6de8565ab246a0cf
tower-h2-7c0484bd51f493bd
tower-h2-aaf68b3c8982de4c
tower-http-243b30f79c7df9d4
tower-http-8b33967e083bb0de
...oh. dang. it looks like it's a bare/shallow checkout. shoot
e
I think @average-vr-56795 is our main hope here.
w
oh! no, it's just nested.
example: 
~/.cache/pants/rust/cargo/git/checkouts/grpc-rs-b50c79747a664460/4dfafe9
@enough-analyst-54434: can i push some things somewhere?
e
Yes - just a sec
So we need pantsbuild/grpc at tip of branch rs-release.
Neither the right grpc-rs nor grpc are in your list above..
w
i have 9dd357da5a2f231ce254d8abdd46068198637beb
but it's not tagged with anything
e
I have the cached grpc submodule @ 3b1b1a7dc0affae281d74ca70f17631d7fe81bc6
which is daniels commit to update boringssl
e
Excellent - that's what is needed. Just a sec for a place to push.
Thanks @early-needle-54791!
e
👍
w
mm, yea, i have that as a submodule as well.
i'll attach this whole subdirectory, because i have to walk out the door.
e
I'll just add henry as a committer to the new fork for a bit.
@early-needle-54791 if you can push to https://github.com/pantsbuild/grpc on the rs-release branch, that would be great.
e
cool I will give that a go
w
rs-release should be == 3b1b1a7dc0affae281d74ca70f17631d7fe81bc6 ?
e
That agrees with Henry above - thanks!
w
i think i can push that as well if needed.
e
If either of you could do it - that would be great.
w
@early-needle-54791: i just started pushing
e
ok
I had an auth issue
w
oh, hm. it says that it would not be a fast forward.
e
--force is fine
w
Copy code
$ git push -f pantsbuild rs-release
Counting objects: 5131, done.
Delta compression using up to 4 threads.
Compressing objects: 100% (1738/1738), done.
Writing objects: 100% (5131/5131), 813.95 KiB | 0 bytes/s, done.
Total 5131 (delta 3939), reused 4445 (delta 3298)
remote: Resolving deltas: 100% (3939/3939), completed with 691 local objects.
To git@github.com:pantsbuild/grpc.git
 + 4158bd0...3b1b1a7 rs-release -> rs-release (forced update)
e
I just forked the pingcap/grpc so its much newer than when @average-vr-56795 forked however long ago.
Thanks!
OK - @early-needle-54791 your CI job should now work
I just restarted your example shard https://travis-ci.org/pantsbuild/pants/jobs/597831968
e
thanks for the fix
that build was bad anyway
e
Hopefully rustfmt at least can run - it'll be good for testing that anyhow,
Wait - it got cancelled. Do you have a replacement shard we can watch?
ok - nm - someone re-restarted it
e
i did both lol. I have a new build running but its still bootstrapping
e
Alright - I think we're good to go. Your shard is now compiling bazel-protos which depends on the problem area.
Thanks @witty-crayon-22786 and @early-needle-54791. Not fun.
w
indeed. Daniel got so close to switching to Tower. would be amazing to see it happen.
e
yeah...never great fun to break master, thanks for the timely fix
w
it's a definite tech-debty issue that would be good to put someone on this fall
e
Agreed.
a
Daniel got so close to switching to Tower
I even switched! (I just gave up and switched back 😛)
Open in Slack
PreviousNext
Powered by