+1

@t-j-h also see #447

At this point, I can completely replace the patch that EDK2 has against 1.0.2, with backports of commits which are already in OpenSSL HEAD.

The one commit I need for the current 1.0.2-based build (as opposed to new stuff to make it possible to switch to OpenSSL 1.1) is commit 2f37773 from this PR. (And a corresponding fix for RSA_NET support, which no longer exists in 1.1).

Consider making an other pull request for the 1.0.2 branch with the commit you want there.

I agree with kurt: make a separate 1.0.2 PR with that commit (looks like a build bugfix to me).

My comment wasn't so much about backporting to 1.0.2. I have a pile of those, if you want them: http://git.infradead.org/users/dwmw2/openssl.git/shortlog/refs/heads/OpenSSL_1_0_2-stable

I don't think you do. I was actually assuming we'd get everything we need for UEFI into the 1.1 release and then switch to that and forget 1.0.2 completely.

My milestone in the meantime, however, is to turn our entire patch-against-1.0.2 into purely backports/cherrypicks of stuff that's already in HEAD. And of the handful of things I have outstanding in pull requests right now, that one commit is the only one that's also relevant to the 1.0.;2 tree and thus standing in the way of that milestone.

Got it. Split that out into a separate MR.

OK, I've split that one out into #755 to stand alone. Thanks.

+1

done.

commit 963bb62. is broken so re-opening this.

Crap, sorry about that. My original version of this commit did get that right; it must have been lost in a subsequent rebase, probably when I rebased around commit 8731a4f.

I treated this as a learning opportunity for the EDK2 folks, who have only just switched to git.
OpenSSL 1.1 status, and a worked example of why you should NEVER rebase

@dwmw2, an interesting article but I disagree with your conclusion! Merges present there own set of problems and add all sorts of complexity to the git history. Figuring out how a problem arrived in the source when it was introduced via a bad merge is a bit of a nightmare. Reading the git history is also unnecessarily confusing. OpenSSL does not accept merge commits at all - so actually you must rebase. If the original author doesn't do it, then the OpenSSL dev will... and they are more likely to screw it up than the author (because they didn't write the code in the first place).

@mattcaswell, I completely agree that figuring out how a problem arrived when it was introduced in a merge is a nightmare. But if you do that same merge as a rebase instead, then the same problem becomes outright impossible because you've thrown away the important information.

The problem happens when two people are doing things at the same time, which end up conflicting with each other. You don't remove that complexity by rebasing. You only make the situation worse.

And really, a history with merges isn't hard to understand as long as you are thinking of history as a graph, not a linear progression. I appreciate that's a hard conceptual leap to take when moving from a legacy version control system to a distributed one, but it's a leap that's worth taking.
But still, that rant was targeted at the EDK2 list. It does have relevance here too, I suppose, but that wasn't my main aim.

And yes, it does look like my RT4310 branch as pushed on February 27th was actually correct, and it was only broken in cherry-picking it onto HEAD. So it wasn't my fault at all :)

I'm not sure I agree that it's more likely that a rebase by the OpenSSL team will screw things up, than a rebase that I do myself. The point here is that two streams of changes have conflicted, and each of us is only intimately familiar with one side of the conflict. Which is why I was perfectly prepared to blame myself for this one.

In reality, whoever does the rebase, it's only ever going to get a quick smoke test, and never the full in-depth analysis that went into the original commit.

It is an unavoidable fact, that rebasing introduces errors, and at the same time it irrevocably erases the history which could allow them to be found.

I'd like to note again, that that message was directed at the EDK2 list — although it does have technical relevance here, it is not really my place to direct a rant like that at OpenSSL. While Intel employs me as an open source person, and prodding Intel projects in the right direction is actually part of my day job :)

With that out of the way, I would like to understand the OpenSSL setup better, so that I can improve my submissions. Having my commits reverted because they broke the build doesn't really help anyone.

So... what should I have done here?

Should I have done a git pull --rebase every day until the PR actually got merged? And painstakingly gone through all the corner cases that I did fully consider when actually producing those patches in the first place, doing various combinations of no-xxx builds under Linux and even in a Windows VM and with tweaked -fno-function-sections builds to test various things? Every day?

No...a periodic rebase is usually sufficient. Often that might be at the request of one of the devs. Sometimes though bad stuff just happens :-( ...hopefully when things calm down a bit the travis/appveyor build results will settle down and be a bit more helpful. Things are too volatile at the moment though.

What Matt said. I wouldn't worry about it. It's a feeding frenzy right now, and sometimes the wrong fish gets nipped. You're good.

Keeping it up to date daily in the hope it can still make beta1. And hoping not to disprove your assertion about me being less likely to screw it up :)

