< bitcoin-git>
bitcoin/master f96dbd1 Gregory Sanders: gdb attaching to process during tests has non-sudo solution
< bitcoin-git>
bitcoin/master 4d661ba Wladimir J. van der Laan: Merge #15244: gdb attaching to process during tests has non-sudo solution
< bitcoin-git>
[bitcoin] laanwj merged pull request #15244: gdb attaching to process during tests has non-sudo solution (master...gdb_ptrace) https://github.com/bitcoin/bitcoin/pull/15244
< wumpus>
I mean the obvious answer would be "services that need to know running bitcoind's process id"
< hebasto>
wumpus: why do you suppose that not being able to create the PID file should be fatal?
< wumpus>
because it would be deterministic?
< hebasto>
aah, I see.
< wumpus>
either you make sure the PID file exists when the process is running
< wumpus>
or you don't create one in the first place
< wumpus>
I don't see the intermediate 'launch the process but don't create a PID file' as useful
< wumpus>
but that might be just me :)
< sdaftuar>
wumpus: i concur :)
< hebasto>
wumpus: sdaftuar: thanks
< wumpus>
this is doubly important imo when providing an alternative pid file path with `-pid=...`, because unlike the data directory it might not be a guaranteed-writable location so the user could have made a mistake with permissions
< wumpus>
hebasto: anyhow it's perfectly fine if you think that's out of scope for #15278, it just surprised me
< provoostenator>
wumpus: I can't remember having lost a github comment recently. It gets a little iffy when opening the same PR in two tabs though.
< wumpus>
provoostenator: ooh that might be part of the issue then
< fanquake>
GH probably just collapsing more comments into uselessness
< wumpus>
and that ^^
< provoostenator>
Yeah the collapsing thing is different. That happens because we force-push and rebase a lot.
< provoostenator>
I sometimes have two tabs open because I want to review commit by commit, and when I switch back to the main comment thread it forgets which commit I was looking at.
< fanquake>
also collapsing on similar comments. i.e 2-3 "utACK commit_hash" comments in a row, the last two may get hidden as "similar comments".
< bitcoin-git>
[bitcoin] achow101 opened pull request #15301: tests: When testing with --usecli, unify RPC arg to cli arg conversion and handle dicts and lists (master...fix-tests-cli-args) https://github.com/bitcoin/bitcoin/pull/15301
< wumpus>
fanquake: I've seen that as well :(
< cjd>
lol microsoft, 1 job
< fanquake>
When throwing RPC_DESERIALIZATION_ERROR, for ex when deserializing a tx, is "TX decode failed" or more verbosity preferred, i.e "TX decode failed CDataStream::read(): end of data: unspecified iostream_category error"?
< fanquake>
RPCs currently use a mix of the two.
< wumpus>
fanquake: I personally prefer more detailed error messages where possible
< wumpus>
(though something can be said for the opposite as well, as in "don't leak implementation details")
< wumpus>
note that tomorrow is strings freeze for 0.18
< jnewbery>
promag: I'm going to try to look at that today
< wumpus>
and in two weeks the feature freeze
< promag>
jnewbery: maybe we should postpone multiwallet gui for 0.19? and maybe backport to 0.18.1?
< sdaftuar>
i would like to review beg for #14897 -- in addition to being a useful feature in its own right, it paves the way for several simple transaction download improvements (some of which i'm hoping could land in 0.18)
< gribble>
https://github.com/bitcoin/bitcoin/issues/14897 | randomize GETDATA(tx) request order and introduce bias toward outbound by naumenkogs · Pull Request #14897 · bitcoin/bitcoin · GitHub
< jamesob>
will take a look today (fwiw)
< wumpus>
sdaftuar: that one is already in high prio I think?
< sdaftuar>
wumpus: yep
< sdaftuar>
i think it's nearly ready (the nits i left on the PR could be dealt with later)
< sdaftuar>
but needs more review
< wumpus>
okay, great!
< jnewbery>
#11082 has required rebase for 10 days and has outstanding review comments from December. Should it be removed from high priority?
< gribble>
https://github.com/bitcoin/bitcoin/issues/11082 | Add new bitcoin_rw.conf file that is used for settings modified by this software itself by luke-jr · Pull Request #11082 · bitcoin/bitcoin · GitHub
< provoostenator>
jnewbery: I don't think it's a good idea to merge that so close to release, as much as I'd like to have it
< provoostenator>
Also it doesn't really do anything on its own afaik, and the stuff on top of it isn't ready.
< wumpus>
I tend to agree
< wumpus>
okay, going to remove it
< provoostenator>
As for multiwallet: it would be nice to get opening a wallet in
< provoostenator>
But not end of the world if not.
< jnewbery>
I agree with trying to get multiwallet open into v0.18
< jonasschnelli>
Agree. Open wallet would be nice.
< promag>
I know I have one HP PR, but it depends on #15280 so if you don't mind that could be in the list too
< sdaftuar>
if we're thinking about pruning from the high priority list to focus on 0.18, then i think #15141 could be removed as well. it's ready for review but not essential for 0.18 IMO (and maybe we'd want it to simmer in master for longer before a release anyway)
< wumpus>
sdaftuar: nah not necessarily! it's just that if PRs have outstanding comments for a long time, and are not being updated, there's not that much urgency apparently
< provoostenator>
Or does it explode?
< wumpus>
provoostenator: idieally it would be avoided completely, but yes that's the effect
< wumpus>
provoostenator: I'm okay with *adding* strings after that if we can't avoid it, but not changing them
< sdaftuar>
wumpus: ok happy to keep it on there too, it's holding up other work i have going on! we can revisit as we get closer to feature freeze i guess
< wumpus>
(e.g. no "improve wording" PRs)
< wumpus>
but yeah it's good to give the translators some time
< luke-jr>
a lot of time seems wasted on rebasing needlessly; I'd like to suggest we only expect rebasing when there's a major conflict, or the PR is literally about to be merged
< luke-jr>
promag: apparently some people consider it a show-stopper on progress for PRs if it "needs" rebase
< wumpus>
people usually want to test PRs on top of master, which is not straightforward if they need rebase, but yea for review it shouldn't strictly be necessary
< wumpus>
it's up to you really
< promag>
I think that's "requested" after trivial review
< meshcollider>
Rebase can often partially invalidate reviews anyway unless its trivial in which case theres no point not doing it
< provoostenator>
Lack of rebase normally won't stop me from reviewing, unless I expect a problem.
< wumpus>
though everything that makes people more willing to review your PR might be welcome given how many there are ...
< provoostenator>
Maybe though we need additional tag "Really Needs Rebase"
< wumpus>
I had it under 242 some weeks ago but ugh
< luke-jr>
well, we have a bot more recently that closes stuff and nags over even trivial rebases
< provoostenator>
Ah that's a good point, maybe Drahtbot should be less aggressive in that regard.
< provoostenator>
Only close if Really Needs Rebase is set? :-)
< wumpus>
drahtbot doesn't close PRs
< jnewbery>
I think Drahtbot only closes if a PR has needed rebase for a _really_ long time
< achow101>
drahtbot will close and reopen PRs to retrigger travis
< wumpus>
it only adds a label "needs rebase" and posts a message in that regard
< jamesob>
doesn't drahtbot only ask for a rebase if there are conflicts?
< meshcollider>
It does after like a very long time and tags it with up for grabs
< meshcollider>
Close PRs ^
< wumpus>
meshcollider: I don't think it does so automatically
< wumpus>
or at least I've never noticed
< jamesob>
drahtbot does close and mark up-for-grabs, e.g. #13200
< meshcollider>
Marco runs an extra script occasionally I think
< wumpus>
though if it's after a very long time I don't mind ...
< provoostenator>
I've seen it happen a few times as well
< wumpus>
there's probaly quite a few PRs that could be closed
< provoostenator>
But often the problem isn't a lack of rebase, it's either a lack of feedback or a lack of addressing feedback.
< wumpus>
sometimes I just want to close them all and start anew xD
< sdaftuar>
luke-jr: i agree with the sentiment you bring up, but its unclear to me how much of the irritation is from being nagged about rebasing, versus the repo activity that is requiring so many rebases in the first place
< jnewbery>
wumpus: I agree. If something's needed rebase for > 6 months then it's clearly not a priority for the contributor. It can always be re-opened if it becames a priority
< jonasschnelli>
also,... there are some PR not meant to be merged (WIP / Experimental)
< wumpus>
jonasschnelli: I think that's great
< provoostenator>
jnewbery: not necessarily, there's no point in rebasing if you're not getting enough concept ACK and agreement on technical direction
< wumpus>
(if they're clearly marked as that)
< jonasschnelli>
Yes. Some PR are to attract developers and spun up new ideas
< provoostenator>
So it could be a priority for the developer, just not for the reviewers.
< jnewbery>
provoostenator: I think Draht only closes if there's been no activity at all. If you're not getting _any_ interaction from other contributors then again, it's probably not a priority for anyone
< wumpus>
so it's a pretty busy repository and a lot of changes are happening, there's nothing to be done about that, it's the same for other popular open source projects
< provoostenator>
Ok, if _any_ activity is fine, then it's not unreasonable to expect e.g. the contributor themselves to give a quick "anybody out there?" update.
< luke-jr>
wumpus: I think it would help, if maintainers indicated they're prepared to merge a PR, and it got rebased specifically for the merge. (assuming reviewers continue to review despite trivial rebases)
< wumpus>
provoostenator: yes, that's fine!
< achow101>
the only issue i have with rebases is that someone would comment "needs rebase", the author rebases, and then receives no reviews until the next "needs rebase" comment from someone
< wumpus>
it's *really* common for a PR to be waiting for *any* response from the author to comments
< wumpus>
even if that's "I prefer not to address this as it's out of scope"
< wumpus>
but you need to reply to comments
< provoostenator>
Yes, one thing that might help is if people are less shy to just take over PRs.
< provoostenator>
achow101 took one over from me pretty quickly, changing it somewhat, which is great, saves me work.
< wumpus>
no one is going to merge a PR with un-addressed comments
< wumpus>
I mean we can prod people to review all we want, if reviews just go ignored there's no point
< provoostenator>
A slightly less drastic alternative to opening an alternative PR is to link to a branch in the comments, but branches are not easy to review.
< provoostenator>
Github, if you're listening, you should add a PR fork feature :-)
< wumpus>
heh
< luke-jr>
it'd be nice if the PR # could stay the same with multiple contributors :P
< jnewbery>
Speaking from personal experience, I've never had much of an issue with rebases. I only ever have a handful of PRs open at maximum, so rebasing isn't too onerous. I think it only becomes a problem if you have a lot of open PRs
< jamesob>
rebasing should be MORE onerous ;)
< wumpus>
it also depends on the kind of PR, if you only have localized changes it's not too bad
< sdaftuar>
some PRs are definitely a pain to rebase
< provoostenator>
It gets exponentially bad if you build multiple PRs on top of each other.
< wumpus>
also means you need to rebase less often because there's less chance of it colliding with other changes
< wumpus>
avoid change-all-over-the-place PRs
< jnewbery>
provoostenator - shouldn't be exponential. In my experience it's sub-linear if the PRs are a series because rebasing the first is often the only actual work.
< jnewbery>
(again, just personal experience. Yours may vary!)
< luke-jr>
I think I have a tendency to rebase once per release, and on the rare occasion someone pings me for a merge
< jonasschnelli>
A rebase quick before a merge is dangerous IMO...
< MarcoFalke>
I agree with what meshcollider said earlier: [14:17] <meshcollider> Rebase can often partially invalidate reviews anyway unless its trivial in which case theres no point not doing it
< jonasschnelli>
we had this in the past
< provoostenator>
It might just be my lacking git skills, still haven't found an optimal way to say "start with master, add branch X, then branch Y, then rebase the new stuff on the current branch"
< jnewbery>
I don't think this is actually a project-wide policy. I personally tend not to review PRs that have needed rebase for a long time because: - it signals that the contributor may not actively be working on the PR; - my review will be invalidated by the rebase anyway.
< jonasschnelli>
Constant rebasing is part of QA (rebased versions gets reviews)... and a sadly necessary IMO
< MarcoFalke>
Indeed, rebases often go wrong (as in the conflict is solved in the wrong way)
< provoostenator>
I often check rebases with: PREV=... N=... && git range-diff `git merge-base --all HEAD $PREV`...$PREV HEAD~$N...HEAD
< provoostenator>
Where PREV is the last thing I acked, and N is the number of commits in the branch
< jonasschnelli>
Also, constant rebasing makes you also up do date with changes around your code (which you otherwise would miss) *duck*
< wumpus>
jnewbery: I agree
< wumpus>
and yes, this isn't a project-wide policy, but one that every reviewer determines for themselves, how do you consider what to review?
< provoostenator>
Drahtbot might be able to help verify that a straight rebase is just that, maybe saying something like "existing ACK ... is the same as [new hash] rebased"
< wumpus>
I don' think we can make much progress here, besides knowing each other's perspective
< MarcoFalke>
Also, DrahtBot will list all conflicts with other pull requests, so it should give some idea what best to review first or what to prioritize (or for the author) if it might help to split up the pull request into smaller changes
< luke-jr>
provoostenator: that's trusting the bot a bit too much
< MarcoFalke>
provoostenator: We don't need a rebase if it is a effective "fast forward"
< provoostenator>
I didn't say "trust the bot"
< MarcoFalke>
Either the bot tracks it as conflict or it doesn't need rebase
< * sipa>
has little opinion
< wumpus>
we don't have any other topics do we :<
< gwillen>
provoostenator: I can probably answer git questions on "how to convince it to do X", given some details
< jnewbery>
I'd like to quickly mention the residency again, but only at the end if we don't have other topics
< promag>
even a straight rebase can result in broken travis
< wumpus>
feature freeze is in roughly two weeks! hurry up :<
< * sdaftuar>
types faster
< wumpus>
hehe
< wumpus>
March 1 is planned branch split-off
< wumpus>
#topic Chaincode residency (jnewbery)
< jnewbery>
Thanks wumpus. A couple of things: 1. I emailed a bunch of you to ask about mentorship. Thank you to everyone who replied!
< jnewbery>
2. we officially announced the residency today. We're still looking for great engineers who want to spend summer working on Bitcoin or Lightning with us. If you know anyone who might be appropriate, please send them our way at https://residency.chaincode.com/#apply
< jnewbery>
(endtopic)
< wumpus>
thanks!
< wumpus>
anyone with other topics ?
< wumpus>
#endmeeting
< lightningbot>
Meeting ended Thu Jan 31 19:45:29 2019 UTC. Information about MeetBot at http://wiki.debian.org/MeetBot . (v 0.1.4)
< gwillen>
provoostenator: although I guess specifically for what you're asking, I don't know of a better way than two rebases (first rebase X onto current, then rebase Y onto current) if I'm correly understanding
< gwillen>
correctly*
< provoostenator>
gwillen: this incantation works for me: PREV=... N=... && git range-diff `git merge-base --all HEAD $PREV`...$PREV HEAD~$N...HEAD
< provoostenator>
It's ugly
< gwillen>
heh, my git does not even have "range-diff"
< provoostenator>
It's pretty new
< provoostenator>
And very colorful.
< provoostenator>
It shows stuff that's been added on top of what was added last time, etc
< gwillen>
right, like the gitlab "changes to this PR since last force-push" feature
< gwillen>
which I forget whether github has
< jnewbery>
provoostenator: I not sure of exactly the scenario you're talking about. If you mean "I have PR X, and PR Y which builds on X, and X needs rebase", then I'd say don't bother updating Y at all. X needs to be merged before anyone should review Y. Once X is merged, reset Y onto master, then cherry-pick the Y commits, resolving conflicts as you go.
< provoostenator>
jnewbery: oh wait, I was talking about the simpler task, during review, of checking what changed since the last rebase. That's what that incantation does.
< provoostenator>
As for the scenario you refer to, the reason I would rebase is so I can keep working on it
< provoostenator>
Usually the issue is that a new version of X fixes something that I need to continue working on Y.
< provoostenator>
Another scenario is rebasing on master, when conflicts arise in X. In that case I usually just wait for the author of X do to the rebase, but then I get back to the above problem.
< jnewbery>
ah ok, sorry. Thought you were still talking about the exponentially bad rebase problem!
< provoostenator>
I think I need to use interactive rebase and then swap out commits with pick.
< jnewbery>
my solution (and this isn't to say anyone else should do this): if I'm running too far ahead of review (too many PRs open, my PRs queued up behind other people's PRs, etc), then I switch focus to reviewing other people's PRs rather than continue writing code that no-one's going to review for the time being.
< promag>
jnewbery: +1, I also pick a couple of issues I'm comfortable with and try to push a fix
< jonasschnelli>
wumpus: would be nice if you could review #15091 since you commented on it.
< bitcoin-git>
[bitcoin] Empact opened pull request #15308: ci: Build and test Trusty against system libraries, fix incompatibilities (master...trusty-no-depends) https://github.com/bitcoin/bitcoin/pull/15308