Load plugins based on VDRVERSION and APIVERSION alternatively

Message ID 44491FE4.1030909@gmx.de
State New
Headers

Commit Message

Udo Richter April 21, 2006, 6:09 p.m. UTC
  Hi Klaus,


The new APIVERSION of 1.3.47 will cause lots of trouble as soon as 
APIVERSION and VDRVERSION differ the first time, as this requires 
updated Makefiles for all plugins. In best case, VDR wont find a plugin 
because it has the wrong name, in worst case, an old version is used 
without noticing.

On the other hand, its not too difficult to load plugins based on 
APIVERSION and VDRVERSION alternatively. That way, plugins that still 
use VDRVERSION continue to work. This even gives plugin authors the 
ability to ignore APIVERSION compatibility if the plugin depends on 
obscure differences between versions. The only drawback is that plugin 
authors will be lazy adapting to APIVERSION.

The attached patch does the trick: It loads plugins using APIVERSION and 
VDRVERSION, and gives VDRVERSION the precedence. The patch also prefers 
VDRVERSION when loading "*". Feel free to use it if you want.

Cheers,

Udo
  

Comments

Klaus Schmidinger April 22, 2006, 10:27 a.m. UTC | #1
Udo Richter wrote:
> Hi Klaus,
> 
> 
> The new APIVERSION of 1.3.47 will cause lots of trouble as soon as 
> APIVERSION and VDRVERSION differ the first time, as this requires 
> updated Makefiles for all plugins. In best case, VDR wont find a plugin 
> because it has the wrong name, in worst case, an old version is used 
> without noticing.
> 
> On the other hand, its not too difficult to load plugins based on 
> APIVERSION and VDRVERSION alternatively. That way, plugins that still 
> use VDRVERSION continue to work. This even gives plugin authors the 
> ability to ignore APIVERSION compatibility if the plugin depends on 
> obscure differences between versions. The only drawback is that plugin 
> authors will be lazy adapting to APIVERSION.
> 
> The attached patch does the trick: It loads plugins using APIVERSION and 
> VDRVERSION, and gives VDRVERSION the precedence. The patch also prefers 
> VDRVERSION when loading "*". Feel free to use it if you want.

I'm afraid I don't like this.

APIVERSION was introduced because several people complained that
they had to recompile all plugins every time there was even the slightest
change in VDR. As long as none of VDR's header files has been changed (in a way
that would cause incompatibilities) a newer version of VDR (which, for
instance, fixes some bugs and has only changes in *.c files) should be
able to use existing compiled versions of plugins. If we start using either
VDRVERSION or APIVERSION, there will never be a clear way of handling this.

Originally I wanted to have a clear 1:1 mapping between a VDR version
and its plugins. However, I do see that it would be good to avoid
unnecessary recompilation. If APIVERSION isn't the right method to
achieve this, I'd rather remove it from the final version 1.4.

Klaus

> ------------------------------------------------------------------------
> 
> --- vdr-1.3.47-orig/plugin.c	2006-04-21 19:08:02.000000000 +0200
> +++ vdr-1.3.47/plugin.c	2006-04-21 19:53:32.709327912 +0200
> @@ -294,11 +294,26 @@
>                if (p) {
>                   *p = 0;
>                   p += strlen(SO_INDICATOR);
> -                 if (strcmp(p, APIVERSION) == 0) {
> +                 if (strcmp(p, VDRVERSION) == 0) {
> +                    // VDRVERSION is always loaded
>                      char *name = e->d_name + strlen(LIBVDR_PREFIX);
>                      if (strcmp(name, "*") != 0) { // let's not get into a loop!
> -                       AddPlugin(e->d_name + strlen(LIBVDR_PREFIX));
> +                       AddPlugin(name);
> +                       }
> +                    }
> +                 if (strcmp(p, APIVERSION) == 0) {
> +                    // APIVERSION found, check whether VDRVERSION exists too
> +                    char *name = e->d_name + strlen(LIBVDR_PREFIX);
> +                    char *buffer = NULL;
> +                    asprintf(&buffer, "%s/%s%s%s%s", directory, LIBVDR_PREFIX, name, SO_INDICATOR, VDRVERSION);
> +                    if (access(buffer,F_OK) != 0) {
> +                       // APIVERSION is the only existing one, use it
> +                       if (strcmp(name, "*") != 0) { // let's not get into a loop!
> +                          AddPlugin(name);
> +                          }
>                         }
> +                    // else: skip APIVERSION, the loop will load VDRVERSION instead
> +                    free(buffer);
>                      }
>                   }
>                }
> @@ -310,7 +325,11 @@
>    if (p)
>       *p = 0;
>    char *buffer = NULL;
> -  asprintf(&buffer, "%s/%s%s%s%s", directory, LIBVDR_PREFIX, s, SO_INDICATOR, APIVERSION);
> +  asprintf(&buffer, "%s/%s%s%s%s", directory, LIBVDR_PREFIX, s, SO_INDICATOR, VDRVERSION);
> +  if (access(buffer,F_OK) != 0) { // Plugin not found using VDRVERSION, try APIVERSION instead
> +     free(buffer);
> +     asprintf(&buffer, "%s/%s%s%s%s", directory, LIBVDR_PREFIX, s, SO_INDICATOR, APIVERSION);
> +     }
>    dlls.Add(new cDll(buffer, Args));
>    free(buffer);
>    free(s);
  
