http://gnusha.org/bitcoin-builds/2020-03-18.log
14:00 <dongcarl> Hello
14:00 <hebasto> hi
14:00 <dongcarl> jonatack hebasto fanquake cfields_
14:00 <fanquake> yo
14:00 <jonatack> hey
14:01 <dongcarl> summoning cory gimme a sec
14:01 * cfields_ present!
14:02 <dongcarl> Woohoo
14:02 <dongcarl> Alright
14:02 <dongcarl> Let's get started
14:02 <dongcarl> Let's do discussion topics first
14:02 <dongcarl> I know fanquake you had one?
14:02 <fanquake> i have a few things in flight
14:02 <fanquake> don't want to start with fd accoutning though
14:03 <fanquake> unless you mean the _MSC_VER stuff?
14:03 <dongcarl> I see
14:03 <dongcarl> How about we go thru your PRs then?
14:03 <dongcarl> The ones you want attention on?
14:03 <dongcarl> And people can ask questions and discuss
14:03 <fanquake> Sure. Can start with #18358 if you want
14:03 <gribble> bitcoin/bitcoin#18358 | util: fix compilation with mingw-w64 7.0.0 by fanquake · Pull Request #18358 · bitcoin/bitcoin · GitHub
14:03 <hebasto> testing right now
14:03 <dongcarl> Yeah that sounds good
14:03 <fanquake> I also answered your question hebasto
14:04 <hebasto> ty
14:04 <fanquake> The underlying mingw-w64 headers being used are 5.0.3
14:04 <fanquake> (on ubuntu bionic)
14:04 <dongcarl> So I think the main fallout from that is we need to use the right detection, correct?
14:04 <cfields> solution sounds good if you're confident that everything in the description is correct.
14: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.
14:05 <dongcarl> fanquake: So the overarching goal is detecting when we're targeting mingw-w64 right?
14:05 <cfields> Oh wait, I didn't look at the actual changes.
14:05 <dongcarl> Or just windows in general?
14:05 <hebasto> fanquake: maybe submit an issue to track them all
14:05 <dongcarl> cfields: Yeah to be clear we are not adding -D, we're using gmtime_s instead
14:05 <fanquake> dongcarl I think it's more Windows in general
14:06 <fanquake> So to give an example
14: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?
14: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
14:07 <fanquake> Are you thinking of SECURE_API?
14:07 <dongcarl> cfields: With the current PR we don't need the extensions
14:07 <cfields> No, I mean specifically for gmtime_s/gmtime_r.
14:07 <cfields> I just need a min to catch up. You guys go ahead :)
14: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.
14:08 <cfields> IIRC neither gmtime_r, nor gmtime_s are standard, they're both extensions.
14:08 <fanquake> Because we wont fall into the _MSC_VER block
14:08 <fanquake> We'll fall into here https://github.com/bitcoin/bitcoin/blob/master/src/random.cpp#L63 instead
14: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?
14: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.
14:10 <dongcarl> fanquake: I see, lemme think a bit
14:10 <dongcarl> cfields: Apparently not in mingw-w64
14:10 <fanquake> SecureZeroMemory is another example: https://github.com/bitcoin/bitcoin/blob/master/src/support/cleanse.cpp#L18
14:10 <dongcarl> cfields: There's no mention of __STDC_LIB_EXT1__ anywhere
14:11 <cfields> I'm worried that you guys are trying to target platforms rather than features.
14:12 <dongcarl> Okay let
14:12 <cfields> but I'll comment on the PR, definitely need to catch up some.
14:12 <dongcarl> Sounds good
14:12 <dongcarl> fanquake: I think I see what you're trying to say
14:13 <fanquake> cfields: do you. mean targetting Windows rather than determining the existence of a certain function?
14:14 <cfields> fanquake: right. "#if platform" is always a last resort.
14:14 <dongcarl> Oh I see...
14: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?
14:14 <dongcarl> Perhaps the right thing to do is to check for functions?
14:15 <hebasto> in configure?
14:15 <fanquake> So we should be able to use something like a WIN32 define, rather than MSVC + MINGW ?
14: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.
14:16 <fanquake> Is that in regards to __rand()?
14: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"
14:17 <cfields> Ah, unless that file's meant to be usable outside of autoconf?
14:18 <fanquake> random.cpp ? There's a lot in that and randomenv that wouldn't do much without autoconf
14:18 * dongcarl is letting the convo play out but keeping present
14:18 <fanquake> dongcarl: how long did you have pegged for this meeting? I'm wary of spending too much time on 1 topic
14:18 <fanquake> If you just wanted recaps of PRs etc.
14:19 <dongcarl> I'm not worried about time unless you guys are short on time
14:19 <fanquake> I can stay up, so that's fine
14:19 <hebasto> me too
14:19 <dongcarl> I don't like forced meetings, but during these one-off ones I'd like to get convos flowing
14:19 <cfields> Nothing else on my agenda.
14:19 * jonatack watching and learning here
14:19 <dongcarl> great!
14:20 <cfields> Just don't let me distract you guys. I know I've missed lots of context.
14:20 <fanquake> Cool. cfields_ did you want to discuss that further/ponder, of did you want me to move onto another change?
14:20 <fanquake> We can always circle back around as well
14:20 <dongcarl> Yeah let's circle back
14:20 <dongcarl> I'll note it
14:20 <dongcarl> fanquake: next pr?
14:20 <fanquake> Ok. #18359
14:20 <gribble> bitcoin/bitcoin#18359 | build: fix sysctl() detection on macOS by fanquake · Pull Request #18359 · bitcoin/bitcoin · GitHub
14:20 <cfields> fanquake: maybe do a quick hack to add them into configure and we can discuss if that's reasonable?
14:21 <fanquake> cfields sure. dongcarl can you note that too?
14:21 <cfields> (or that might illustrate why it wasn't done that way in the first place)
14:21 <dongcarl> fanquake: done
14:21 <dongcarl> I think 18359 looks good to go, right?
14: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.
14:22 <fanquake> maybe grab an ACK from cfields too?
14:22 <cfields> Now I'm curious...
14: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?
14:22 <cfields> How'd you find it?
14:22 <fanquake> -Wundef
14:23 <fanquake> When I was looking at some #ifdef related changes
14:23 <cfields> Ah, heh, trying to configure with that option and couldn't make it through configure itself?
14:23 <fanquake> It would throw warnings during compile
14:23 <cfields> I see.
14:23 <fanquake> and I didn't think they looked correct, so went digging
14: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.
14:25 <cfields> So, to clarify, we're switching it to the looser calling convention, so that it now happens to work on both?
14:25 <dongcarl> fanquake: What were you thinking as an alternative sysctl detection mechanism?
14:26 <fanquake> cfields: correct. The change in the PR works on macOS and I tested on FreeBSD and OpenBSD
14:26 <fanquake> dongcarl: I'm not really sure yet. Maybe I'm incorrect in thinking it "feels" a bit messy.
14:27 <fanquake> cfields: I did consider casing per platform in the check, but thought that could get messy.
14:27 <cfields> fanquake: I wonder if we could side-step the issue entirely using unnamed vars?
14:27 <hebasto> is looser calling convention safe?
14:28 <cfields> sysctl({CTL_KERN, KERN_VERSION}, 2, nullptr, nullptr, nullptr, 0)
14:28 <fanquake> cfields: I can test that
14:29 <dongcarl> cfields: why is that better than what fanquake has?
14:29 <dongcarl> hebasto: 🤷�♂�
14:31 <cfields> dongcarl: trying to get away from having to specify exactly what the constant's type is, letting it be inferred instead.
14:31 <fanquake> autoconf doesn't like sysctl({CTL_KERN, KERN_VERSION}, 2, nullptr, nullptr, nullptr, 0) very much
14:31 <cfields> Not sure if it'll work, though.
14:31 <fanquake> configure.ac:936: ERROR: end of file in string
14:31 <cfields> blah, ok, I'll play with that one.
14:31 <dongcarl> cfields: What's the advantage for letting it be inferred?
14:31 <cfields> Otherwise, sounds like a great fix.
14:32 <fanquake> Cool. Should we move to another PR?
14:32 <dongcarl> Sure let's move on
14:32 <fanquake> Ok. Next is #17929
14:32 <gribble> bitcoin/bitcoin#17929 | build: add --enable-linker-optimizations configure flag by fanquake · Pull Request #17929 · bitcoin/bitcoin · GitHub
14:32 <cfields> dongcarl: avoiding issues like this where a slight function signature difference ends up tripping us up.
14:32 <dongcarl> cfields: cool
14:33 <fanquake> 17929 adds a --enable-linker-optimizations flag to configure, which if passed, passes -Wl,-O2 through to the linker
14:33 <fanquake> Off by default.
14:33 <fanquake> Potential to use it at gitian/guix build time in the future if we ever desired.
14:34 <fanquake> Also not LTO
14:34 <dongcarl> There was a comment about this being the default in many autotools projects...
14:34 <cfields> Huh, is that needed? I thought all the compiler drivers forwarded the opt options?
14:34 <cfields> Or is this intended to logically separate those options?
14: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.
14:36 * dongcarl reading a bit
14:36 <hebasto> do I understand correctly that optimization possibilities of ld are lesser than lld and gold?
14:37 <dongcarl> hebasto: I think they have different optimization goals
14:37 <cfields> ok, all garzik is saying is that the link command has always had cflags/cxxflags included.
14:37 <dongcarl> ld optimizes for lookup speed, and lld optimizes for size, if I remember correctly
14:37 <hebasto> do you mean time vs size?
14:37 <dongcarl> right
14:38 <dongcarl> Okay, so I'm going to do devil's advocate here...
14:38 <cfields> so his "autotools has done this for years" comment is misleading, if the intentention is to create a separation.
14:38 <dongcarl> Why not just add "-Wl,-O2" to gitian/guix LDFLAGS
14:38 <dongcarl> and don't change configure?
14:38 <fanquake> dongcarl: I have though about this a bit. I think it
14:38 <fanquake> ahh
14:39 * cfields waits for fanquake's notes before piling on.
14: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
14:40 <gribble> bitcoin/bitcoin#18135 | build: add --enable-determinism configure flag by fanquake · Pull Request #18135 · bitcoin/bitcoin · GitHub
14: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.
14: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.
14:41 <fanquake> cfields: I'm searching for those!
14: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
14:42 <cfields> fanquake: I'm doing my own local testing here and it looks like you're absolutely right.
14:43 * fanquake glad he isn't too crazy
14:43 <dongcarl> fanquake: How about we bundle it into enable-determinism?
14:43 <fanquake> Trying to convert my crappy notes into markdown
14:44 <dongcarl> Because it seem a little weird to have a wrapper flag around a _single_ LDFLAG
14:44 <dongcarl> But it does make sense for a configure flag to apply a "profile" of flags
14:44 <dongcarl> Does that make sense?
14:44 <fanquake> https://gist.github.com/fanquake/d68d23377f2cbfe9e6b12f94b26c6d9f
14: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.
14:46 <fanquake> dongcarl: so we'd have a "make deterministic and optimize" flag?
14: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.
14:47 <cfields> fanquake: Does ./configure "LDFLAGS=-Wl,-O2" work as expected?
14:47 <fanquake> cfields: not convinced in having it in the build system, or us using it for releases etc? or either?
14: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)
14:48 <cfields> Just the former.
14:48 <fanquake> Ok. So maybe a "adding it to LDFLAGS in gitian" approach sometime in future would be preferable?
14:48 <fanquake> as dongcarl mentioned earlier
14:49 <cfields> Yeah, +1 for dongcarl's approach.
14:49 <dongcarl> Yeah I think perhaps as-soon-as-we-take-a-look-at-numbers instead of "sometime in the future"
14:49 <fanquake> dongcarl: ok. I can rebenchmark, and convert that PR to your approach if you're interested?
14:49 <cfields> sure, no reason not to.
14:50 <dongcarl> fanquake: Yes, that would be excellent!
14:50 <fanquake> Cool. Could you make a note of that too please, so I wont forget
14:50 <fanquake> Probably time to switchup PRs
14:50 <hebasto> do I understand correctly that optimization building system in general (not release) is not a goal?
14:51 <dongcarl> fanquake: Thanks for digging up the flag though, it seems very valuable
14:51 <dongcarl> Okay, fanquake do you have other urgent ones or shall we move on to others?
14:51 <fanquake> I have 1 more short one, which is more just an update
14: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.
14:52 <dongcarl> hebasto: what might be helpful is documentation on configure flags
14:52 <dongcarl> fanquake: go ahead!
14:52 <fanquake> Just wanted to note that I got a response from the linker/loader architect at Apple, which I posted in #18295
14:52 <gribble> bitcoin/bitcoin#18295 | build: teach ld64 to actually insert MH_BINDATLOAD by fanquake · Pull Request #18295 · bitcoin/bitcoin · GitHub
14:52 <dongcarl> OH THAT'S AWESOME
14: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.
14:53 <fanquake> I'd like to hear cfields thoughts though
14: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?
14: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
14:54 <cfields> yeah, I'm not really satisfied with that response.
14:55 <cfields> well, a few things to unpack there.
14:55 <fanquake> (the loader never looks for it either)
14: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 :)
14:56 <cfields> fanquake: that's cool that you got a response. Great work!
14:57 <fanquake> cfields: cool. I'm glad we are following it up. Hopefully get some sort of closure soon.
14:57 <fanquake> Ok. Someone else should suggest a PR
14:57 <fanquake> I've taken up too much time
14:57 <dongcarl> Okay sounds good
14:58 <dongcarl> Let's do hebasto's chain!
14:58 <hebasto> it begins from #18297
14:58 <gribble> bitcoin/bitcoin#18297 | build: Use pkg-config in BITCOIN_QT_CONFIGURE for all hosts including Windows by hebasto · Pull Request #18297 · bitcoin/bitcoin · GitHub
14:58 <cfields> Ahhh! Thank you!
14:58 <cfields> :)
14:58 <hebasto> and the final goal is #18307
14:58 <gribble> bitcoin/bitcoin#18307 | build: Require pkg-config for all of the hosts by hebasto · Pull Request #18307 · bitcoin/bitcoin · GitHub
14:59 <cfields> Concept ACK.
14:59 <cfields> Will go through and (concept) ack the PRs.
14:59 <cfields> Any specific gotchas to discuss?
15:00 <dongcarl> There's a lot of code removal in these PRs, and if they don't introduce regressions, that's great news :-)
15:00 <hebasto> not sure if it is broken in right way to make review easier...
15:00 <hebasto> there is a regression in 18297
15:01 <hebasto> building on windows if depends built with DEBUG=1
15:01 <dongcarl> The progression is: #18297 -> #18361 -> #18298 -> #18307
15:01 <gribble> bitcoin/bitcoin#18297 | build: Use pkg-config in BITCOIN_QT_CONFIGURE for all hosts including Windows by hebasto · Pull Request #18297 · bitcoin/bitcoin · GitHub
15:01 <gribble> bitcoin/bitcoin#18361 | build: Make the help string for --with-gui configure option unambiguous by hebasto · Pull Request #18361 · bitcoin/bitcoin · GitHub
15:01 <gribble> bitcoin/bitcoin#18307 | build: Require pkg-config for all of the hosts by hebasto · Pull Request #18307 · bitcoin/bitcoin · GitHub
15:01 <gribble> bitcoin/bitcoin#18298 | build: Fix Qt processing of configure script for depends with DEBUG=1 by hebasto · Pull Request #18298 · bitcoin/bitcoin · GitHub
15:01 <hebasto> right
15:02 <fanquake> hebasto: what is your test plan for most of the changes? Mostly running gitian builds?
15:02 <hebasto> fanquake: yes
15:02 <cfields> dongcarl: hah, thanks for that.
15:03 <dongcarl> We should check that native builds work as well somehow
15:03 <dongcarl> I guess travis will check that?
15:04 <fanquake> dongcarl: somewhat. We'll probably want to do some testing in other VMs as well
15:04 <fanquake> Do we build with Qt on most of travis?
15:05 <dongcarl> MarcoFalke: Would you know?
15:05 <fanquake> Had a quick look in /ci/test/ and I see a fair bit of qt
15:06 <dongcarl> I think it would be good for us to take 5 mins to look at the first PR in the chain
15:06 <jonatack> only the first amd64 description mentions "no gui"
15:06 <dongcarl> And see if it needs to be broken down
15: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.
15:07 <fanquake> dongcarl: looking at 18297 ?
15:07 <dongcarl> Right, #18297
15:07 <gribble> bitcoin/bitcoin#18297 | build: Use pkg-config in BITCOIN_QT_CONFIGURE for all hosts including Windows by hebasto · Pull Request #18297 · bitcoin/bitcoin · GitHub
15:08 <cfields> So.. first question...
15:08 <cfields> Has the pkg-config fix been upstreamed to qt? Or a discussion started there?
15:08 * fanquake was also curious
15:09 <hebasto> cfields: yes, since 5.14.0
15: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.
15:09 <cfields> hebasto: ah, it's fixed there?
15:09 <hebasto> https://bugreports.qt.io/browse/QTBUG-4155
15:10 <cfields> hebasto: great!
15:10 <cfields> hebasto: Is it possible to backport their patch directly? So that it contains the commit message?
15:11 <fanquake> ^
15:11 <fanquake> Having some info in the patch would be handy
15:12 <cfields> IIRC some of our other patches are done that way.
15:12 <hebasto> not sure, will look into it
15:12 <fanquake> It looks like our patch doesn't quite match upstream when the change was commited
15:12 <hebasto> cfields: examples?
15:12 <cfields> hebasto: looking, sec :)
15:12 <fanquake> hebasto: https://github.com/bitcoin/bitcoin/blob/master/depends/patches/zeromq/0001-fix-build-with-older-mingw64.patch
15:13 <fanquake> Or: https://github.com/bitcoin/bitcoin/blob/master/depends/patches/zeromq/0002-disable-pthread\_set\_name\_np.patch
15:13 <cfields> ^^
15:14 <hebasto> their patch seems in conflict with ours previous one
15:15 <dongcarl> we can "rebase" their patch but keep the comment?
15:15 <cfields> hebasto: ok, don't worry about it too much. I see you referenced the bug report...
15:15 <cfields> hebasto: it would be helpful to have a comment somewhere, though, that we can drop when we bump to 5.14.
15:15 <hebasto> sure
15:15 <cfields> Though I guess it'll be obvious when trying to re-apply a patch :)
15:16 * fanquake silently wishes qt will be someone elses problem before we have to bump to 5.14
15:16 <cfields> Heh
15:18 <fanquake> cfields: if you're doing build stuff today, could you also add #17874 to your review list?
15:18 <gribble> bitcoin/bitcoin#17874 | build: make linker checks more robust by fanquake · Pull Request #17874 · bitcoin/bitcoin · GitHub
15:18 <dongcarl> hebasto: anything else you wanted to discuss about your chain of PRs?
15:19 <cfields> fanquake: oh, forgot about that one too. Sure thing.
15:19 <cfields> fanquake: heh, I forgot how much we talked about while you were here. Thanks for keeping up with all of it!
15:19 <hebasto> dongcarl: I think the first pr in the chain is a good start
15:19 <fanquake> cfields: heh. I'm just hanging out to get back over..
15:19 <cfields> hebasto: the first PR looks good at first glance. I'll look at that one in detail today.
15:20 <hebasto> cfields: thanks
15:20 <dongcarl> Okay so, cfields do you have anything you want to point us to?
15:21 <cfields> dongcarl: heh, nope. Just lots of review to catch up on.
15:21 <hebasto> could we discuss #18349?
15:21 <gribble> bitcoin/bitcoin#18349 | build: Fix quick hack for version string in releases by hebasto · Pull Request #18349 · bitcoin/bitcoin · GitHub
15:21 * cfields runs away
15:22 <dongcarl> hahaha
15:22 <cfields> a quick high-level point before looking into this...
15: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.
15:23 <cfields> So... don't try too hard to find reason in the current approach. It needs an overhaul.
15:23 * cfields clicks
15:24 <hebasto> it was interesting to dig into it ;)
15: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 :)
15:25 <dongcarl> cfields: Always good to have the context!
15:25 <jonatack> hebasto: i'm digging these PRs that remove code. sometimes i wish the description gave more info on how to test.
15: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..
15:26 <hebasto> jonatack: making gitian builds
15:27 <fanquake> ^ and using diffoscope
15:27 <hebasto> cfields: there is an opinion that git and autotools are not friends ;)
15:27 <cfields> Wait. What's with this about requiring autogen?
15:27 <cfields> That... defeats the entire purpose of autotools.
15: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.
15:29 <dongcarl> cfields: Yes, this is the part of the convo I wanted to have
15:29 <dongcarl> cfields: Could you elaborate a little on this?
15:29 <dongcarl> jonatack: Perhaps that would be a good idea, esp if TheCharlatan can be there
15:30 <cfields> dongcarl: when you download a bootstrapped tarball, all you need is a working bash shell and 'make
15:30 <cfields> '
15:31 <cfields> the job of autotools is to turn all of the crazy build stuff into a single .sh.
15:31 <hebasto> yeah, that is the autotools purpose
15:31 <cfields> I mean, maybe it's worth abandoning that goal, but that means that it's just a giant complicated abstraction :(
15:33 <dongcarl> Yeah I think it's a hard call
15:34 <dongcarl> Is there a way to fix the version string hack
15:34 <cfields> What's the gain? A more accurately generated embedded version?
15:34 <dongcarl> without requiring autogen?
15:35 <cfields> dongcarl: it's a complicated problem because we're catering to lots of build configuration.
15:35 <dongcarl> "Is there a way to fix the version string hack without requiring autogen?" is my full question
15: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.
15:36 <hebasto> autogen is required after #18331
15:36 <gribble> bitcoin/bitcoin#18331 | build: Use git archive as source tarball by hebasto · Pull Request #18331 · bitcoin/bitcoin · GitHub
15:36 <cfields> NACK 18331.
15:37 <cfields> Is there some context I should catch up on?
15:37 <dongcarl> cfields: There's convo in the PR description
15:37 <hebasto> cfields: bitcoin/bitcoin#17097 (comment)
15:39 <cfields> Ok. Good chance I'm just flat-out stuck in the past on this one, and need to catch up.
15:39 <cfields> Will think on that some.
15:41 <dongcarl> Sure, I'm not sure what the best solution is either, but perhaps we can look at how other projects handle this
15:41 <cfields> Wait, no, there are a bunch of bootstrapped files that need to be added.
15:41 <cfields> Grrr...
15:41 <cfields> So, some context on this:
15:41 * dongcarl is excited
15:41 * fanquake is still here just slowly falling asleep
15: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...
15:42 <dongcarl> Right
15:42 <cfields> That was a huge improvement over what came before, which was "tar czf source.tar.gz src/"
15:42 <cfields> Which often included lots of binaries, temp files, etc.
15:43 <hebasto> git archive is not the same as "tar czf source.tar.gz src/"
15: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.
15: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.
15:45 <cfields> But we're faaaaar from there now :)
15:46 <dongcarl> That's true
15:47 <hebasto> #16734 by MarcoFalke
15:47 <gribble> bitcoin/bitcoin#16734 | Generate release tarball with git archive · Issue #16734 · bitcoin/bitcoin · GitHub
15:48 <dongcarl> Yeah I'm not sure what the best solution is here
15:48 <dongcarl> It's almost a human problem
15:49 <dongcarl> It should be: if you add a file that should be in the tarball, always make sure to modify *_DIST
15:49 <dongcarl> Anyway, I think we can all think about it in the back of our minds
15:49 <dongcarl> No hurry on this one, if that's okay with you hebasto/
15:49 <hebasto> ok
15: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.
15:50 <dongcarl> �
15:50 <cfields> But judging by the comments, others don't agree with that.
15:50 <dongcarl> Alright
15:50 <dongcarl> I think we should call it a day
15:50 <dongcarl> or a meeting rather
15:51 <fanquake> Sounds good
15:51 <dongcarl> Thanks everyone for coming out
15:51 <hebasto> agree
15:51 <cfields> +1
15:51 <fanquake> I think this was pretty productive
15:51 <dongcarl> Yeah very productive
15:51 <cfields> Thanks for putting it together :)
15:51 <dongcarl> Sounds good, next time we feel the backlog coming, let's do this again
15:51 <dongcarl> #endmeeting
15:52 <fanquake> yea thanks dongcarl
15:52 * fanquake sleeps
15:52 <hebasto> dongcarl: thank you
15:52 * jonatack waves
15:53 <jonatack> learning and assimilating -- thanks everyone
16:01 <cfields> Another approach to this we've never discussed: committing the bootstrapped files.
16:02 <cfields> That's a hassle, and probably a bad idea for us, but it is another way.
16:02 <cfields> IIRC gcc works like that, for example.
16:02 <dongcarl> Right, not sure if good idea but could be good to think about
16:06 <dongcarl> Okay a few notes from the discussion:
16: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.
16: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.
16:06 <gribble> bitcoin/bitcoin#18358 | util: fix compilation with mingw-w64 7.0.0 by fanquake · Pull Request #18358 · bitcoin/bitcoin · GitHub
16:06 <gribble> bitcoin/bitcoin#17929 | build: add --enable-linker-optimizations configure flag by fanquake · Pull Request #17929 · bitcoin/bitcoin · GitHub