so which commits? all of them? merge this whole branch? sorry, being dense.

There's only one left. It all got merged, one commit was broken in the rebase, and I'm resubmitting it as (currently) commit 4a4fecd

I should shut up, but... concerning merge vs rebase, I've happily lived with both. They each have pros and cons, and I agree that history gets lost with rebasing and can become a mess if you're not careful, but the same goes with merges, they can become an impossible spaghetti to follow.

So I'd say that technically as well as philosophically, it comes down to a choice and what a team feels most comfortable with. Obviously, the OpenSSL team prefers a linear history, and it becomes a histry of application rather than a chronological one. Sounds like the EDK2 list would possibly go the other way, with a higher appreciation for parallellism, chronology, and possibly spaghetti ;-)

Me, I'm neutral on the matter. Both methods work, with their own quirks.

That remaining patch makes perfect sense to me

+1

Mostly, the only thing that ever needs to follow merge spaghetti is 'git bisect'. And it's quite good at it :)
Sure, when your bisect lands on a merge commit, you get a horrible sinking feeling. But then it's only one specific merge you're looking at, not spaghetti. And this is precisely the case where you are saved by the branch-and-merge strategy. Because if the code had been rebased, you'd never have had a working vs. non-working version to compare and see what went wrong.

But yeah, I should shut up too. I'm just not very good at it :)

@levitte, will you do this one patch to e_os.h (test-build) and then close this PR? Else I will do it in a couple of hours. +1 from me.

@dwmw2, sure, bisect is good, and looking at things with tools like gitk has your hair turn grey in an instant ;-) (gitk is particularly bad at displaying merge spaghetti, it makes it even worse)

Shall we both shut up now? I'm getting busy merging (no, sorry, rebasing) your patch into master.

Ok, hold on here... I don't mind this fix and am applying it no matter what as it does make sense, but just saying there the no-sock work seems incomplete. I just had a test build, and it went BAMF on apps/s_server.c, there seems to be a lot not very well guarded there. Quite possibly, s_client and s_server shouldn't even be built with no-sock...

libcrypto builds cleanly on both Linux and Windows with no-sock, though, that's enough for me to go ahead and merge this into master.

... and I spoke too fast. Got this on Windows:

        cl -DDSO_WIN32 -DOPENSSL_THREADS -DOPENSSL_NO_STATIC_ENGINE -DOPENSSL_PI
C -DOPENSSL_BN_ASM_PART_WORDS -DOPENSSL_IA32_SSE2 -DOPENSSL_BN_ASM_MONT -DOPENSSL_BN_ASM_GF2m -DSHA1_ASM -DSHA256_ASM -DSHA512_ASM -DMD5_ASM -DRMD160_ASM -DAES_ASM -DVPAES_ASM -DWHIRLPOOL_ASM -DGHASH_ASM -DECP_NISTZ256_ASM -DPOLY1305_ASM "-DENGINESDIR=\"\\usr\\local\\lib\\engines\"" "-DOPENSSLDIR=\"\\usr\\local\\ssl\"" -W3 -wd4090 -Gs0 -GF -Gy -nologo -DOPENSSL_SYS_WIN32 -DWIN32_LEAN_AND_MEAN -DL_ENDIAN -D_CRT_SECURE_NO_DEPRECATE -DUNICODE -D_UNICODE /MT /Ox /O2 /Ob2 /Zl /Zi /Fdlib -D_WINDLL /I include /I ..\master /I ..\master\include -c /Fossl\bio_ssl.obj @C:\Users\Richard\AppData\Local\Temp\nm5FC5.tmp
bio_ssl.c
w:\gitwrk\openssl.net\official\master\ssl\ssl_locl.h(1501) : error C2079: 'next_timeout' uses undefined struct 'timeval'
NMAKE : fatal error U1077: '"c:\Program Files\Microsoft Visual Studio 9.0\VC\BIN\cl.EXE"' : return code '0x2'
Stop.

That was with this configuration:

perl Configure VC-WIN32 no-sock

So at this point, I'm temporarly taking back my +1. This needs a bit more work.

Yeah, apps don't build with no-sock. I've fixed things up so that the libraries were building for Linux and EDK2 at least. Does the no-sock build work on Windows even before this patch? It shouldn't make any difference to a DOS/Windows build, or to any other build unless OPENSSL_NO_SOCK is set. It's only supposed to affect the OPENSSL_NO_SOCK, non-Windows build.

The answer can be found here

Apparently, MicroSoft thought that <winsock2.h> would be the perfect place to define struct timeval, because of course, it's only used with select(), right? I cannot tell you how upset I get over this kind of thing.

So you know, my +1 comes back, dealing with VC things like this is a different deal and another PR.

Merged. ffbc5b5