--- Log opened Wed Mar 18 00:00:25 2020 01:08 -!- sonofhan [~sonofhan@ip72-193-7-145.lv.lv.cox.net] has joined #bitcoin-builds 01:13 -!- sonofhan [~sonofhan@ip72-193-7-145.lv.lv.cox.net] has quit [Ping timeout: 250 seconds] 01:26 -!- ghost43 [~daer@gateway/tor-sasl/daer] has quit [Remote host closed the connection] 01:27 -!- ghost43 [~daer@gateway/tor-sasl/daer] has joined #bitcoin-builds 03:02 -!- kanzure_ [~kanzure@unaffiliated/kanzure] has joined #bitcoin-builds 03:04 -!- Netsplit *.net <-> *.split quits: kanzure 03:09 -!- sonofhan [~sonofhan@ip72-193-7-145.lv.lv.cox.net] has joined #bitcoin-builds 03:11 -!- ghost43 [~daer@gateway/tor-sasl/daer] has quit [Remote host closed the connection] 03:12 -!- ghost43 [~daer@gateway/tor-sasl/daer] has joined #bitcoin-builds 03:14 -!- sonofhan [~sonofhan@ip72-193-7-145.lv.lv.cox.net] has quit [Ping timeout: 250 seconds] 04:04 -!- Herman63Mills [~Herman63M@ns334669.ip-5-196-64.eu] has joined #bitcoin-builds 05:10 -!- sonofhan [~sonofhan@ip72-193-7-145.lv.lv.cox.net] has joined #bitcoin-builds 05:15 -!- sonofhan [~sonofhan@ip72-193-7-145.lv.lv.cox.net] has quit [Ping timeout: 250 seconds] 06:08 -!- meshcollider [meshcollid@gateway/shell/ircnow/x-wiarqpxplzhwfubz] has quit [Ping timeout: 256 seconds] 06:12 -!- kanzure_ is now known as kanzure 06:26 -!- meshcollider [meshcollid@gateway/shell/ircnow/x-eocmmwsmzrsbaslx] has joined #bitcoin-builds 06:44 -!- kanzure [~kanzure@unaffiliated/kanzure] has quit [Ping timeout: 246 seconds] 06:46 -!- kanzure [~kanzure@unaffiliated/kanzure] has joined #bitcoin-builds 06:49 -!- ghost43 [~daer@gateway/tor-sasl/daer] has quit [Remote host closed the connection] 06:49 -!- ghost43 [~daer@gateway/tor-sasl/daer] has joined #bitcoin-builds 06:55 < dongcarl> 5 mins 07:00 < dongcarl> Hello 07:00 < hebasto> hi 07:00 < dongcarl> jonatack hebasto fanquake cfields_ 07:00 < fanquake> yo 07:00 < jonatack> hey 07:01 < dongcarl> summoning cory gimme a sec 07:01 * cfields_ present! 07:01 -!- cfields_ is now known as cfields 07:02 < dongcarl> Woohoo 07:02 < dongcarl> Alright 07:02 < dongcarl> Let's get started 07:02 < dongcarl> Let's do discussion topics first 07:02 < dongcarl> I know fanquake you had one? 07:02 < fanquake> i have a few things in flight 07:02 -!- Herman63Mills [~Herman63M@ns334669.ip-5-196-64.eu] has quit [Ping timeout: 250 seconds] 07:02 < fanquake> don't want to start with fd accoutning though 07:03 < fanquake> unless you mean the _MSC_VER stuff? 07:03 < dongcarl> I see 07:03 < dongcarl> How about we go thru your PRs then? 07:03 < dongcarl> The ones you want attention on? 07:03 < dongcarl> And people can ask questions and discuss 07:03 < fanquake> Sure. Can start with #18358 if you want 07:03 < gribble> https://github.com/bitcoin/bitcoin/issues/18358 | util: fix compilation with mingw-w64 7.0.0 by fanquake · Pull Request #18358 · bitcoin/bitcoin · GitHub 07:03 < hebasto> testing right now 07:03 < dongcarl> Yeah that sounds good 07:03 < fanquake> I also answered your question hebasto 07:04 < hebasto> ty 07:04 < fanquake> The underlying mingw-w64 headers being used are 5.0.3 07:04 < fanquake> (on ubuntu bionic) 07:04 < dongcarl> So I think the main fallout from that is we need to use the right detection, correct? 07:04 < cfields> solution sounds good if you're confident that everything in the description is correct. 07:04 < fanquake> Yes. I've had a quick look at our usage of _MSC_VER throughout the code, and it looks like there are a few places we should follow up. 07:05 < dongcarl> fanquake: So the overarching goal is detecting when we're targeting mingw-w64 right? 07:05 < cfields> Oh wait, I didn't look at the actual changes. 07:05 < dongcarl> Or just windows in general? 07:05 < hebasto> fanquake: maybe submit an issue to track them all 07:05 < dongcarl> cfields: Yeah to be clear we are not adding -D, we're using gmtime_s instead 07:05 < fanquake> dongcarl I think it's more Windows in general 07:06 < fanquake> So to give an example 07:06 < cfields> IIRC there's a very specific define that's meant to enable these c extensions. Are you sure we're not just meant to be turning on a more fine-grained one? 07:07 < dongcarl> fanquake: Right, I think we should either: 1. Define a flag that symbolizes "windows in general", or 2. Find a flag that does this 07:07 < fanquake> Are you thinking of SECURE_API? 07:07 < dongcarl> cfields: With the current PR we don't need the extensions 07:07 < cfields> No, I mean specifically for gmtime_s/gmtime_r. 07:07 < cfields> I just need a min to catch up. You guys go ahead :) 07:08 < fanquake> dongcarl: to give an example of what I'm thinking about. Currently if you build master, cross compiling using mingw-w64, we wont call __rdtsc() (https://github.com/bitcoin/bitcoin/blob/master/src/random.cpp#L58), even though mingw-w64 does provide it. 07:08 < cfields> IIRC neither gmtime_r, nor gmtime_s are standard, they're both extensions. 07:08 < fanquake> Because we wont fall into the _MSC_VER block 07:08 < fanquake> We'll fall into here https://github.com/bitcoin/bitcoin/blob/master/src/random.cpp#L63 instead 07:09 < fanquake> What I'm trying to clarify is, should we be using all the windows specific functions, like gmtime_s, __rand() etc, when compiling using mingw-w64, and not just MSVC? 07:09 < cfields> As with all bounds-checked functions, gmtime_s is only guaranteed to be available if __STDC_LIB_EXT1__ is defined by the implementation and if the user defines __STDC_WANT_LIB_EXT1__ to the integer constant 1 before including time.h. 07:10 < dongcarl> fanquake: I see, lemme think a bit 07:10 < dongcarl> cfields: Apparently not in mingw-w64 07:10 < fanquake> SecureZeroMemory is another example: https://github.com/bitcoin/bitcoin/blob/master/src/support/cleanse.cpp#L18 07:10 < dongcarl> cfields: There's no mention of __STDC_LIB_EXT1__ anywhere 07:11 < cfields> I'm worried that you guys are trying to target platforms rather than features. 07:11 -!- sonofhan [~sonofhan@ip72-193-7-145.lv.lv.cox.net] has joined #bitcoin-builds 07:12 < dongcarl> Okay let 07:12 < cfields> but I'll comment on the PR, definitely need to catch up some. 07:12 < dongcarl> Sounds good 07:12 < dongcarl> fanquake: I think I see what you're trying to say 07:13 < fanquake> cfields: do you. mean targetting Windows rather than determining the existence of a certain function? 07:14 < cfields> fanquake: right. "#if platform" is always a last resort. 07:14 < dongcarl> Oh I see... 07:14 < fanquake> cfields: right. I can look for more generic examples, but in the case of something like SecureZeroMemory(). That is a very Windows only/specific function right? 07:14 < dongcarl> Perhaps the right thing to do is to check for functions? 07:15 < hebasto> in configure? 07:15 < fanquake> So we should be able to use something like a WIN32 define, rather than MSVC + MINGW ? 07:15 < cfields> the fact that there are lots of "#if x86" in there may make it seem more ok, but that's because that's around x86-specific asm. In that case, it's a perfectly reasonable guard. 07:16 < fanquake> Is that in regards to __rand()? 07:16 -!- sonofhan [~sonofhan@ip72-193-7-145.lv.lv.cox.net] has quit [Ping timeout: 250 seconds] 07:16 < cfields> So yes, in that case, I don't see why we're not checking for it in configure and saying "#ifdef HAVE_rdtsc" 07:17 < cfields> Ah, unless that file's meant to be usable outside of autoconf? 07:18 < fanquake> random.cpp ? There's a lot in that and randomenv that wouldn't do much without autoconf 07:18 * dongcarl is letting the convo play out but keeping present 07:18 < fanquake> dongcarl: how long did you have pegged for this meeting? I'm wary of spending too much time on 1 topic 07:18 < fanquake> If you just wanted recaps of PRs etc. 07:19 < dongcarl> I'm not worried about time unless you guys are short on time 07:19 < fanquake> I can stay up, so that's fine 07:19 < hebasto> me too 07:19 < dongcarl> I don't like forced meetings, but during these one-off ones I'd like to get convos flowing 07:19 < cfields> Nothing else on my agenda. 07:19 * jonatack watching and learning here 07:19 < dongcarl> great! 07:20 < cfields> Just don't let me distract you guys. I know I've missed lots of context. 07:20 < fanquake> Cool. cfields_ did you want to discuss that further/ponder, of did you want me to move onto another change? 07:20 < fanquake> We can always circle back around as well 07:20 < dongcarl> Yeah let's circle back 07:20 < dongcarl> I'll note it 07:20 < dongcarl> fanquake: next pr? 07:20 < fanquake> Ok. #18359 07:20 < gribble> https://github.com/bitcoin/bitcoin/issues/18359 | build: fix sysctl() detection on macOS by fanquake · Pull Request #18359 · bitcoin/bitcoin · GitHub 07:20 < cfields> fanquake: maybe do a quick hack to add them into configure and we can discuss if that's reasonable? 07:21 < fanquake> cfields sure. dongcarl can you note that too? 07:21 < cfields> (or that might illustrate why it wasn't done that way in the first place) 07:21 < dongcarl> fanquake: done 07:21 < dongcarl> I think 18359 looks good to go, right? 07:21 < fanquake> 18359 is a fix for sysctl() detection on macOS. The parameter types are slightly different between macOS and BSD. So for master right now, sysctl() detection fails on macOS. 07:22 < fanquake> maybe grab an ACK from cfields too? 07:22 < cfields> Now I'm curious... 07:22 < fanquake> As I mentioned earlier. I think that sysctl related detection in configure is still a bit messy, but maybe we can come back and fix it up later? 07:22 < cfields> How'd you find it? 07:22 < fanquake> -Wundef 07:23 < fanquake> When I was looking at some #ifdef related changes 07:23 < cfields> Ah, heh, trying to configure with that option and couldn't make it through configure itself? 07:23 < fanquake> It would throw warnings during compile 07:23 < cfields> I see. 07:23 < fanquake> and I didn't think they looked correct, so went digging 07:24 < fanquake> I think I'd like to turn that warning on at some point, but there are a few other things to clean up first. 07:25 < cfields> So, to clarify, we're switching it to the looser calling convention, so that it now happens to work on both? 07:25 < dongcarl> fanquake: What were you thinking as an alternative sysctl detection mechanism? 07:26 < fanquake> cfields: correct. The change in the PR works on macOS and I tested on FreeBSD and OpenBSD 07:26 < fanquake> dongcarl: I'm not really sure yet. Maybe I'm incorrect in thinking it "feels" a bit messy. 07:27 < fanquake> cfields: I did consider casing per platform in the check, but thought that could get messy. 07:27 < cfields> fanquake: I wonder if we could side-step the issue entirely using unnamed vars? 07:27 < hebasto> is looser calling convention safe? 07:28 < cfields> sysctl({CTL_KERN, KERN_VERSION}, 2, nullptr, nullptr, nullptr, 0) 07:28 < fanquake> cfields: I can test that 07:29 < dongcarl> cfields: why is that better than what fanquake has? 07:29 < dongcarl> hebasto: 🤷‍♂️ 07:31 < cfields> dongcarl: trying to get away from having to specify exactly what the constant's type is, letting it be inferred instead. 07:31 < fanquake> autoconf doesn't like sysctl({CTL_KERN, KERN_VERSION}, 2, nullptr, nullptr, nullptr, 0) very much 07:31 < cfields> Not sure if it'll work, though. 07:31 < fanquake> configure.ac:936: ERROR: end of file in string 07:31 < cfields> blah, ok, I'll play with that one. 07:31 < dongcarl> cfields: What's the advantage for letting it be inferred? 07:31 < cfields> Otherwise, sounds like a great fix. 07:32 < fanquake> Cool. Should we move to another PR? 07:32 < dongcarl> Sure let's move on 07:32 < fanquake> Ok. Next is #17929 07:32 < gribble> https://github.com/bitcoin/bitcoin/issues/17929 | build: add --enable-linker-optimizations configure flag by fanquake · Pull Request #17929 · bitcoin/bitcoin · GitHub 07:32 < cfields> dongcarl: avoiding issues like this where a slight function signature difference ends up tripping us up. 07:32 < dongcarl> cfields: cool 07:33 < fanquake> 17929 adds a --enable-linker-optimizations flag to configure, which if passed, passes -Wl,-O2 through to the linker 07:33 < fanquake> Off by default. 07:33 < fanquake> Potential to use it at gitian/guix build time in the future if we ever desired. 07:34 < fanquake> Also not LTO 07:34 < dongcarl> There was a comment about this being the default in many autotools projects... 07:34 < cfields> Huh, is that needed? I thought all the compiler drivers forwarded the opt options? 07:34 < cfields> Or is this intended to logically separate those options? 07:35 < fanquake> I dont think they always get forwarded. I was seeing differences in build output building with and without this. Let me find notes. 07:36 * dongcarl reading a bit 07:36 < hebasto> do I understand correctly that optimization possibilities of ld are lesser than lld and gold? 07:37 < dongcarl> hebasto: I think they have different optimization goals 07:37 < cfields> ok, all garzik is saying is that the link command has always had cflags/cxxflags included. 07:37 < dongcarl> ld optimizes for lookup speed, and lld optimizes for size, if I remember correctly 07:37 < hebasto> do you mean time vs size? 07:37 < dongcarl> right 07:38 < dongcarl> Okay, so I'm going to do devil's advocate here... 07:38 < cfields> so his "autotools has done this for years" comment is misleading, if the intentention is to create a separation. 07:38 < dongcarl> Why not just add "-Wl,-O2" to gitian/guix LDFLAGS 07:38 < dongcarl> and don't change configure? 07:38 < fanquake> dongcarl: I have though about this a bit. I think it 07:38 < fanquake> ahh 07:39 * cfields waits for fanquake's notes before piling on. 07:40 < fanquake> I think it's nice to have the ability to turn some of these things on from configure. Which was also my thinking with #18135 07:40 < gribble> https://github.com/bitcoin/bitcoin/issues/18135 | build: add --enable-determinism configure flag by fanquake · Pull Request #18135 · bitcoin/bitcoin · GitHub 07:40 < fanquake> I have a couple other things I'd like to bundle into that, and having the ability to pass that to configure, and get deterministic builds is pretty useful, without having to go through gitian etc. 07:41 < fanquake> It might also make a transition away from libfaketime easier. I think that is not unrealistic at this point, but just might not be worth trying to do. 07:41 < fanquake> cfields: I'm searching for those! 07:42 < fanquake> So, I can give you some hash table size figures, which was part of the difference I was seeing when the opt flags were being passed 07:42 < cfields> fanquake: I'm doing my own local testing here and it looks like you're absolutely right. 07:43 * fanquake glad he isn't too crazy 07:43 < dongcarl> fanquake: How about we bundle it into enable-determinism? 07:43 < fanquake> Trying to convert my crappy notes into markdown 07:44 < dongcarl> Because it seem a little weird to have a wrapper flag around a _single_ LDFLAG 07:44 < dongcarl> But it does make sense for a configure flag to apply a "profile" of flags 07:44 < dongcarl> Does that make sense? 07:44 < fanquake> https://gist.github.com/fanquake/d68d23377f2cbfe9e6b12f94b26c6d9f 07:45 < fanquake> Still looks average, but if your interested in some of the figures I collected a while back, they are in there. I will likely re-measure/test again soon. 07:46 < fanquake> dongcarl: so we'd have a "make deterministic and optimize" flag? 07:46 < cfields> fanquake: I'm also not convinced this is something we need to worry about. Especially if the opt flags are likely to get more complicated over time. 07:47 < cfields> fanquake: Does ./configure "LDFLAGS=-Wl,-O2" work as expected? 07:47 < fanquake> cfields: not convinced in having it in the build system, or us using it for releases etc? or either? 07:47 < cfields> (I'm not saying we shouldn't look into using it, just not sure it makes sense to account for it in our buildsystem) 07:48 < cfields> Just the former. 07:48 < fanquake> Ok. So maybe a "adding it to LDFLAGS in gitian" approach sometime in future would be preferable? 07:48 < fanquake> as dongcarl mentioned earlier 07:49 < cfields> Yeah, +1 for dongcarl's approach. 07:49 < dongcarl> Yeah I think perhaps as-soon-as-we-take-a-look-at-numbers instead of "sometime in the future" 07:49 < fanquake> dongcarl: ok. I can rebenchmark, and convert that PR to your approach if you're interested? 07:49 < cfields> sure, no reason not to. 07:50 < dongcarl> fanquake: Yes, that would be excellent! 07:50 < fanquake> Cool. Could you make a note of that too please, so I wont forget 07:50 < fanquake> Probably time to switchup PRs 07:50 < hebasto> do I understand correctly that optimization building system in general (not release) is not a goal? 07:51 < dongcarl> fanquake: Thanks for digging up the flag though, it seems very valuable 07:51 < dongcarl> Okay, fanquake do you have other urgent ones or shall we move on to others? 07:51 < fanquake> I have 1 more short one, which is more just an update 07:52 < cfields> hebasto: we can't account for _every_ combination of optimizations, so it generally makes sense to leave those options up to the person doing configure. 07:52 < dongcarl> hebasto: what might be helpful is documentation on configure flags 07:52 < dongcarl> fanquake: go ahead! 07:52 < fanquake> Just wanted to note that I got a response from the linker/loader architect at Apple, which I posted in #18295 07:52 < gribble> https://github.com/bitcoin/bitcoin/issues/18295 | build: teach ld64 to actually insert MH_BINDATLOAD by fanquake · Pull Request #18295 · bitcoin/bitcoin · GitHub 07:52 < dongcarl> OH THAT'S AWESOME 07:53 < fanquake> It looks like teaching LD64 to insert MH_BINDATLOAD might not be the optimal approach. Instead we could look for some different data in otool output as part of our symbol/security checks. 07:53 < fanquake> I'd like to hear cfields thoughts though 07:54 < dongcarl> Yeah I'm a little lost on this one. However, whatever solution we come up with at the end, perhaps we should email Nick to verify that it makes sense to him? 07:54 < fanquake> Also unclear if Apple if going to correct their documentation going forward? As it's still misleading to say the linker will insert the flag when it doesn't 07:54 < cfields> yeah, I'm not really satisfied with that response. 07:55 < cfields> well, a few things to unpack there. 07:55 < fanquake> (the loader never looks for it either) 07:56 < cfields> mmm, actually, I'll need to page all that back into my head before I say something dumb. I'll look into that one again today. That's a fun one :) 07:56 < cfields> fanquake: that's cool that you got a response. Great work! 07:57 < fanquake> cfields: cool. I'm glad we are following it up. Hopefully get some sort of closure soon. 07:57 < fanquake> Ok. Someone else should suggest a PR 07:57 < fanquake> I've taken up too much time 07:57 < dongcarl> Okay sounds good 07:58 < dongcarl> Let's do hebasto's chain! 07:58 < hebasto> it begins from #18297 07:58 < gribble> https://github.com/bitcoin/bitcoin/issues/18297 | build: Use pkg-config in BITCOIN_QT_CONFIGURE for all hosts including Windows by hebasto · Pull Request #18297 · bitcoin/bitcoin · GitHub 07:58 < cfields> Ahhh! Thank you! 07:58 < cfields> :) 07:58 < hebasto> and the final goal is #18307 07:58 < gribble> https://github.com/bitcoin/bitcoin/issues/18307 | build: Require pkg-config for all of the hosts by hebasto · Pull Request #18307 · bitcoin/bitcoin · GitHub 07:59 < cfields> Concept ACK. 07:59 < cfields> Will go through and (concept) ack the PRs. 07:59 < cfields> Any specific gotchas to discuss? 08:00 < dongcarl> There's a lot of code removal in these PRs, and if they don't introduce regressions, that's great news :-) 08:00 < hebasto> not sure if it is broken in right way to make review easier... 08:00 < hebasto> there is a regression in 18297 08:01 < hebasto> building on windows if depends built with DEBUG=1 08:01 < dongcarl> The progression is: #18297 -> #18361 -> #18298 -> #18307 08:01 < gribble> https://github.com/bitcoin/bitcoin/issues/18297 | build: Use pkg-config in BITCOIN_QT_CONFIGURE for all hosts including Windows by hebasto · Pull Request #18297 · bitcoin/bitcoin · GitHub 08:01 < gribble> https://github.com/bitcoin/bitcoin/issues/18361 | build: Make the help string for --with-gui configure option unambiguous by hebasto · Pull Request #18361 · bitcoin/bitcoin · GitHub 08:01 < gribble> https://github.com/bitcoin/bitcoin/issues/18307 | build: Require pkg-config for all of the hosts by hebasto · Pull Request #18307 · bitcoin/bitcoin · GitHub 08:01 < gribble> https://github.com/bitcoin/bitcoin/issues/18298 | build: Fix Qt processing of configure script for depends with DEBUG=1 by hebasto · Pull Request #18298 · bitcoin/bitcoin · GitHub 08:01 < hebasto> right 08:02 < fanquake> hebasto: what is your test plan for most of the changes? Mostly running gitian builds? 08:02 < hebasto> fanquake: yes 08:02 < cfields> dongcarl: hah, thanks for that. 08:03 < dongcarl> We should check that native builds work as well somehow 08:03 < dongcarl> I guess travis will check that? 08:04 < fanquake> dongcarl: somewhat. We'll probably want to do some testing in other VMs as well 08:04 < fanquake> Do we build with Qt on most of travis? 08:05 < dongcarl> MarcoFalke: Would you know? 08:05 < fanquake> Had a quick look in /ci/test/ and I see a fair bit of qt 08:06 < dongcarl> I think it would be good for us to take 5 mins to look at the first PR in the chain 08:06 < jonatack> only the first amd64 description mentions "no gui" 08:06 < dongcarl> And see if it needs to be broken down 08:06 < fanquake> There are some smaller changes in there, that could be pulled out. Depends if they are dependant on the first commit or not. 08:07 < fanquake> dongcarl: looking at 18297 ? 08:07 < dongcarl> Right, #18297 08:07 < gribble> https://github.com/bitcoin/bitcoin/issues/18297 | build: Use pkg-config in BITCOIN_QT_CONFIGURE for all hosts including Windows by hebasto · Pull Request #18297 · bitcoin/bitcoin · GitHub 08:08 < cfields> So.. first question... 08:08 < cfields> Has the pkg-config fix been upstreamed to qt? Or a discussion started there? 08:08 * fanquake was also curious 08:09 < hebasto> cfields: yes, since 5.14.0 08:09 < cfields> This is one we really don't want to have to carry ourselves, because we'll likely have to touch it for lots of qt bumps. 08:09 < cfields> hebasto: ah, it's fixed there? 08:09 < hebasto> https://bugreports.qt.io/browse/QTBUG-4155 08:10 < cfields> hebasto: great! 08:10 < cfields> hebasto: Is it possible to backport their patch directly? So that it contains the commit message? 08:11 < fanquake> ^ 08:11 < fanquake> Having some info in the patch would be handy 08:12 < cfields> IIRC some of our other patches are done that way. 08:12 < hebasto> not sure, will look into it 08:12 < fanquake> It looks like our patch doesn't quite match upstream when the change was commited 08:12 < hebasto> cfields: examples? 08:12 < cfields> hebasto: looking, sec :) 08:12 < fanquake> hebasto: https://github.com/bitcoin/bitcoin/blob/master/depends/patches/zeromq/0001-fix-build-with-older-mingw64.patch 08:13 < fanquake> Or: https://github.com/bitcoin/bitcoin/blob/master/depends/patches/zeromq/0002-disable-pthread_set_name_np.patch 08:13 -!- lukedashjr [~luke-jr@unaffiliated/luke-jr] has joined #bitcoin-builds 08:13 < cfields> ^^ 08:14 < hebasto> their patch seems in conflict with ours previous one 08:15 < dongcarl> we can "rebase" their patch but keep the comment? 08:15 -!- luke-jr [~luke-jr@unaffiliated/luke-jr] has quit [Ping timeout: 256 seconds] 08:15 < cfields> hebasto: ok, don't worry about it too much. I see you referenced the bug report... 08:15 < cfields> hebasto: it would be helpful to have a comment somewhere, though, that we can drop when we bump to 5.14. 08:15 < hebasto> sure 08:15 < cfields> Though I guess it'll be obvious when trying to re-apply a patch :) 08:16 * fanquake silently wishes qt will be someone elses problem before we have to bump to 5.14 08:16 < cfields> Heh 08:17 -!- lukedashjr is now known as luke-jr 08:18 < fanquake> cfields: if you're doing build stuff today, could you also add #17874 to your review list? 08:18 < gribble> https://github.com/bitcoin/bitcoin/issues/17874 | build: make linker checks more robust by fanquake · Pull Request #17874 · bitcoin/bitcoin · GitHub 08:18 < dongcarl> hebasto: anything else you wanted to discuss about your chain of PRs? 08:19 < cfields> fanquake: oh, forgot about that one too. Sure thing. 08:19 < cfields> fanquake: heh, I forgot how much we talked about while you were here. Thanks for keeping up with all of it! 08:19 < hebasto> dongcarl: I think the first pr in the chain is a good start 08:19 < fanquake> cfields: heh. I'm just hanging out to get back over.. 08:19 < cfields> hebasto: the first PR looks good at first glance. I'll look at that one in detail today. 08:20 < hebasto> cfields: thanks 08:20 < dongcarl> Okay so, cfields do you have anything you want to point us to? 08:21 < cfields> dongcarl: heh, nope. Just lots of review to catch up on. 08:21 < hebasto> could we discuss #18349? 08:21 < gribble> https://github.com/bitcoin/bitcoin/issues/18349 | build: Fix quick hack for version string in releases by hebasto · Pull Request #18349 · bitcoin/bitcoin · GitHub 08:21 * cfields runs away 08:22 < dongcarl> hahaha 08:22 < cfields> a quick high-level point before looking into this... 08:23 < cfields> This stuff is a mess, brought on by conflicting approaches in the way tools handled this stuff about 10 years ago. It's little more than tangled nonsense now. 08:23 < cfields> So... don't try too hard to find reason in the current approach. It needs an overhaul. 08:23 * cfields clicks 08:24 < hebasto> it was interesting to dig into it ;) 08:24 < cfields> In fact, obj/build.h was a hack that I kept when originally porting to the autotools buildsystem. This stuff is really old :) 08:25 < dongcarl> cfields: Always good to have the context! 08:25 < jonatack> hebasto: i'm digging these PRs that remove code. sometimes i wish the description gave more info on how to test. 08:26 < cfields> The reason I say all that is that there may be a smarter way to handle this these days. I wonder how newer projects embed the string.. 08:26 < hebasto> jonatack: making gitian builds 08:27 < fanquake> ^ and using diffoscope 08:27 < hebasto> cfields: there is an opinion that git and autotools are not friends ;) 08:27 < cfields> Wait. What's with this about requiring autogen? 08:27 < cfields> That... defeats the entire purpose of autotools. 08:28 < jonatack> hebasto: fanquake: thanks. fwiw the review club hasn't done any build PRs. maybe good to host some to begin onboarding more build reviewers. 08:29 < dongcarl> cfields: Yes, this is the part of the convo I wanted to have 08:29 < dongcarl> cfields: Could you elaborate a little on this? 08:29 < dongcarl> jonatack: Perhaps that would be a good idea, esp if TheCharlatan can be there 08:30 < cfields> dongcarl: when you download a bootstrapped tarball, all you need is a working bash shell and 'make 08:30 < cfields> ' 08:31 < cfields> the job of autotools is to turn all of the crazy build stuff into a single .sh. 08:31 < hebasto> yeah, that is the autotools purpose 08:31 < cfields> I mean, maybe it's worth abandoning that goal, but that means that it's just a giant complicated abstraction :( 08:33 < dongcarl> Yeah I think it's a hard call 08:34 < dongcarl> Is there a way to fix the version string hack 08:34 < cfields> What's the gain? A more accurately generated embedded version? 08:34 < dongcarl> without requiring autogen? 08:35 < cfields> dongcarl: it's a complicated problem because we're catering to lots of build configuration. 08:35 < dongcarl> "Is there a way to fix the version string hack without requiring autogen?" is my full question 08:36 < cfields> For tagged releases, we want to embed the release version, and do no configuration. For self-configure, it should change every time and ignore hard-coded values. Those concepts are generally in conflict with each-other. 08:36 < hebasto> autogen is required after #18331 08:36 < gribble> https://github.com/bitcoin/bitcoin/issues/18331 | build: Use git archive as source tarball by hebasto · Pull Request #18331 · bitcoin/bitcoin · GitHub 08:36 < cfields> NACK 18331. 08:37 < cfields> Is there some context I should catch up on? 08:37 < dongcarl> cfields: There's convo in the PR description 08:37 < hebasto> cfields: https://github.com/bitcoin/bitcoin/pull/17097#issuecomment-540691850 08:39 < cfields> Ok. Good chance I'm just flat-out stuck in the past on this one, and need to catch up. 08:39 < cfields> Will think on that some. 08:41 < dongcarl> Sure, I'm not sure what the best solution is either, but perhaps we can look at how other projects handle this 08:41 < cfields> Wait, no, there are a bunch of bootstrapped files that need to be added. 08:41 < cfields> Grrr... 08:41 < cfields> So, some context on this: 08:41 * dongcarl is excited 08:41 * fanquake is still here just slowly falling asleep 08:42 < cfields> Autotools wants you to add every file, individually, by hand. So that when you type 'make dist', you know exactly what you're getting... 08:42 < dongcarl> Right 08:42 < cfields> That was a huge improvement over what came before, which was "tar czf source.tar.gz src/" 08:42 < cfields> Which often included lots of binaries, temp files, etc. 08:43 < hebasto> git archive is not the same as "tar czf source.tar.gz src/" 08:43 < cfields> So.. the goal of 'make dist' is to make sure that _not only_ are all the necessary files are included, but _also_ that no extra files are there. 08:44 < cfields> I'm just trying to adjust my thinking. Because originally, this stuff was done by hand by a single person, and uploaded somewhere. 08:45 < cfields> But we're faaaaar from there now :) 08:46 < dongcarl> That's true 08:47 < hebasto> #16734 by MarcoFalke 08:47 < gribble> https://github.com/bitcoin/bitcoin/issues/16734 | Generate release tarball with git archive · Issue #16734 · bitcoin/bitcoin · GitHub 08:48 < dongcarl> Yeah I'm not sure what the best solution is here 08:48 < dongcarl> It's almost a human problem 08:49 < dongcarl> It should be: if you add a file that should be in the tarball, always make sure to modify *_DIST 08:49 < dongcarl> Anyway, I think we can all think about it in the back of our minds 08:49 < dongcarl> No hurry on this one, if that's okay with you hebasto/ 08:49 < hebasto> ok 08:49 < cfields> dongcarl: Agree, and I've never thought that to be too much of a sacrifice. In fact, I think it's a good thing that all sources are listed. 08:50 < dongcarl> 👍 08:50 < cfields> But judging by the comments, others don't agree with that. 08:50 < dongcarl> Alright 08:50 < dongcarl> I think we should call it a day 08:50 < dongcarl> or a meeting rather 08:51 < fanquake> Sounds good 08:51 < dongcarl> Thanks everyone for coming out 08:51 < hebasto> agree 08:51 < cfields> +1 08:51 < fanquake> I think this was pretty productive 08:51 < dongcarl> Yeah very productive 08:51 < cfields> Thanks for putting it together :) 08:51 < dongcarl> Sounds good, next time we feel the backlog coming, let's do this again 08:51 < dongcarl> #endmeeting 08:52 < fanquake> yea thanks dongcarl 08:52 * fanquake sleeps 08:52 < hebasto> dongcarl: thank you 08:52 * jonatack waves 08:53 < jonatack> learning and assimilating -- thanks everyone 09:01 < cfields> Another approach to this we've never discussed: committing the bootstrapped files. 09:02 < cfields> That's a hassle, and probably a bad idea for us, but it is another way. 09:02 < cfields> IIRC gcc works like that, for example. 09:02 < dongcarl> Right, not sure if good idea but could be good to think about 09:06 < dongcarl> Okay a few notes from the discussion: 09:06 < dongcarl> 1. For #18358, we should investigate how to detect features (i.e. check for function availability at configure time), rather than detecting platforms. This will be more robust. 09:06 < dongcarl> 2. For #17929, we should switch to just adding the -Wl,-O2 to LDFLAGs in Gitian/Guix, and benchmark to see the differences. 09:06 < gribble> https://github.com/bitcoin/bitcoin/issues/18358 | util: fix compilation with mingw-w64 7.0.0 by fanquake · Pull Request #18358 · bitcoin/bitcoin · GitHub 09:06 < gribble> https://github.com/bitcoin/bitcoin/issues/17929 | build: add --enable-linker-optimizations configure flag by fanquake · Pull Request #17929 · bitcoin/bitcoin · GitHub 09:12 -!- sonofhan [~sonofhan@ip72-193-7-145.lv.lv.cox.net] has joined #bitcoin-builds 09:17 -!- sonofhan [~sonofhan@ip72-193-7-145.lv.lv.cox.net] has quit [Ping timeout: 250 seconds] 09:24 -!- sonofhan [~sonofhan@ip72-193-7-145.lv.lv.cox.net] has joined #bitcoin-builds 09:45 -!- jb55 [~jb55@gateway/tor-sasl/jb55] has joined #bitcoin-builds 10:16 -!- sonofhan [~sonofhan@ip72-193-7-145.lv.lv.cox.net] has quit [Quit: Leaving...] 12:12 < TheCharlatan> thanks for pinging dongcarl , would indeed be interested. 13:18 < jonatack> TheCharlatan: would be great if you'd like to host a review club session, ping jnewbery or comment at https://github.com/bitcoin-core-review-club/bitcoin-core-review-club.github.io/issues/14 14:23 -!- Merle66White [~Merle66Wh@ns334669.ip-5-196-64.eu] has joined #bitcoin-builds 16:08 -!- Merle66White [~Merle66Wh@ns334669.ip-5-196-64.eu] has quit [Ping timeout: 250 seconds] 16:43 -!- sonofhan [~sonofhan@ip72-193-7-145.lv.lv.cox.net] has joined #bitcoin-builds 16:43 -!- sonofhan [~sonofhan@ip72-193-7-145.lv.lv.cox.net] has quit [Remote host closed the connection] 21:02 -!- jb55 [~jb55@gateway/tor-sasl/jb55] has quit [Ping timeout: 240 seconds] 23:10 -!- meshcollider [meshcollid@gateway/shell/ircnow/x-eocmmwsmzrsbaslx] has quit [Ping timeout: 256 seconds] 23:17 -!- meshcollider [meshcollid@gateway/shell/ircnow/x-jtzbynqfgtrhnaly] has joined #bitcoin-builds 23:31 -!- meshcollider [meshcollid@gateway/shell/ircnow/x-jtzbynqfgtrhnaly] has quit [Remote host closed the connection] 23:57 -!- meshcollider [meshcollid@gateway/shell/ircnow/x-crzloldrvcwcmbym] has joined #bitcoin-builds --- Log closed Thu Mar 19 00:00:28 2020