< wumpus>
is there a way to see what appveyor is currently doing? I see only 'queued' PRs, even under history
< wumpus>
"Current build" is in queued state too
< wumpus>
BlueMatt: https://github.com/bitcoin/bitcoin/pull/15382#issuecomment-551451359 kind of reminds me what has bothing me about the rust stuff; I think the build system there also needs to focus on features, not "use rust or not", don't group them on programmings language/dependency; if someone wants to enable a feature that happens to be implemented in rust, request them to install a rust compiler :)
< wumpus>
this is also why we have with-gui, not with-qt
< wumpus>
though admittedly, the default there is auto, which sets it to enabled or not based on whether qt development package is installed, which is used as a proxy for 'is this a headless server or a desktop', I guess ...
< wumpus>
as said I'm somewhat skeptical at those kinds of attempts to detect what the user wants
< elichai2>
wumpus: I actually don't completely on that. for a long time I didn't understood a thing about bitcoin's build system and/or code and I liked that it installed basically everything I could manage (I'm one of those freaks who likes the GUI :) )
< elichai2>
I think currently the build system is bloated with options such that there's no way for a naive user to even *know* what he can enable/disable
< elichai2>
(though maybe auto detection isn't the right answer but splitting features into "normal" and "advanced" is the better answer )
< wumpus>
elichai2: sure, but how does features things based on what dependencies you happen to have installed help with that?
< wumpus>
picking features*
< elichai2>
wumpus: that you somehow ends up with a lot of features that at least some of them you want? :P
< elichai2>
(i.e. I remember installing qt and then compiling core so I can get the GUI, and if I didn't get it I knew my QT is missing)
< wumpus>
like, for example the wallet, we assume someone wants a wallet unless --disable-wallet is provided. I think that is great.
< wumpus>
wouldn't it be wtf if it was determined based on whether someone has bdb headers installed
< wumpus>
no, if you don't have bdb headers installed, and don't provide --disable-wallet, you get a clear message
< wumpus>
so you can choose, you want a wallet and install the headers, or disable it and don't
< wumpus>
a bloated build system is a different problem, and I agree it wouldn't be great if you would have to do that for 500 different things, but choosing automatically based on the phase of the moon just *hides* that problem
< elichai2>
yeah but I think for the wallet 90% of users want it. GUI 50%, and then the rest idk (even those this is 100% guess according to users I know heh)
< wumpus>
for a while I was frustrated that for example, tor wouldn't pass configure unless some documentation building thing is installed, or --no-docs is passed. But I think it makes sense now. Most people (should) want documentation, and having *random documentation generator* not installed doesn't automatically mean you don't.
< wumpus>
and if you really don't, then tell it...
< wumpus>
the only thing that makes this behavior somewhat acceptable is the summary at the end of configure :)
< wumpus>
because at least you see what automatic choices it made, before you start building and conclude you're building a GUI you don't want, for ex.
< elichai2>
yeah. if you're already venting, I hate that setting CFLAGS/CPPFLAGS/CC/CXX can sometimes break the configure
< wumpus>
well if you set some insane flags like --no-stdlib that's expected :)
< elichai2>
It happened to me a when I tried to switch a linker but ???
< bitcoin-git>
[bitcoin] sipsorcery opened pull request #17416: Appveyor improvement - adds check for all required vcpkg packages (master...check-missing-vcpkgs) https://github.com/bitcoin/bitcoin/pull/17416
< bitcoin-git>
[bitcoin] fanquake opened pull request #17417: [0.19] cli: fix -getinfo output when compiled with no wallet (0.19...backport_17368_019) https://github.com/bitcoin/bitcoin/pull/17417
< bitcoin-git>
[bitcoin] RandyMcMillan opened pull request #17418: Build: add REPO_SLUG to fix errors in forked builds (master...repo-slug) https://github.com/bitcoin/bitcoin/pull/17418
< michaelfolkson>
With the PR review club we often need to look back on old PRs that have been merged (in some cases years ago). Any thoughts (especially from maintainers, long term contributors) on asking questions or adding links/resources on a closed PR for educational purposes? Or is this too noisy and you'd prefer to not have activity on PRs once merged/closed?
< michaelfolkson>
An alternative would be to ask a question on StackExchange referring to a particular PR. That would be preferable right?
< emilengler>
michaelfolkson: We could create a masterthread issue on the main repo
< emilengler>
And then we could link older PRs as well
< emilengler>
If you look the older PR you will se that it got mentioned but IIRC no party will get a notification
< emilengler>
And it is not counted as activity in that PR as well IIRC
< michaelfolkson>
The no notifications sounds good. What is a masterthread issue though? A issue label "masterthread" which doesn't notify anyone? You'd set up a new issue each time you have a question on a PR? Or an issue is set up mapping to one particular PR and then all discussion for that PR is on that one issue?
< Bullit>
Where Bernstein was a conglomerate in Aberdeen
< BlueMatt>
wumpus: hmmm...could do a experimental-anticensorship-features or so, but there was some discussion of doing a gbt replacement in rust, too
< BlueMatt>
which....
< BlueMatt>
so, leaves me without a *great* name for it
< BlueMatt>
but happy to rename it to something else if you have an idea
< sipsorcery>
@fanquake you around? PR's #17412 and #17416 are a little bit orthogonal.
< gribble>
https://github.com/bitcoin/bitcoin/issues/17416 | Appveyor improvement - adds check for all required vcpkg packages by sipsorcery . Pull Request #17416 . bitcoin/bitcoin . GitHub
< fanquake>
sipsorcery: yea
< sipsorcery>
17416 is a fix to allow new vcpkg's to be added without needing the appveyor cache to be invalidated.
< sipsorcery>
17412 is adding the specific package boost-process to the list of packages.
< fanquake>
Yes, but isn't the reason 17412 is required because of the issue 17416 is fixing? Otherwise I don't understand why the boost packages need to be added in advance.
< sipsorcery>
no. at least I don't think so.
< fanquake>
I'd rather not do that, or modify the build docs until the feature that requires them is actually merged.
< fanquake>
Right. So does this mean that you can basically smash appveyors global cache and break all other PR builds just buy opening a new PR with a new dependency?
< fanquake>
*by
< sipsorcery>
actually boost-process is being added in 15382. so 17412 seems to be a pre-emptive add to avoid future cache invalidations.
< sipsorcery>
i think i understand what you're getting at now.
< sipsorcery>
as far as the appveyor cache goes it's going to get regularly invalidated due to changes to the build
< sipsorcery>
so it's not something that has to be avoided. it's something to minimise.
< sipsorcery>
so 17416 will allow new packages to be added without manual intervention and avoid unnecessary vcpkg operations when there are no new packages.
< fanquake>
That sounds like an improvement
< sipsorcery>
the addition of boost-process looks like it can wait for 15382.
< sipsorcery>
and 17412 can stay closed.
< sipsorcery>
all good now, thx.
< fanquake>
?
< bitcoin-git>
[bitcoin] MarcoFalke closed pull request #17418: Build: add REPO_SLUG to fix errors in forked builds (master...repo-slug) https://github.com/bitcoin/bitcoin/pull/17418
< bitcoin-git>
[bitcoin] RandyMcMillan closed pull request #17389: build: add background.tiff and .pngs to "make clean" (master...make-clean-tiff) https://github.com/bitcoin/bitcoin/pull/17389
< jonatack>
Suggested topic: deprecating getaddressinfo label in favor of labels, and adding multiple labels associated with an address (currently only one)
< phantomcircuit>
hi
< achow101>
jonatack: is it useful to have multiple labels with an address?
< luke-jr>
when I do my taxes, I take the label and match multiple regexs to extract metadata - having separate labels could simplify it
< meshcollider>
if it's not an enormous change to support multiple labels then concept ack
< luke-jr>
that being said, there is always a primary label, so maybe we should keep / should have kept it
< jnewbery>
There's no concept of a primary label
< luke-jr>
eg, "bought my pet hamster" isn't likely to have more than one tx
< luke-jr>
jnewbery: maybe there should be
< meshcollider>
Depends if label is for categorising or label is for describing I guess
< luke-jr>
I've always used it for the latter, with the former thrown in
< luke-jr>
categories without a description has limited usability
< luke-jr>
I'd suggest instead a "notes" with the description part, but .. we've had label for years
< jnewbery>
jonatack: I'll take a look at the PR and give concept feedback
< jonatack>
luke-jr: say, ability to designate one of the labels as a primary, and one only?
< achow101>
I think having multiple labls would be somewhat invasive due to all of the legacy accounts stuff associated with labels
< jonatack>
jnewbery: ty
< meshcollider>
I mean, I don't think it matters if the distinction is in the code. Supporting multiple labels allows the user to do whatever they like with them, I don't think it needs a "primary" one
< meshcollider>
Ok yeah this can be discussed in the PR
< meshcollider>
Other topics?
< jonatack>
a) it seems we should decide re multiple labels before deprecating
< luke-jr>
jonatack: maybe
< luke-jr>
achow101: maybe rename "labels" to "tags"?
< meshcollider>
jonatack: review beg for all his open wallet PRs :p
< achow101>
#17373
< gribble>
https://github.com/bitcoin/bitcoin/issues/17373 | wallet: Various fixes and cleanup to keypool handling in LegacyScriptPubKeyMan and CWallet by achow101 . Pull Request #17373 . bitcoin/bitcoin . GitHub
< meshcollider>
jnewbery: sounds good, review beg for that too then
< jonatack>
thanks
< achow101>
it'd be nice to merge these so I don't have to rebase the same PRs every day
< ryanofsky>
i have never understood what "labels are associated with addresses, instead of addresses associated with labels" means
< ryanofsky>
"bob is associated with mary, but mary is not associated with bob." huh?
< meshcollider>
It means it's not bijective, there is no well defined label -> address map
< meshcollider>
Because multiple addresses can have the same label
< sipa>
meshcollider: that was the case with accounts too
< meshcollider>
Yeah
< jnewbery>
one address couldn't have multiple accounts. It was always intended that one address could have multiple labels
< sipa>
i always assumed it meant "you should think as labels being the property of an address" rather than "a label is a collection of addresses"
< jnewbery>
sipa: +1
< ryanofsky>
so "X is associated with Y" means "a Y can only have one X"
< meshcollider>
Who knows, associated probably isn't the best word
< jnewbery>
ryanofsky: if the 'associated with' doesn't make sense for you, just use sipa's definition
< ryanofsky>
of course, yeah, i was just wondering how that associated thing that i see repeated made sense to anyone
< jnewbery>
I copied it in my PR descript from wumpus's original PR because it made sense to me, but I don't think it's used in any user-facing documentation
< jonatack>
I saw it as analogous to relational databases: has one relation vs belongs to relation
< jamesob>
sounds like an indexing thing: you can look up the labels associated with an address easily, but you cannot look up the addresses associated with a label easily (even though in reality it's probably quick to build that index ad hoc depending on the size of the wallet)
< luke-jr>
jamesob: that's the technical side
< luke-jr>
on the abstract side, a label isn't a container; it doesn't "have" transactions/addresses
< luke-jr>
it's a property of those transactions
< luke-jr>
so you can conceptually search for all transactiosn with a label, but not view the contents of a label
< jamesob>
curious, what're the contents of a label if not the associated txns?
< jnewbery>
luke-jr: yep, labels don't have a balance
< luke-jr>
there are no contents, that's the point
< jamesob>
gotcha
< luke-jr>
it's like putting a sticker on something
< luke-jr>
sure, you can find all objects with the sticker, but asking for the contents of such a sticker is silly
< meshcollider>
So a sticker is associated with the thing it's stuck on, but the thing it's stuck on isn't associated with the sticker
< wumpus>
yes, that's one of the next steps, I usually do that before the uploading the binaries (e.g. after the gitian process), but it doesn't really matter if it's a little sooner I guess
< jnewbery>
wumpus fanquake MarcoFalke: I think #15934 is ready for merge. ACKs on the head commit 083c954 from ariard, jamesob and jnewbery. Also ACKs on previous versions from fjahr and jonatack.
< wumpus>
thanks for improving the horror that is bitcoin core option parsing a bit :)
< midnight>
inb4 option parsing gets its own scripting language
< wumpus>
midnight: hah, I know you're joking, but I don't think a lack of flexibility was ever the problem, just that the code is a mess and problems are silently ignored... but also we've come a long way from #1044
< * jamesob>
waits for TheBlueMatt to suggest porting options parsing to rust
< wumpus>
it's, a good lesson for new projects, get these things designed well in the first release, if not you're going to spend years cleaning up the mess while competing with PRs that add functionality etc
< wumpus>
jamesob: lol
< bitcoin-git>
[bitcoin] MarcoFalke opened pull request #17423: ci: Make ci system read-only on the git work tree (master...1911-ciRo) https://github.com/bitcoin/bitcoin/pull/17423
< harding>
FYI, BitcoinCore.org isn't updating when new commits are merged. I DM'd BlueMatt a while ago, but I guess he's not around. I'm not sure who else has access to the server to debug it (this happens from time to time, but I'm not sure what can BlueMatt usually kicks to get things restarted).
< midnight>
wumpus: Only half joking. Complex options are heading that way, it seems to me. At some point *behaviour* of programs with complex options it seems to me might be helpful to be reactive or capable of simple decision-making
< * midnight>
shudders at the future
< wumpus>
harding: strange, I don't have access immediately either but probably we need to poke whatever does updates
< emilengler>
harding: Btw why is bitcoincore.org not hosted at github
< harding>
BlueMatt: is looking into it now.
< wumpus>
emilengler: cuuursed idea
< emilengler>
What's wrong with GH pages? It adds an extra layer of transparency
< emilengler>
Well you can't really do server side stuff though
< sipa>
emilengler: we really don't want to rely on github's security for distribution of binaries
< sipa>
if anything, the degree to which we depend on github already is scary
< wumpus>
midnight: I don't know, I dont' think *that* is so bad with bitcoin core. I do have experiences like that with things like printer settings though, where the software tries to 'intelligently' guess what your're trying to do, correcting for some settings, so at some point you need to make second-order guesses so that the software will make the correct guess and eventually send the right value to the
< wumpus>
hardware
< emilengler>
sipa: +1 but GH can't fake PGP signatures and if the users aren't verifying it it is pretty much their own fault IMO
< sipa>
emilengler: whether you like it or not, reality is that very few users verify PGP signatures
< harding>
emilengler: I don't have any data on BitcoinCore.org, but back when the binaries were exclusively hosted on Bitcoin.org, there was less than 1% as many downloads of SHA256SUMS.asc than there was of the binary files.
< wumpus>
but no, we're not going to trust github for hosting the website as well as hosting the repository
< wumpus>
one thing is enough
< emilengler>
Btw why isn't the repo hosted on the server then
< wumpus>
probably too much, anyhow, but let's not argue for making things worse
< emilengler>
ok
< wumpus>
in a far future I'm imagine we can reduce our dependency on github.com, but we're at least not trying to increase it
< wumpus>
(and yes, the bitcoincore.org server update verifies GPG signatures on commits independently, so someone compromising the github repository doesn't automatically compromise the website)
< midnight>
neat
< midnight>
I verify PGP signatures (most of the time!)