Ville Skyttä April 22, 2006, 10:42 a.m. UTC | #2
On Sat, 2006-04-22 at 12:27 +0200, Klaus Schmidinger wrote:

> If APIVERSION isn't the right method to
> achieve this, I'd rather remove it from the final version 1.4.

Please don't, that's a very welcome feature.

Plugins that have not been changed to use APIVERSION yet upstream can be
usually locally updated with something as simple as:
$ sed -i -e s/VDRVERSION/APIVERSION/g Makefile

...and somewhat off topic, the DVBDIR changes can be done with:
$ sed -i -e '/^DVBDIR/d' -e 's|-I$(DVBDIR)/include||' Makefile
  
Clemens Kirchgatterer April 22, 2006, 10:51 a.m. UTC | #3
Klaus Schmidinger <Klaus.Schmidinger@cadsoft.de> wrote:

> APIVERSION was introduced because several people complained that
> they had to recompile all plugins every time there was even the
> slightest change in VDR. As long as none of VDR's header files has
> been changed (in a way that would cause incompatibilities) a newer
> version of VDR (which, for instance, fixes some bugs and has only
> changes in *.c files) should be able to use existing compiled
> versions of plugins. If we start using either VDRVERSION or
> APIVERSION, there will never be a clear way of handling this.
> 
> Originally I wanted to have a clear 1:1 mapping between a VDR version
> and its plugins. However, I do see that it would be good to avoid
> unnecessary recompilation. If APIVERSION isn't the right method to
> achieve this, I'd rather remove it from the final version 1.4.

i for myself always use the same version numbering scheme to distinct
between incompatible APIs in my own projects.

X.Y.Z

a raise in Z marks a compatible change in both, source code and
binary. (module can be reused without recompilation)

a changed Y marks source code compatible but not binary compatible
changes. (module has to be recompiled but needs not to be altered)

a bumped X means the meaning of the API changed in a source and binary
incompatible way. (module needs adoption and recompilation)

this scheme has the benefit of being very precise, but the drawback,
of being not feasable in very young/fast moving projects with lots of
API changes as the X number would go through the roof. but once you can
call the API rather stable, it works very well (at least for me) ;-)

best regards ...
clemens
  
Udo Richter April 22, 2006, 2:40 p.m. UTC | #4
Klaus Schmidinger wrote:
> I'm afraid I don't like this.
> 
> APIVERSION was introduced because several people complained that
> they had to recompile all plugins every time there was even the slightest
> change in VDR. As long as none of VDR's header files has been changed 
> (in a way that would cause incompatibilities) a newer version of VDR (which, for
> instance, fixes some bugs and has only changes in *.c files) should be
> able to use existing compiled versions of plugins. If we start using either
> VDRVERSION or APIVERSION, there will never be a clear way of handling this.

I understand, and I also see this as the major drawback of the patch. I 
just wanted to make the suggestion before this gets hot.

We'll see how much chaos will arise when VDRVERSION and APIVERSION 
differ the first time. Hopefully, most plugins will soon be updated.

Cheers,

Udo
  
Wolfgang Rohdewald April 23, 2006, 9:58 a.m. UTC | #5
On Saturday 22 April 2006 12:42, Ville Skyttä wrote:
> Plugins that have not been changed to use APIVERSION yet upstream can be
> usually locally updated with something as simple as:
> $ sed -i -e s/VDRVERSION/APIVERSION/g Makefile
> 
> ...and somewhat off topic, the DVBDIR changes can be done with:
> $ sed -i -e '/^DVBDIR/d' -e 's|-I$(DVBDIR)/include||' Makefile

