debian/rules questions and my plans for dual-build.

[Sven Mueller]

Henrique de Moraes Holschuh wrote on 06/05/2006 17:09:
>>-dnl Select a method for IMAP IDLE
>>-dnl
>>-AC_ARG_WITH(idle,[  --with-idle=METHOD      use METHOD for IMAP IDLE
>>-                          METHOD is [poll], idled or no],
>>-	WITH_IDLE="${withval}",WITH_IDLE="poll")
>>-AC_SUBST(WITH_IDLE)
>>-if test "$WITH_IDLE" = "idled"; then
>>-  IMAP_PROGS="$IMAP_PROGS idled"
>>-fi
>>-
>>-dnl

> You don't want to do that, it makes submiting the patch upstream more
> difficult.

I don't really get why you should be able to change the default here at
build time. Or why it would make the patch harder to get into upstream
if the --with-idle configure-option is removed. But if you want to keep
a configure option to change the default, I would really suggest
changing the option name to --with-idle-default, do clearly distinguish
it from what the old option did.

> Instead, you want to keep WITH_IDLE, and make it select the *default* idle
> backend.  The manpages should be generated reflecting whatever default the
> user coose using --with-idle, as well.

Hmm, I have actually no idea how to achieve that, I probably really
should learn how to use autoconf/automake some time.

> The default selection should be the current upstream default.

Which is "poll".

>>@@ -2205,13 +2207,13 @@
>> 
>>     /* Start doing mailbox updates */
>>     if (imapd_mailbox) index_check(imapd_mailbox, 0, 1);
>>-    idle_start(imapd_mailbox);
>>+    IDLE->start(imapd_mailbox);
>> 
>>     /* Get continuation data */
>>     c = getword(imapd_in, &arg);
>> 
>>     /* Stop updates and do any necessary cleanup */
>>-    idle_done(imapd_mailbox);
>>+    IDLE->done(imapd_mailbox);
> 
> 
> I'd rather you did it a bit differently:
> 
> make idle_enabled(), idle_start(), idle_done(), etc macros (or keep
> them as functions, but hint gcc that they probably should be inlined) that
> does, e.g. the IDLE->enabled() test if IDLE is non-null.  This minimizes the
> changes to the rest of the code, and it is cleaner too (and faster if it is
> just a macro that does ( if (config_idle != NULL) config_idle->enabled(); )
> or something like that.

Sorry, but how to you switch between different _inline_ functions at
runtime? Those are functions which come directly from the different
idle-backends, as usual when you dynamically want to switch to the
different implementations, this is done via function pointers.

Though doing the (config_idle != NULL) test might still be better than
the null-Backend. But I leave that to Ondrej or any other ontakers. I
spent a few hours today getting Ondrej's Patch to actually compile _and_
work (see /branches/idled). I also incorporated the easier tasks you
outlined, including the test on config_idle vs. NULL. I didn't remove
the idle_no backend though yet. I'm realizing just how tired I am at the
moment (have been up since 30 hours or so now), so I would rather not
mess it all up now that it is currently working.

> You wouldn't need an idle_backend "No", then.  You could just use NULL for
> that.  Otherwise, make sure to bomb if config_idle gets set as NULL by a
> mishap in global.c.

See above, tests implemented, idle_no not yet removed. I chose not to
bomb out, but rather to interpret config_idle as if the "no" backend was
chosen.

> Why the IDLE define, btw?

I wondered the same thing and removed it. It's pretty easy to replace
all idle_config occurences with IDLE again if we would ever need that.

>>-    if (idle_method_desc)
>>-	snprintf(env_buf + strlen(env_buf), MAXIDVALUELEN - strlen(env_buf),
>>-		 "; idle = %s", idle_method_desc);
>>+    snprintf(env_buf + strlen(env_buf), MAXIDVALUELEN - strlen(env_buf),
>>+             "; idle = runtime");
> 
> This hunk should print the current active idle backend, not just "runtime".

Fixed.

Regards,
Sven

PS: Thanks Ondrej for the initial patch, I wouldn't have been able to
implement it that fast.