< aj>
wow travis sure isn't behaving at its best this week
< Randolf>
Is it just slow, or is it exhibiting some problems?
< aj>
jnewbery: +1 on splitting up release-notes and combining in the rc-phase
< aj>
Randolf: https://travis-ci.org/bitcoin/bitcoin/builds/359365323 failed badly, causing a bug to slip through into master that's only picked up in a travis-only (ish) test, causing travis to fail for all PRs after that, needing #12821 to fix
< Randolf>
aj: Have the folks in the #travis channel become aware of this? I found them to be very helpful when I asked them a question a few weeks ago.
< jtimon>
sorry for not following up, but promag will take care of the second part he discovered, please let's move on with https://github.com/bitcoin/bitcoin/pull/12172 's from 58 comments anf +9-4 as it was supposed to be
< bitcoin-git>
[bitcoin] jtimon closed pull request #11426: BIP90: Make buried deployments slightly more easily extensible (master...e16-bip90-extensible) https://github.com/bitcoin/bitcoin/pull/11426
< bitcoin-git>
[bitcoin] eklitzke opened pull request #12825: Only allocate a LevelDB block cache if LevelDB will actually use it (master...buffer-cache) https://github.com/bitcoin/bitcoin/pull/12825
< bitcoin-git>
[bitcoin] matthias-g opened pull request #12827: Trivial: Don't use short version of 'tinyformat/fmt' namespace (master...tinyformat-fmt) https://github.com/bitcoin/bitcoin/pull/12827
< wumpus>
fanquake: I'm not sure about #12767 - tend to agree with sipa. We shouldn't make unbridled changed to the code everywhere just to make broken static analysis tools happy. I have a similar problem with #12827.
< bitcoin-git>
[bitcoin] matthias-g closed pull request #12827: Trivial: Don't use short version of 'tinyformat/fmt' namespace (master...tinyformat-fmt) https://github.com/bitcoin/bitcoin/pull/12827
< fanquake>
wumpus heh, looks like the second issue was fixing a problem with a specific IDE?
< wumpus>
fanquake: apparently! I only now see it's an IDE, thought it was another analysis tool
< wumpus>
there are so many of those, and while they can be useful, they tend to have lots of false positives too. Most of the PRs resulting from them solve false positives, not actual problems found.
< fanquake>
Looks like #12790 can be merged.
< gribble>
https://github.com/bitcoin/bitcoin/issues/12790 | [Tests] Use blockmaxweight where tests previously had blockmaxsize by conscott · Pull Request #12790 · bitcoin/bitcoin · GitHub
< bitcoin-git>
[bitcoin] practicalswift closed pull request #12789: Don't return a CExtPubKey filled with random data when DecodeExt{Pub,}Key is given input not passing DecodeBase58Check(...) (master...CExtKey-junk) https://github.com/bitcoin/bitcoin/pull/12789
< jnewbery>
wumpus: if you're on a merge spree, #10762 and #11773 look ready
< wumpus>
jtimon: looks like build 1 didn't even start yet?
< jtimon>
I tried cancelling the job and restarting it, but yeah, it didn't even start
< wumpus>
it's possible that it's hanging on a previous PR/build and cannot allocate a builder to that, yet
< jnewbery>
Perhaps we should just not update release-notes.md at all in individual PRs and just have the wiki page open from the beginning of the release cycle. PRs that require release notes can be tagged as requires_release_notes so we can verify that they all got done at the end of the cycle.
< jnewbery>
Maybe something to discuss in the meeting
< wumpus>
I'd never have expected the release notes to become a bottleneck. One positive thing about this is: people are writing release notes for their changes!
< jnewbery>
it's not a huge bottleneck, but it seems like a completely avoidable annoyance to have reviews invalidated by release-notes.md conflicts
< wumpus>
yes, a wiki might be better for this, though on the other hand, having the changed synced to merges makes sense
< wumpus>
changes*
< jnewbery>
yes, good point. Let's discuss in the meeting
< fanquake>
Forgot it was meeting night tonight. Should probably make and effort to join.
< wumpus>
that'd be cool, though I know it's not easy for that part of the world
< wumpus>
before that, we had no depth defined in the yml
< wumpus>
(which effectively means depth=1 IIRC)
< arubi>
it's 50 I think
< wumpus>
so maybe it'd be better to remove the depth specification and leave it up to travis again
< wumpus>
on the other hand, this way it spends less time building old master commits :-)
< sipa>
meeting time?
< jnewbery>
hello
< eklitzke>
hi
< provoostenator>
hi
< bitcoin-git>
[bitcoin] Sjors opened pull request #12833: WIP [qt] move QSettings to bitcoin.conf where possible (master...2018/03/bitcoin-conf-rw) https://github.com/bitcoin/bitcoin/pull/12833
< achow101>
meting?
< sipa>
meeting, me think
< jamesob_>
yo
< sipa>
wumpus: ?
< jimpo>
hi
< wumpus>
#startmeeting
< lightningbot>
Meeting started Thu Mar 29 19:03:52 2018 UTC. The chair is wumpus. Information about MeetBot at http://wiki.debian.org/MeetBot.
< BlueMatt>
jnewbery: well its a trivial rebase that shouldnt materially effect review
< jamesob_>
I'd like to nominate ryanofsky's #10244. The burden of rebasing/conflict resolution is high and I think it's in pretty good shape (though needs rebase atm).
< wumpus>
I've... already said everything I wanted to said about that, won't repeat myself
< BlueMatt>
so I guess I'm more of a fan of this than the wallet/main split, but I feel like we need to think a bit harder about the api between the gui/wallet+main before we go split it
< BlueMatt>
I mean some of these things maybe shouldnt be blocking calls
< wumpus>
TBH we discussed this at the new york meeting
< wumpus>
and the agreement was that this could be improved after it goes in
< BlueMatt>
ok, well I will shut up, then, if its been beaten to death
< BlueMatt>
ok, nvm
< wumpus>
I'm ok with that. I'd have preferred to make the GUI asynchronous first
< wumpus>
but Iom' not going to beat that topic to death
< wumpus>
right
< ryanofsky>
api is definitely meant to be improved, especially the init stuff which is pretty ugly
< kanzure>
are there any big blockers to asynchronous gui things?
< BlueMatt>
yea, I mean that was what I was gonna say, but if there was agreement its not worth re-opening the book on that to discuss
< wumpus>
no, it's just a different set of work
< wumpus>
it's somewhat orthogonal to this - my gut just hates blocking RPC calls in GUI threads, it's more of an instinctive revulsion than anything I can explain, so I'll just go along
< jamesob_>
this PR introduces no RPC calls
< provoostenator>
I think part of the understanding was that this interface should be considered very much not final.
< BlueMatt>
jamesob_: it introduces a whole new rpc interface...
< wumpus>
it does, it introduces an RPC layer between the wallet and the core
< provoostenator>
Just having _an_ interface was step one.
< wumpus>
please don't deny that
< BlueMatt>
anyway, next topic?
< wumpus>
yes, any other topic suggestions?
< sipa>
wumpus: i think jamesob_ means RPC as in the existing JSON RPC system
< sipa>
not RPC as a generic term
< jamesob_>
correct
< wumpus>
ok, yes, RPC is a general term for cross-process calls
< ryanofsky>
jamesob_, an earlier version of this pr did mention ipc, but i took that stuff out
< jnewbery>
This first step isn't cross-process
< BlueMatt>
lol, ok, so any topics *aside* from debating rpc/ipc/whatever terminology?
< jnewbery>
I don't think it's a major issue, but it is irritating to have reviews invalidated due to release notes conflicts
< jnewbery>
options: 1) do nothing because it's not a huge issue
< wumpus>
could do them in a separate commit, at the end
< sipa>
do we know if githubdeals correctly with the gitattributes merge=union stuff?
< wumpus>
oh wait that doesn't help with rebases...
< achow101>
Maybe we should have the release notes dev wiki thing continuously up and people just add stuff to it as needed
< jnewbery>
2) don't use release_notes.md and just use a wiki for the whole release cycle
< jnewbery>
3) have separate release_notes files for each PR and merge them at the end
< BlueMatt>
I mean as long as its a separate commit no reason to invalidate reviews
< jnewbery>
4) ?
< sipa>
4) is the merge=union thing?
< jnewbery>
merge=union doesn't help with github I think
< achow101>
I prefer 2
< sipa>
i don't like 2
< sipa>
too much process overhead
< wumpus>
achow101: I think the only argument against 2 is that it decouples the merge from the release mode update
< wumpus>
notes*
< ryanofsky>
an option 4) would be to insert 50-100 blank lines in the file, and add release new notes in the blank space. this would avoid most conflicts
< BlueMatt>
option n) leave release notes as a comment on pr and tag the release-notes-needed issue
< wumpus>
cfields: storing it *per section* would still help!
< BlueMatt>
easy to merge at the end
< BlueMatt>
and they exist in the pr itself
< ryanofsky>
i also like 3 best
< wumpus>
'leave it to the maintainer at the end' is not an option :p
< sipa>
it may be a release notes file per "feature" too, i think, if multiple PRs sequentially update the se thing
< jnewbery>
sipa: sounds reasonable, if they're serial
< sipa>
right
< jimpo>
Yeah, I like the idea of basically having a file for each section in the current release notes
< wumpus>
I mean what you want to avoid is that *unrelated* PRs collide in the release notes
< sipa>
wumpus: yyp
< wumpus>
if PRs that already affect the same thing collide, that's not too bad, because the code likely does too
< wumpus>
so yes, 3 sounds like a good idea to me, though it might be overdesign for something that doesn't cause too much trouble in practice, I wonder if anyone will actually do it
< sipa>
we can see how it plays out
< jnewbery>
if it's in the developer notes, then I think people will do it
< jnewbery>
I'll do it for my PRs to avoid conflicts
< jamesob_>
could add a lint step to the build that fails if the PR touches the main release notes files as well as src/ files
< wumpus>
definitely needs to be in the developer notes, like "what directory to use for partial release notes'
< wumpus>
oh no no more lints
< jnewbery>
I think that's probably enough discussion. As long as the maintainers don't object to partial release notes then individual contributors can start using them
< wumpus>
I get quite angry if yet another redundant python import breaks travis
< jamesob_>
suggestion retracted :)
< instagibbs>
I don't even think there's contribution notes yet
< instagibbs>
for release notes
< wumpus>
jamesob_: sorry :)
< instagibbs>
i had to ask promag
< jnewbery>
wumpus: is that not caught in the PR's travis run?
< wumpus>
jnewbery: I think it is
< sipa>
topic suggestion: avoid undefined behaviour when it shouldn't matter? (#12789)
< gribble>
https://github.com/bitcoin/bitcoin/issues/12789 | Dont return a CExtPubKey filled with random data when DecodeExt{Pub,}Key is given input not passing DecodeBase58Check(...) by practicalswift · Pull Request #12789 · bitcoin/bitcoin · GitHub
< wumpus>
#topic avoid undefined behaviour when it shouldn't matter?
< jtimon>
ryanofsky: why not just create a separated pr editing the release notes after the actual pr doing things has been merged?
< BlueMatt>
"shouldnt"
< sipa>
i bring it up here because it may be something we should or shouldn't have as a guideline
< sipa>
for example, should you initialize a variable that isn't read anywhere, because soke compiler warning fails to understand it isn't being read?
< sipa>
argument in favor: more deterministic failures
< BlueMatt>
oh, well that isnt "shouldnt"
< BlueMatt>
that is "doesnt, but compiler warns"
< sipa>
argument against: reduces the ability for tools to detect things stativally
< provoostenator>
Other argument in favor: means a linter can catch all uninitialized variables.
< sipa>
well i say shouldn't, because reviewers may be wrong and the compiler may be right
< wumpus>
jtimon: that's a possibility too, though like the wiki option it decouples the code change from the release notes change itselff
< wumpus>
jtimon: also: EVEN MORE PRs :(
< jtimon>
wumpus: yep, although I guess the bigger drawback is more prs
< jtimon>
right
< BlueMatt>
I mean if its at all tricky to show that it *wont* be read, then should def follow the compiler, but the nonstop stream of "this compiler is shit and warned on something that it shouldnt be" prs is....not ideal
< wumpus>
yeah...
< BlueMatt>
honestly of all those pros/cons, the pr volume is probably the most important imnsho
< wumpus>
so many *fix some and some false positive for my crappy static analysis tool/compiler with warnings jacked up*
< sipa>
i generally dislike the "compiler/analyzer/linter/tool doesn't understand X, let's initialize everything to shut it up"
< wumpus>
me too
< wumpus>
just fix your tools FFS
< bitcoin-git>
[bitcoin] MarcoFalke closed pull request #12823: doc: Switch release-notes.md to union merge (master...Mf1803-docGitattributes) https://github.com/bitcoin/bitcoin/pull/12823
< wumpus>
if it's correct, human-understandable C++ code and we know there's no problems with it, it should not be changes because compiler blabla
< wumpus>
too risky, too
< sipa>
or improve the code so it is easier for tools (and humans) to see it is correct
< wumpus>
if it's not broken don't change it
< sipa>
true
< sipa>
ok, just wanted to hear opinions about this
< wumpus>
unless it's a refactor to prepare for osmething else, of course, but that wasn't the premise :)
< wumpus>
so I think we agree
< sipa>
yes
< wumpus>
any other topics?
< jtimon>
BlueMatt: I don't know, will more volume of prs specific to release notes be that much more cumbersome?
< wumpus>
jtimon: yes. In that case I prefer the wiki
< BlueMatt>
less so than code-change pr volume
< BlueMatt>
but whatever
< jnewbery>
wumpus: I agree
< wumpus>
that's why we have the wiki-phase at all before releases, to prevent a jungle of update-release-notes PRs
< jtimon>
yeah, I mean, I don't have a strong opinion either way
< wumpus>
(which will also conflict with each other! though easier to rebase..)
< ryanofsky>
jtimon, imo including release notes along with changes makes changes easier to understand, and also probably more well thought out
< wumpus>
yes, it's better than code-change PR volume that's for sure
< wumpus>
ryanofsky: hey that's a good point
< jtimon>
sipa: sometimes warning are useful, sometimes they are not and it's alright to leave them there. but not sure what the discussion is. nobody is proposing we use -Werror, right?
< wumpus>
I remember seeing the 'release notes per item' before in some project, not sure which
< jtimon>
ryanofsky: I agree, but then you have to deal with rebases, I don't see a way around it
< wumpus>
jtimon: warning being good or evil wasn't what the topic was about
< sipa>
jtimon: my view is (for example) that if you systemativally initialize every variable (even those for which you know won't be used), you will lose the ability for the compiler to give you warnings about accidentially uninitialized things
< jtimon>
wumpus: that's what I'm saying, that I'm not sure what the topic is
< sipa>
this is more general than just compiler warnings, and variable initialization though
< wumpus>
at ASML we had that as part of the C coding standard - every, single, variable had to be initialized
< wumpus>
no I don't think we need that here :)
< cfields>
sipa: yes, i really like newer gcc/clang's ability to warn about being unitialized for one or more paths
< wumpus>
I do think all class variables should be initialized in the constructor, in general
< cfields>
wumpus: agreed, but I'd like to start using more c++11 member-initialization for trivial types as it's so much less verbose
< wumpus>
cfields: they had that in the static analyzer for quite a while, now it moved to a compiler warning, a good thing
< cfields>
right
< wumpus>
cfields: yes, that's a nicer syntax
< wumpus>
ok, any other topics?
< sipa>
seems not
< wumpus>
#endmeeting
< lightningbot>
Meeting ended Thu Mar 29 19:44:25 2018 UTC. Information about MeetBot at http://wiki.debian.org/MeetBot . (v 0.1.4)
< cfields>
out of curiosity, does the c spec allow for compilers to ignore initializers if a value is always set before it's used?
< wumpus>
only if there are no side effects
< cfields>
i'm wondering if compilers are allowed to do the opposite optimization: you always initialize, but it removes them when possible.
< BlueMatt>
the compiler could run any obvious part of your program and just change the program to have the same effective in/out results, so....yes?
< wumpus>
^^
< BlueMatt>
see-also: crypto-memset
< wumpus>
the C spec wouldn't say anything on that, because it's not visible to the code in any path
< luke-jr>
ETA until compilers try to do IBD for you?
< luke-jr>
☺
< BlueMatt>
luke-jr: they'll fail on the first net access :(
< luke-jr>
BlueMatt: yes, I'm joking :P
< wumpus>
luke-jr: only if you manage to do blockvalidation in c++78 metaprogramming
< wumpus>
(or maybe it's already possible with current standards, at least compile time hashing is already possible :-)
< cfields>
heh
< BlueMatt>
rust has a fucking full ast interpreter in the front-end compiler now, to in the future run anything with no io as a constexpr.......
< wumpus>
that's an interesting choice
< BlueMatt>
or, well, thats a possible future use for it, but they have an interpreter in the front-end
< wumpus>
the drawback with c++ compile-time metaprogramming has always been that it's really slow, as it's circuitous because it (ab)uses features meant for something else. So, why not just include an ast interpreter.
< wumpus>
(compile-time slow, I mean)
< wumpus>
so apparently the Tor project is working on porting parts to rust
< wumpus>
not sure what parts, but it has always been an exclusively C codebase before
< booyah>
also, what for
< booyah>
after decades it's probably rather safe from low-level errors, isn't it
< wumpus>
hehe :)
< wumpus>
that's anyone's guess, really
< BlueMatt>
seems premature tbh to me, mostly cause if you want to, eg, compile it on debian stable you have to use a super-old version of rust and end up getting a billion warnings from recent versions telling you to use new syntax :(
< wumpus>
debian stable is the problem there
< sipa>
cfields: not only can they, i believe that SSA transforms will pretty much automatically do that
< BlueMatt>
wumpus: true, but, what, you're gonna not support debian stable? so...you lose, what, 1/5 your users?
< cfields>
sipa: so there's no (performance) downside of initialize-by-default as a rule?
< wumpus>
BlueMatt: but the only way to pressure them into upgrading their rust version is likely for major projects to start using it, it's always a chicken/egg problem
< BlueMatt>
why would they make an exception to their ship-only-insanely-out-of-date-software rule for a *compiler*?
< wumpus>
if e.g. bitcoin would start using it, no one would care, but something like tor has quite a lot of influence I think
< BlueMatt>
that seems like the one thing they'd be least likely to make an exception for
< wumpus>
oh we'll see
< wumpus>
I'm glad someone is taking the initative there that's not me
< BlueMatt>
lol, well at least we succeeded at getting them to stop shiping bitcoin
< wumpus>
firefox is likely the main pusher for (decent) rust support in distros
< BlueMatt>
maybe if the tor folks also succeed
< BlueMatt>
debian already stopped shipping firefox a long time ago :p
< BlueMatt>
(because of this exact issue, too....)
< wumpus>
huh? really?
< BlueMatt>
iceweasel, yo
< wumpus>
iceweasel is simply a rebranded firefox
< wumpus>
because of some license issue...
< BlueMatt>
yes, but they had to because they wanted to ship super old versions and that wasnt allowed
< BlueMatt>
afair
< BlueMatt>
(among other issues)
< wumpus>
shipping old versions of browsers is really dangerous
< BlueMatt>
yes, just pointing out that debian was so headstrong in their desire to do stupid insecure shit that they rebranded firefox for it....
< wumpus>
yes I didn't know that was the reason
< BlueMatt>
i mean i may be misrecalling, but I believe that was one of the things that violated the acceptable-use license that firefox required to use their branding
< BlueMatt>
(among a few other issues)
< cfields>
I thought debian was back to firefox now?
< cfields>
iirc the tm issue was resolved somehow
< BlueMatt>
seems like it, yes, still, my point stands