can plugins with those changes still be compiled with older vdr
versions? Otherwise all plugins need a separate Makefile.VDRVERSION,
or is there a better solution?
  
Klaus Schmidinger April 23, 2006, 11:32 a.m. UTC | #6
Wolfgang Rohdewald wrote:
> On Saturday 22 April 2006 12:42, Ville Skyttä wrote:
> 
>>Plugins that have not been changed to use APIVERSION yet upstream can be
>>usually locally updated with something as simple as:
>>$ sed -i -e s/VDRVERSION/APIVERSION/g Makefile
>>
>>...and somewhat off topic, the DVBDIR changes can be done with:
>>$ sed -i -e '/^DVBDIR/d' -e 's|-I$(DVBDIR)/include||' Makefile
> 
> 
> can plugins with those changes still be compiled with older vdr
> versions? Otherwise all plugins need a separate Makefile.VDRVERSION,
> or is there a better solution?

Maybe this is what you're looking for:

http://linuxtv.org/pipermail/vdr/2006-April/008884.html

Klaus
  
Udo Richter April 23, 2006, 1:12 p.m. UTC | #7
Wolfgang Rohdewald wrote:
>> ...and somewhat off topic, the DVBDIR changes can be done with:
>> $ sed -i -e '/^DVBDIR/d' -e 's|-I$(DVBDIR)/include||' Makefile
> 
> can plugins with those changes still be compiled with older vdr
> versions? Otherwise all plugins need a separate Makefile.VDRVERSION,
> or is there a better solution?

They may fail on old 2.4 platforms that don't have DVB headers in the 
standard include paths. Platforms with 2.6 kernel headers should compile 
fine even on pre-1.3.47 VDR's.

I've added a hint to my readme's that you may have to add INCLUDES += 
-I/path/to/DVB/driver/include to the VDR Make.config or (in my case) to 
the plugin's Make.config.

Cheers,

Udo
  
Wolfgang Rohdewald April 23, 2006, 1:33 p.m. UTC | #8
On Sunday 23 April 2006 13:32, Klaus Schmidinger wrote:
> Maybe this is what you're looking for:
> 
> http://linuxtv.org/pipermail/vdr/2006-April/008884.html

yes, thank you.
  

Patch

--- vdr-1.3.47-orig/plugin.c	2006-04-21 19:08:02.000000000 +0200
+++ vdr-1.3.47/plugin.c	2006-04-21 19:53:32.709327912 +0200
@@ -294,11 +294,26 @@ 
               if (p) {
                  *p = 0;
                  p += strlen(SO_INDICATOR);
-                 if (strcmp(p, APIVERSION) == 0) {
+                 if (strcmp(p, VDRVERSION) == 0) {
+                    // VDRVERSION is always loaded
                     char *name = e->d_name + strlen(LIBVDR_PREFIX);
                     if (strcmp(name, "*") != 0) { // let's not get into a loop!
-                       AddPlugin(e->d_name + strlen(LIBVDR_PREFIX));
+                       AddPlugin(name);
+                       }
+                    }
+                 if (strcmp(p, APIVERSION) == 0) {
+                    // APIVERSION found, check whether VDRVERSION exists too
+                    char *name = e->d_name + strlen(LIBVDR_PREFIX);
+                    char *buffer = NULL;
+                    asprintf(&buffer, "%s/%s%s%s%s", directory, LIBVDR_PREFIX, name, SO_INDICATOR, VDRVERSION);
+                    if (access(buffer,F_OK) != 0) {
+                       // APIVERSION is the only existing one, use it
+                       if (strcmp(name, "*") != 0) { // let's not get into a loop!
+                          AddPlugin(name);
+                          }
                        }
+                    // else: skip APIVERSION, the loop will load VDRVERSION instead
+                    free(buffer);
                     }
                  }
               }
@@ -310,7 +325,11 @@ 
   if (p)
      *p = 0;
   char *buffer = NULL;
-  asprintf(&buffer, "%s/%s%s%s%s", directory, LIBVDR_PREFIX, s, SO_INDICATOR, APIVERSION);
+  asprintf(&buffer, "%s/%s%s%s%s", directory, LIBVDR_PREFIX, s, SO_INDICATOR, VDRVERSION);
+  if (access(buffer,F_OK) != 0) { // Plugin not found using VDRVERSION, try APIVERSION instead
+     free(buffer);
+     asprintf(&buffer, "%s/%s%s%s%s", directory, LIBVDR_PREFIX, s, SO_INDICATOR, APIVERSION);
+     }
   dlls.Add(new cDll(buffer, Args));
   free(buffer);
   free(s);