https://pantsbuild.org/ logo
Powered by
e

enough-analyst-54434

10/14/2019, 2:04 PM
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

average-vr-56795

10/14/2019, 2:04 PM
Yeah, we managed to get back onto upstream 
grpc/grpc
, feel free to delete 🙂
e

enough-analyst-54434

10/14/2019, 2:05 PM
Yay! Thanks!
🎉 1
a

average-vr-56795

10/14/2019, 2:05 PM
FWIW we’re now pretty far behind pingcap/grpc-rs 😞
e

enough-analyst-54434

10/14/2019, 2:05 PM
Although - fwict we did not go back upstream. We use forked pingcap/grpc-rs which uses its own forked grpc/grpc
a

average-vr-56795

10/14/2019, 2:06 PM
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

enough-analyst-54434

10/14/2019, 2:07 PM
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

average-vr-56795

10/14/2019, 2:08 PM
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

witty-crayon-22786

10/14/2019, 3:59 PM
and also, it's called Tonic now
a

average-vr-56795

10/14/2019, 4:06 PM
… Is that why I have an urge for gin right now?
e

early-needle-54791

10/14/2019, 9:22 PM
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

enough-analyst-54434

10/14/2019, 9:24 PM
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

early-needle-54791

10/14/2019, 9:26 PM
I'm trying to figure out why exactly cargo-fmt is pulling that lib
e

enough-analyst-54434

10/14/2019, 9:31 PM
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

early-needle-54791

10/14/2019, 9:32 PM
alright thanks
e

enough-analyst-54434

10/14/2019, 9:34 PM
Tracking here: https://github.com/pantsbuild/pants/issues/8473 I'll inform general since this is a non-revertable master break.
w

witty-crayon-22786

10/14/2019, 9:40 PM
So, I think that clones of these repositories end up in the cargo cache
Do we need a copy of the repo?
e

enough-analyst-54434

10/14/2019, 9:41 PM
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

witty-crayon-22786

10/14/2019, 9:45 PM
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

enough-analyst-54434

10/14/2019, 9:48 PM
I think @average-vr-56795 is our main hope here.
w

witty-crayon-22786

10/14/2019, 9:48 PM
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

enough-analyst-54434

10/14/2019, 9:49 PM
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

witty-crayon-22786

10/14/2019, 9:52 PM
i have 9dd357da5a2f231ce254d8abdd46068198637beb
but it's not tagged with anything
e

early-needle-54791

10/14/2019, 9:52 PM
I have the cached grpc submodule @ 3b1b1a7dc0affae281d74ca70f17631d7fe81bc6
which is daniels commit to update boringssl
e

enough-analyst-54434

10/14/2019, 9:52 PM
Excellent - that's what is needed. Just a sec for a place to push.
Thanks @early-needle-54791!
e

early-needle-54791

10/14/2019, 9:53 PM
👍
w

witty-crayon-22786

10/14/2019, 9:54 PM
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

enough-analyst-54434

10/14/2019, 9:55 PM
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

early-needle-54791

10/14/2019, 9:58 PM
cool I will give that a go
w

witty-crayon-22786

10/14/2019, 9:59 PM
rs-release should be == 3b1b1a7dc0affae281d74ca70f17631d7fe81bc6 ?
e

enough-analyst-54434

10/14/2019, 10:00 PM
That agrees with Henry above - thanks!
w

witty-crayon-22786

10/14/2019, 10:00 PM
i think i can push that as well if needed.
e

enough-analyst-54434

10/14/2019, 10:00 PM
If either of you could do it - that would be great.
w

witty-crayon-22786

10/14/2019, 10:00 PM
@early-needle-54791: i just started pushing
e

early-needle-54791

10/14/2019, 10:00 PM
ok
I had an auth issue
w

witty-crayon-22786

10/14/2019, 10:01 PM
oh, hm. it says that it would not be a fast forward.
e

enough-analyst-54434

10/14/2019, 10:01 PM
--force is fine
w

witty-crayon-22786

10/14/2019, 10:01 PM
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

enough-analyst-54434

10/14/2019, 10:01 PM
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

early-needle-54791

10/14/2019, 10:03 PM
thanks for the fix
that build was bad anyway
e

enough-analyst-54434

10/14/2019, 10:04 PM
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

early-needle-54791

10/14/2019, 10:07 PM
i did both lol. I have a new build running but its still bootstrapping
e

enough-analyst-54434

10/14/2019, 10:15 PM
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

witty-crayon-22786

10/14/2019, 10:16 PM
indeed. Daniel got so close to switching to Tower. would be amazing to see it happen.
e

early-needle-54791

10/14/2019, 10:16 PM
yeah...never great fun to break master, thanks for the timely fix
w

witty-crayon-22786

10/14/2019, 10:17 PM
it's a definite tech-debty issue that would be good to put someone on this fall
e

enough-analyst-54434

10/14/2019, 10:17 PM
Agreed.
a

average-vr-56795

10/14/2019, 10:18 PM
Daniel got so close to switching to Tower
I even switched! (I just gave up and switched back 😛)
Powered by