Re: [PATCH] imap-send: replace auto-probe libcurl with hard dependency

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Feb 2, 2023, 00:20 UTC
Message-ID: <230202.86v8kkq53u.gmgdl@evledraar.gmail.com>
In-Reply-To: <Y9r84bezJ0scapwC@coredump.intra.peff.net>

On Wed, Feb 01 2023, Jeff King wrote:

Show 21 quoted lines

> On Wed, Feb 01, 2023 at 03:22:24PM -0800, Junio C Hamano wrote:
>
>> > Let's also hide the old --curl and --no-curl options, and die if
>> > "--no-curl" is provided.
>> 
>> In other words, if we are building imap-send, we sure know cURL is
>> there, and there is no need to tell a running imap-send not to use
>> cURL to talk to the IMAP service?  I am not sure the linkage of this
>> change with the rest of the patch.  Isn't that a totally orthogonal
>> issue?  Your imap-send might be cURL enabled, but unless we stop to
>> ship with our own IMAP routines compiled into imap-send, --no-curl
>> does have a purpose.
>> 
>> Or did you just forget to document that we stop to ship with our own
>> IMAP routines in the above?  If so, as long as it is made a bit more
>> prominent in the proposed log message in a reroll, I would be happy
>> with such a change rolled into the same patch.
>
> FWIW, I had the same urge as Ævar, to drop the non-curl support
> completely, and was puzzled that his patch did not have a big code
> deletion. ;)

FWIW I arrived at this from looking at the mandatory $(shell)-outs in the Makefile, and wasn't looking to drop the OpenSSL code.

Then in looking at that, I found that we could probably make the curl dependency mandatory.

Show 16 quoted lines

> The problem is that the tunnel mode still relies on the non-curl code.
> There was a series to address that a while ago:
>
>   https://lore.kernel.org/git/ab866314-608b-eaca-b335-12cffe165526@morey-chaisemartin.com/
>
> but it ran into the problem that curl did not support PREAUTH
> connections (which is one of the main points of tunneling). It looks
> like that got added to curl via their befaa7b14f, which is in curl
> 7.56.0 from 2017. That's not old enough for us to require for http, but
> might be OK for a marginal component like the tunneling mode of
> imap-send.
>
> I think there was also some question of how you even get the tunnel
> going. Curl really wants to have a single socket descriptor, not two
> pipe descriptors, so there may have to be some trickery with
> socketpair(). There's more discussion in the linked thread.

That's neat, I didn't know about that attempt.

Show 5 quoted lines

> So I think there's a path forward here for getting rid of the legacy
> code (and I'd be really happy to see it gone; it's imported code that
> does not seem super well maintained by us). But until we do that,
> disabling --no-curl doesn't seem like that big a win, if that code can
> all still be triggered for tunnel mode.

I think the biggest win is that we're dropping the dual curl/OpenSSL codepath for everything except the "tunnel" mode, which is really obscure compared to the already-obscure functionality of the main "imap-send" tool.

It would also get us part of the way to e.g. depending on 7.56.0, as a hard dependency on curl (and a newer version than we usually depend on) would reveal if anyone's got an issue with that stepping stone.