Hi, Here's the summary of the IRC meeting.
--- COMMUNITY MEETING Place: #openvpn-meeting on irc.freenode.net Date: Wed 17th March 2021 Time: 11:30 CET (10:30 UTC) Planned meeting topics for this meeting were here: https://community.openvpn.net/openvpn/wiki/Topics-2021-03-17 Your local meeting time is easy to check from services such as <http://www.timeanddate.com/worldclock> SUMMARY cron2, d12fk, dazo, lev, mattock, ordex and plaisthos participated in this meeting. --- Noted that OpenVPN 2.5.2 release has been postponed to next week. --- Patch v2 to fix the "mbedTLS 2.25.0 crash bug / patch " is on the list, but review is still lacking. --- Mattock mentioned that there is no progress on the buildslave refactoring due to lack of focus and time. --- Agreed that we should try to let OpenVPN Inc. QA replicate the "Bridged Windows 10 Causes Sporadic Crashes" issue: <https://community.openvpn.net/openvpn/ticket/1385> Mattock will contact the person who reported this and QA will replicate the environment. Then the problem can be reproduced and fixed eventually. --- Talked about the review culture. Everyone agreed that whitespace and formatting issues are important, but those should preferably detected automatically before any human even looks at the patch. It is possible that we could do this with Patchwork and uncrustify, but that would require some effort. Agreed that as a first step we should move the code formatting instructions in the CONTRIBUTING file up. --- Talked about the technical details regarding kicking out the embedded lz4 library. Did not find a perfect solution yet. --- Full chatlog attached
(12:28:33) mattock: almost time (12:30:29) cron2: I'll be a few minutes late for the meeting (still in a call) (12:31:41) mattock: ok (12:31:44) mattock: who else do we have here? (12:32:22) ***d457k <- (12:32:44) d457k: weird nick (12:33:22) mattock: indeed :) (12:34:25) d457k è ora conosciuto come d12fk (12:34:54) modalità (+o d12fk) da ChanServ (12:35:13) mattock: I pinged the guys on internal chat (12:36:32) ***dazo is here (12:36:40) lev__: hello (12:36:46) mattock: hi! (12:37:53) ***plaisthos is here (12:38:52) dazo ha scelto come argomento: Agenda https://community.openvpn.net/openvpn/wiki/Topics-2021-03-17 (12:39:27) lev__: Page Topics-2021-03-17 not found (12:39:58) ***dazo is soooo tempted to update the topic url once more, to include the ?__cf_chl_jschl_tk__={blob} part ..... (12:40:28) dazo: https://community.openvpn.net/openvpn/wiki/Topics-2021-03-10 ... so probably catch-up from here then (12:41:40) plaisthos: for 2 that is postponed (12:41:46) mattock: let's see (12:42:05) dazo: yeah, 2 is postponed one more week (12:42:54) plaisthos: btw. our mail server is down (12:42:57) plaisthos: :/ (12:43:17) lev__: Use Outlook web access :) (12:43:34) dazo: Then living without e-mail is better (12:43:50) dazo: OWA is webmail done in the 90s (12:44:57) d12fk: are we waiting for cron2? (12:45:02) mattock: yes I think so (12:45:17) mattock: well, any news on "mbedTLS 2.25.0 crash bug / patch "? (12:45:42) plaisthos: v2 is on the list (12:45:47) plaisthos: no review yet (12:46:38) mattock: ok (12:46:55) mattock: so the question on "how to deal with it" has probably been resolved (12:47:16) plaisthos: interestingly Mail.app still works (12:47:17) mattock: regarding buildslaves - no progress, no point in me trying to 30 minutes here and 30 minutes there, needs focus (12:47:22) plaisthos: probably uses the exchange interface (12:47:49) plaisthos: but I am not looking into Mail.app+gpg (12:48:42) dazo: :-D (12:50:31) mattock: I have some updates on "Bridged Windows 10 Causes Sporadic Crashes" (https://community.openvpn.net/openvpn/ticket/1385) (12:50:49) mattock: so, the person was willing to grant access to a Windows instance with this problem (12:51:16) mattock: I recall lev almost volunteered to have a look at this (12:52:04) cron2: ok, now I'm fully here (12:52:25) mattock: welcome! (12:52:46) lev__: I asked for a stack trace from the driver (12:53:52) lev__: but I've never done bridging on windows (12:54:22) mattock: lev: what if I connect you with the guy directly? (12:54:45) mattock: he seemed reluctant to start meddling with stack traces, but maybe creating that would be quite easy (12:54:49) cron2: it seems to be "a supported feature", but a) for some people it bluescreens, and b) for other people it stopped working with the last Win10 update (12:54:49) mattock: you could instruct him (12:57:24) mattock: lev: I take silence as a "yes" :D (12:57:32) lev__: well wait (12:57:54) lev__: how much this case is important comparison to dco-win I am working on (12:58:13) mattock: probably quite unimportant (12:58:44) mattock: at least for most people, but might be really important for a small subset of users (12:59:01) cron2: lev__: way less important (12:59:18) lev__: maybe we should ask our QA to test bringing first (12:59:26) dazo: Any b0rken feature is important to those users using it .... but that doesn't mean it's important for the project as a whole (12:59:45) mattock: lev: what about asking the guy how to reproduce _his_ setup? (12:59:49) mattock: then QA could replicate that (12:59:54) mattock: no work on your part (13:00:30) cron2: might work :) (13:00:36) lev__: yeah (13:01:04) mattock: that's a deal then (13:01:13) ***ordex is here too (13:01:14) mattock: I'll ask him the technical details (13:01:32) mattock: lev: what JIRA project should I place the ticket about this? (13:01:36) mattock: hi ordex! (13:01:42) lev__: OSS ? (13:02:11) lev__: which obviously states for "Office of strategic services" (13:03:16) mattock: if QA has access to that strategic service, then yes (13:05:12) cron2: ok. Can we spend 2 minutes on "review culture"? I need to make complaints :) (13:05:31) dazo: yes, please (13:05:55) cron2: we all know that our reviews take way too long, and this is not easily fixed (13:06:01) mattock: +1 (13:06:05) dazo: +1 (13:06:12) cron2: but I've seen a few cases where after quite a long wait time, all the review said was (13:06:16) cron2: "your whitespace stinks!" (13:06:32) cron2: and I think this is very frustrating for those who write code (13:07:14) cron2: so my request is: please do the reviews based on code quality and functionality, *and* point out "there is a whitespace issue that MUST be fixed!". But do not focus on "I won't look at this before all the blanks and brackets are painted pink!" (13:07:28) cron2: minor whitespace can be handled at merge time (13:07:28) dazo: +1 (13:08:14) cron2: *major* whitespace havoc (like "a whole function is formatted totally weird") might need a v2 of the patch, but "the functionality is good, and we generally agree with the principle, but the code sucks" is a very different message than "your whitespace stinks" (13:08:34) cron2: thanks for listening :-) (13:08:42) dazo: And if the white-spacing is too much "off" .... for reviewing, pull it down, uncrustify it, review code. (13:09:40) ordex: agreed (13:10:03) dazo: Maybe we should also reconsider how we apply patches too ... that uncrustifying will be part of the commit process. If the config is not well defined in certain areas, it's up to the committer to keep some consistence (13:10:28) dazo: thoughts about that, cron2? (13:10:33) cron2: I have tried to find a reasonable balance on that (13:10:52) dazo: I have no complaints to that balance ;-) (13:10:54) cron2: like: I remove extra blank lines, and fix "if ( a<0)" mishaps (13:11:12) cron2: but I do not normally do a full uncrustify run (13:11:29) cron2: because that makes trackability "is this really the code that was submitted?" more complicated (13:11:34) cron2: like (13:12:15) dazo: fair point .... we probably need another round of "great reformatting" again; we briefly talked about doing it right before 2.5.0, but probably fell through the cracks (13:12:44) dazo: would probably help doing the commit-time uncrustify easier (13:14:36) cron2: if (a<0) foo(); (13:14:36) cron2: turning into (13:14:36) cron2: if (a < 0) (13:14:36) cron2: { (13:14:36) cron2: foo(); (13:14:36) cron2: } (13:14:37) ordex: otoh, we have little time to spend on reviewing and merging. why should this little time go on noting/fixing code style issues? (13:14:37) ordex: imho it should be responsibility of the sdubmitter to make sure code follows at least our uncrustify configuration (13:14:38) plaisthos: yeah (13:14:38) plaisthos: but the problem is that if we don't run uncrustify on every commit you end up with having to deal with a lot of changes from uncrustify not related to your own code (13:14:38) cron2: ordex: yes, but then tell them right after the patch is sent. If we leave it linger for two months, and then send "ah, your whitespace sucks", this is very frustrating, and will drive contributors away (13:14:38) plaisthos: cron2: I hope I never did that (13:14:38) ordex: well, if our submission guidelines says "make sure uncrustify is happy" we can't really be responsible for that kind of reply, no? (13:14:54) ordex: we all have little time after all, due diligence is expected on each side (13:15:05) cron2: ordex: you really like doing these whitespace-only reviews, and I think this is not helpful (13:15:44) ordex: yeah yeah, I know it is mostly me. I got this education on the kernel mailing list -> checkpatch says there are issue -> you moron go away, we don't even review your patch (13:15:49) cron2: I'm all fine with rejecting a patch on grounds of formatting *but* please do include a review on functionality and code safety (13:16:16) mattock: +1, otherwise many rounds of review will ensue (13:16:43) ordex: it's also about education..if we accept fixing style on our side, submitters will never spend time adjusting it (13:16:45) dazo: I do see both sides of the table here .... from ordex point of view, it ensures the code is always in shape. But the reality is that some cruft slips through every now and then unnoticed .... and then enforcing uncrustify gets really messy after a wile (13:16:49) dazo: *while (13:17:00) cron2: don't misunderstand me (13:17:08) cron2: I *want* well-formatted code :-) (13:17:12) ordex: hehe I know (13:17:21) ordex: and I get your point - which I tend to agree with (13:17:36) cron2: but the message needs to be a different one - *or* we find a way to auto-review and auto-reject patches based on uncrustify (13:17:46) cron2: which might be a nice addition to patchwork... :-) (13:17:50) ordex: yeah (13:18:03) ordex: the problem I guess is that we don't have a deterministic uncrustify config (13:18:08) ordex: (at the moment) (13:18:43) ordex: but if we improve that, maybe I Can also just quickly run uncrustify on a patch, instead of goig through it :D that'd be faster too (or let pw do it for us) (13:19:03) dazo: So .... is that because our config is not optimal? due to various behaviours in uncrustify versions? Or that uncrustify itself is really not up-to-the-task we expect? (13:19:34) ordex: I am not sure. I know I have seen things accepted by uncrustify, which were not the way we wanted them (13:19:42) cron2: uncrustify and ordex sometimes disagree on details (13:19:45) ordex: :D (13:19:46) d12fk: to get to the uncrustify rules it takes two hops from CONTRIBUTING.. and a text desert (13:20:08) d12fk: if it is so important we should place it more importantly (13:20:15) dazo: good point! (13:20:38) ordex: hehe, so what comes later in the list .. "does not need to be respected"? :D (13:20:40) cron2: +1 (13:21:11) ordex: anyway, I am fine with anything that can improve the process (obviously) (13:21:19) d12fk: devs tend to hack instead of rtfm, especially if the M is lengthy (13:21:30) ordex: moving it up the list and improving our config a bit might be good (13:21:38) dazo: +1 ... I'm guilty of that as well, d12fk (13:22:00) ordex: anyway - $partner calls for lunch (13:22:06) mattock: 8 minutes left (13:22:24) d12fk: are the $rules in git somewhere? (13:22:27) ordex: on my side I can look at uncrustify to make sure i agree with it more (13:22:28) ordex: :p (13:22:34) cron2: d12fk: yes, in-tree (13:22:42) dazo: ordex: that would be great! (13:22:52) cron2: ./dev-tools/uncrustify.conf (13:22:59) d12fk: simple to point there immediatly then (13:23:02) mattock: ah indeed, our Exchange is offline, probably due to the horrible exchange hack (13:23:21) plaisthos: mattock: mail.app works (13:23:26) plaisthos: only imap access is down it seems (13:23:36) mattock: yep (13:23:47) mattock: I guess I have to go back to 1990's to write summary (13:23:49) mattock: :) (13:24:02) d12fk: lz4-rebaser sound dangerous =) (13:24:42) dazo: d12fk: hehe ... but it actually does a decent job .... but we have been discussing kicking out the embedded lz4 support, though ... just never concluded (13:25:22) plaisthos: dazo: you said you will prepare a patch :P (13:25:31) cron2: we could do that for 2.6 - we do not very much like compression anyway anymore *and* platforms tend to have lz4 these days (13:25:33) dazo: did I? .... whooops (13:25:41) plaisthos: yeah (13:25:44) plaisthos: we discussed that (13:25:44) dazo: I'll do that now (13:25:56) plaisthos: update for 2.5 release and remove from master (13:25:58) cron2: it's quite a bit of configure.ac... (13:26:05) ***dazo probably does a bit too much in parallel (13:26:48) cron2: I get interrupts every 5 minutes "can you look at my Homework?" :-) (13:26:55) dazo: ahh ... I vaguely recall some configure.ac related discussions .... was it some tricky details there about pkg-config and such? (13:27:28) cron2: yep :-) "try to find it on the system, and if not, fall back to build our own" (13:28:16) dazo: cron2: can we expect pkg-config to be available on all platforms (minus non-mingw Windows)? (13:28:24) cron2: no (13:28:42) plaisthos: just keep the code asis :P (13:28:47) plaisthos: just remove the fallback to internal (13:29:15) cron2: it's an optional extra on at least some of the BSDs, but I can't give you a definite list... (and the system lz4 port would need to install a lz4.pc...) (13:29:27) cron2: this is all not very nasty across platforms (13:29:35) dazo: cron2: okay .... can we then have it so if pkg-config is not present, you need to pass ./configure args pointing at the LZ4 libs/headers to enable it? No "auto-detect" ... Is that acceptable? (13:29:50) cron2: "what plaisthos says" :) (13:31:25) cron2: I admit I have no idea what is there today... (13:31:40) dazo: My point is ... iirc today's functionality .... if pkg-config is not found, it fallsback to our compat-lz4. But what should be the behaviour if not found? Do we want to enforce --disable-lz4 (or --enable-lz4) and fail .... or should it be enabled/disabled based on what it finds (13:32:28) dazo: and then comes the finer variants .... if "enable/disable on what it finds" ... what if LZ4_LIBS is defined and not found, should it bail out with error or disable? (13:32:44) dazo: I want this to be simple and sane (13:34:04) dazo: So essentially .... should this line result in an error or disable LZ4? (13:34:05) dazo: AC_MSG_RESULT([ usable LZ4 library or header not found, using version in src/compat/compat-lz4.*]) (13:34:29) cron2: dazo: I'd say instead of fallbacking, error out (13:34:50) cron2: if someone does not want LZ4, --disable-lz4, or pass in LZ4_LIBS (13:34:56) cron2: well (13:34:57) dazo: And that will enforce users not having lz4-devel installed to add --disable-lz4 to ./configure (13:35:04) cron2: yes (13:35:15) dazo: okay, I can prepare a patch for that behaviour (13:35:41) cron2: this should be a conscious thing to enable or disable lz4, but not "it will be enabled or disabled depending on other bits of the system" (13:36:13) dazo: yeah ... but that's kinda what we had ... just in the context "you always get lz4 - system or ours" (13:36:49) cron2: well, we didn't have "--enable-lz4 leads to not having LZ4" :-) - this should be explicit (13:38:55) dazo: Yeah, true .... So we already have --disable-lzo and --disable-lz4 - so that is consistent. And we should probably consider switching that to --enable-* at some point too, and if not present ensure the STUB compression is available by default .... and by saying that, I believe I start to recall more of the discussions .... that it perhaps should be able to *accept compressed* incoming traffic, but *never send* compressed data .... (13:39:41) dazo: lets solve the STUB scenario another day, together with switching from --disable-* to --enable-* (13:39:46) cron2: yep (13:39:59) plaisthos: dazo: we already have that in (13:40:04) cron2: someone brave can fix quite a few more things in our configure.ac :) (13:40:23) dazo: cron2: shoot! I can put that on my plate (13:40:44) plaisthos: dazo: --allow-compression yes, asym, no (13:41:00) cron2: one thing that irked me since ever is "on a FreeBSD system, configure will not find lzo2 in the normal place, unless told with LZO_LIB=..." (13:41:19) plaisthos: same on mac os (13:41:35) cron2: because it's in /usr/local/{include,lib}/ which is "the normal place on FreeBSD for non-system things", but configure does not want to look there, "because that would not be The Right Thing To Do" (13:41:49) plaisthos: I usually end up with things like (13:41:49) plaisthos: ../openvpn-git/configure --with-crypto-library=mbedtls MBEDTLS_CFLAGS="-I$HOME/oss/mbedtls/include -I$HOME/oss/mbedtls/crypto/include" MBEDTLS_LIBS="-L$HOME/oss/mbedtls/library -lmbedtls -lmbedx509 -lmbedcrypto" (13:41:56) dazo: okay .... (13:42:21) plaisthos: but defaulting to pkg-config would probably be nice to find things %) (13:42:31) cron2: ah, yes, same thing for mbedtls builds - they are also in /usr/local/*/ on FreeBSD, so maybe there is a .pc now, etc. (13:42:32) dazo: cron2: does lzo have a pkg-config present on FreeBSD? And is pkg-config generally working fine on FreeBSD? (13:42:51) dazo: because our lzo section in configure.ac does not look for it (13:42:53) cron2: it can not be relied-upon because it's optional, and not all packages provide a .pc (13:43:20) cron2: lzo2 does (13:43:21) cron2: /usr/local/libdata/pkgconfig/lzo2.pc (13:43:30) dazo: well, that's not so different from Linux .... but the "optional" more commonly installed by default anyhow (13:43:50) dazo: so, lets try to add the pkg-config support and see if that can resolve it nicely (13:43:53) plaisthos: cron2: does pkg-config lzo2 --cflags work for you? (13:44:06) cron2: -I/usr/local/include/lzo (13:44:07) cron2: yes (13:44:07) ***d12fk is out of here again, later (13:44:17) cron2: well (13:44:18) cron2: no (13:44:22) cron2: *sigh* (13:44:49) cron2: (because we include <lzo/lzo1x.h> and not <lzo1x.h>...) (13:44:53) cron2: how annoying is that (13:45:06) dazo: huh!? (13:45:18) plaisthos: well kinda (13:45:18) plaisthos: #if defined(HAVE_LZO_LZO1X_H) (13:45:18) plaisthos: #include <lzo/lzo1x.h> (13:45:18) plaisthos: #elif defined(HAVE_LZO1X_H) (13:45:18) plaisthos: #include <lzo1x.h> (13:45:21) plaisthos: #endif (13:45:27) cron2: oh, interesting (13:45:33) dazo: 8-o (13:46:07) cron2: so it would work... --libs provides (13:46:10) cron2: -L/usr/local/lib -llzo2 (13:46:13) cron2: which is correct (13:46:47) plaisthos: Alon commited that with (13:46:48) plaisthos: build: proper lzo detection and usage (13:47:39) plaisthos: but the lzo/ with/without thing is older than his commit (13:47:45) dazo: okay, so ... I'll fix those to issues .... there are a few ticking autotools bombs lingering too ... LT_INIT (which we get complaints about on newer platforms) ... and the subdir-objects (which is a bit annoying, but will probably end up as the default behaviour some time in a future autotools update) (13:48:11) plaisthos: pkg-config lzo2 --cflags (13:48:12) plaisthos: -I/usr/local/Cellar/lzo/2.10/include/lzo (13:48:23) cron2: that sounds workable (13:48:25) plaisthos: I also get a path to a directory that contains the lzo headers directory (13:48:52) cron2: mbedtls does not have a .pc on FreeBSD :-( (13:49:09) plaisthos: wolfssl has one ;) (13:49:23) cron2: let's drop mbedtls and embrace wolfssl instead! (13:49:50) plaisthos: but neither has openssl (13:52:22) dazo: is it libssl.pc and libcrypto.pc perhaps? (13:52:44) plaisthos: dazo: have you seen OpenSSL's build system? :P (13:53:00) d12fk: "system" (13:53:04) dazo: No, I have more than enough to grok their C APIs :-P (13:53:24) plaisthos: the C APIs are okay in 1.1.1 (13:53:38) d12fk: plaisthos has stockholm syndrome (13:53:38) plaisthos: dazo: pkg-config --list-all does not have anything openssl related for me (13:54:30) plaisthos: d12fk: no honestely they are okay in 1.1.1 compared to 1.0.2 (13:54:34) mattock: mmm (13:54:39) mattock: I will send out the summary, you go on :) (13:54:51) dazo: alright ... so ... pkg-config on *BSD is kinda a half-hearted effort (13:54:57) d12fk: plaisthos: just kidding, have not seen them
_______________________________________________ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
