Re: pkg/45047: pkgtools/pkg_install minix support

[Joerg Sonnenberger <joerg%britannica.bec.de@localhost>]

The following reply was made to PR pkg/45047; it has been noted by GNATS.

From: Joerg Sonnenberger <joerg%britannica.bec.de@localhost>
To: gnats-bugs%NetBSD.org@localhost
Cc: pkg-manager%netbsd.org@localhost, gnats-admin%netbsd.org@localhost, 
pkgsrc-bugs%netbsd.org@localhost
Subject: Re: pkg/45047: pkgtools/pkg_install minix support
Date: Sat, 11 Jun 2011 19:57:58 +0200

 On Fri, Jun 10, 2011 at 03:55:00PM +0000, tcort%minix3.org@localhost wrote:
 > diff --git a/pkgtools/pkg_install/Makefile b/pkgtools/pkg_install/Makefile
 > index 392093c..b262265 100644
 > --- a/pkgtools/pkg_install/Makefile
 > +++ b/pkgtools/pkg_install/Makefile
 > @@ -142,6 +142,10 @@ LIBS+=          -larchive
 >  CPPFLAGS+=  -I${WRKDIR}/libfetch
 >  LDFLAGS+=   -L${WRKDIR}/libfetch
 >  
 > +.  if ${OPSYS} == "Minix"
 > +LIBS+=              -larchive -lbz2 -lz
 > +.  endif
 > +
 >  CONFIGURE_ENV+=     LIBS=${LIBS:Q}
 >  
 >  do-extract:
 
 This is wrong, it shouldn't be needed.
 
 > diff --git a/pkgtools/pkg_install/files/add/perform.c 
 > b/pkgtools/pkg_install/files/add/perform.c
 > index aa3dff3..cf7a5c5 100644
 > --- a/pkgtools/pkg_install/files/add/perform.c
 > +++ b/pkgtools/pkg_install/files/add/perform.c
 > @@ -52,6 +52,7 @@ __RCSID("$NetBSD: perform.c,v 1.98 2010/09/14 22:26:18 gdt 
 > Exp $");
 >  #include <stdlib.h>
 >  #include <string.h>
 >  #include <unistd.h>
 > +#include <assert.h>
 >  
 >  #include <archive.h>
 >  #include <archive_entry.h>
 > @@ -126,7 +127,12 @@ static const struct pkg_meta_desc {
 >      { 0, NULL, 0, 0 },
 >  };
 >  
 > -static int pkg_do(const char *, int, int);
 > +struct dependency_stack {
 > +    struct dependency_stack *prev;
 > +    const char *pkgpath;
 > +};
 > +
 > +static int pkg_do(const char *, int, int, struct dependency_stack *);
 >  
 >  static int
 >  end_of_version(const char *opsys, const char *version_end)
 > @@ -738,7 +744,7 @@ extract_files(struct pkg_task *pkg)
 >                      continue;
 >  
 >              case PLIST_CMD:
 > -                    if (format_cmd(cmd, sizeof(cmd), p->name, pkg->prefix, 
 > last_file))
 > +                    if (format_cmd(cmd, sizeof(cmd), p->name, 
 > pkg->install_prefix, last_file))
 >                              return -1;
 >                      printf("Executing '%s'\n", cmd);
 >                      if (!Fake && system(cmd))
 
 What are you trying to do here?
 
 > @@ -858,6 +864,35 @@ pkg_register_depends(struct pkg_task *pkg)
 >      free(text);
 >  }
 >  
 > +#ifdef __minix
 > +static void
 > +normalise_version(char *release, char *version)
 > +{
 > +    char actual_version[50];
 > +    int l1, l2;
 > +
 > +    assert(release && version);
 > +
 > +    l1 = strlen(release);
 > +    l2 = strlen(version);
 > +
 > +    assert(l1 + l2 + 2 < sizeof(actual_version));
 > +
 > +    if(l1 > 0 && l2 > 0)
 > +            snprintf(actual_version, sizeof(actual_version),
 > +                    "%s.%s", release, version);
 > +        else if(strlen(release) > 0)
 > +            strncpy(actual_version, release, sizeof(actual_version)-1);
 > +    else if(strlen(version) > 0)
 > +            strncpy(actual_version, version, sizeof(actual_version)-1);
 > +    else
 > +            errx(EXIT_FAILURE, "no version info");
 > +
 > +    strcpy(release, actual_version);
 > +    version[0] = '\0';
 > +}
 > +#endif
 > +
 >  /*
 >   * Reduce the result from uname(3) to a canonical form.
 >   */
 > @@ -870,6 +905,9 @@ normalise_platform(struct utsname *host_name)
 >      span = strspn(host_name->release, "0123456789.");
 >      host_name->release[span] = '\0';
 >  #endif
 > +#ifdef __minix
 > +    normalise_version(host_name->release, host_name->version);
 > +#endif
 >  }
 >  
 >  /*
 > @@ -880,7 +918,7 @@ check_platform(struct pkg_task *pkg)
 >  {
 >      struct utsname host_uname;
 >      const char *effective_arch;
 > -    int fatal;
 > +    int fatal = 0;
 >  
 >      if (uname(&host_uname) < 0) {
 >              if (Force) {
 > @@ -906,6 +944,10 @@ check_platform(struct pkg_task *pkg)
 >      else
 >              fatal = 0;
 >  
 > +#ifdef __minix
 > +    normalise_version(host_uname.release, host_uname.version);
 > +#endif
 > +
 >      if (fatal ||
 >          compatible_platform(OPSYS_NAME, host_uname.release,
 >                              pkg->buildinfo[BI_OS_VERSION]) != 1) {
 > @@ -1093,7 +1135,7 @@ check_implicit_conflict(struct pkg_task *pkg)
 >  }
 >  
 >  static int
 > -check_dependencies(struct pkg_task *pkg)
 > +check_dependencies(struct pkg_task *pkg, struct dependency_stack 
 > *dependency_stack)
 >  {
 >      plist_t *p;
 >      char *best_installed;
 > @@ -1124,7 +1166,7 @@ check_dependencies(struct pkg_task *pkg)
 >                                  p->name);
 >                              continue;
 >                      }
 > -                    if (pkg_do(p->name, 1, 0)) {
 > +                    if (pkg_do(p->name, 1, 0, dependency_stack)) {
 >                              if (ForceDepends) {
 >                                      warnx("Can't install dependency %s, "
 >                                          "continuing", p->name);
 > @@ -1373,12 +1415,33 @@ check_license(struct pkg_task *pkg)
 >   * Install a single package.
 >   */
 >  static int
 > -pkg_do(const char *pkgpath, int mark_automatic, int top_level)
 > +pkg_do(const char *pkgpath, int mark_automatic, int top_level, 
 > +    struct dependency_stack *dependency_stack)
 >  {
 >      char *archive_name;
 >      int status, invalid_sig;
 >      struct pkg_task *pkg;
 >  
 > +    /* workaround 2010-12-10: prevent endless recursion for circular 
 > dependencies */
 > +    struct dependency_stack dependency_stack_top;
 
 Please don't. If you want to catch circular dependencies, just use a
 counter. Bigger question of course is, why you ended up here in first
 place...
 
 > +
 > +    dependency_stack_top.prev = dependency_stack;
 > +    dependency_stack_top.pkgpath = pkgpath;
 > +
 > +    while (dependency_stack) {
 > +            if (strcmp(dependency_stack->pkgpath, pkgpath) == 0) {
 > +                    fprintf(stderr, "warning: ignoring circular 
 > dependency:\n");
 > +                    dependency_stack = &dependency_stack_top;
 > +                    while (dependency_stack) {
 > +                            fprintf(stderr, "- %s\n", 
 > dependency_stack->pkgpath);
 > +                            dependency_stack = dependency_stack->prev;
 > +                    }
 > +                    return 0;
 > +            }
 > +            dependency_stack = dependency_stack->prev;
 > +    }
 > +    /* end workaround */
 > +    
 >      pkg = xcalloc(1, sizeof(*pkg));
 >  
 >      status = -1;
 > @@ -1490,7 +1553,7 @@ pkg_do(const char *pkgpath, int mark_automatic, int 
 > top_level)
 >                      pkg->install_logdir_real = NULL;
 >              }
 >  
 > -            if (check_dependencies(pkg))
 > +            if (check_dependencies(pkg, &dependency_stack_top))
 >                      goto nuke_pkgdb;
 >      } else {
 >              /*
 > @@ -1498,7 +1561,7 @@ pkg_do(const char *pkgpath, int mark_automatic, int 
 > top_level)
 >               * Install/update dependencies first and
 >               * write the current package to disk afterwards.
 >               */ 
 > -            if (check_dependencies(pkg))
 > +            if (check_dependencies(pkg, &dependency_stack_top))
 >                      goto clean_memory;
 >  
 >              if (write_meta_data(pkg))
 > @@ -1582,7 +1645,7 @@ pkg_perform(lpkg_head_t *pkgs)
 >      lpkg_t *lpp;
 >  
 >      while ((lpp = TAILQ_FIRST(pkgs)) != NULL) {
 > -            if (pkg_do(lpp->lp_name, Automatic, 1))
 > +            if (pkg_do(lpp->lp_name, Automatic, 1, NULL))
 >                      ++errors;
 >              TAILQ_REMOVE(pkgs, lpp, lp_link);
 >              free_lpkg(lpp);
 > diff --git a/pkgtools/pkg_install/files/configure 
 > b/pkgtools/pkg_install/files/configure
 
 Please don't include generated files in diffs, they just provide noise.
 
 > diff --git a/pkgtools/pkg_install/files/info/show.c 
 > b/pkgtools/pkg_install/files/info/show.c
 > index e44a6c9..89940e3 100644
 > --- a/pkgtools/pkg_install/files/info/show.c
 > +++ b/pkgtools/pkg_install/files/info/show.c
 > @@ -60,6 +60,9 @@ __RCSID("$NetBSD: show.c,v 1.31 2010/11/22 09:00:12 joerg 
 > Exp $");
 >  #if HAVE_ERR_H
 >  #include <err.h>
 >  #endif
 > +#if HAVE_SYS_PARAM_H
 > +#include <sys/param.h>
 > +#endif
 >  
 >  #include "defs.h"
 >  #include "lib.h"
 
 Why?
 
 > diff --git a/pkgtools/pkg_install/files/lib/plist.c 
 > b/pkgtools/pkg_install/files/lib/plist.c
 > index f1b6c6d..d171ad4 100644
 > --- a/pkgtools/pkg_install/files/lib/plist.c
 > +++ b/pkgtools/pkg_install/files/lib/plist.c
 > @@ -514,6 +514,7 @@ delete_package(Boolean ign_err, package_t *pkg, Boolean 
 > NoDeleteFiles,
 >      int     fail = SUCCESS;
 >      Boolean preserve;
 >      char    tmp[MaxPathSize];
 > +    char    cmd[MaxPathSize];
 >      const char *prefix = NULL, *name = NULL;
 >  
 >      if (!pkgdb_open(ReadWrite)) {
 > @@ -580,10 +581,12 @@ delete_package(Boolean ign_err, package_t *pkg, 
 > Boolean NoDeleteFiles,
 >              case PLIST_UNEXEC:
 >                      if (NoDeleteFiles)
 >                              break;
 > -                    format_cmd(tmp, sizeof(tmp), p->name, prefix, 
 > last_file);
 > -                    printf("Executing `%s'\n", tmp);
 > -                    if (!Fake && system(tmp)) {
 > -                            warnx("unexec command for `%s' failed", tmp);
 > +                    (void) snprintf(tmp, sizeof(tmp), "%s%s%s",
 > +                                    destdir ? destdir : "", destdir ? "/" : 
 > "", prefix);
 > +                    format_cmd(cmd, sizeof(cmd), p->name, tmp, last_file);
 > +                    printf("Executing `%s'\n", cmd);
 > +                    if (!Fake && system(cmd)) {
 > +                            warnx("unexec command for `%s' failed", cmd);
 >                              fail = FAIL;
 >                      }
 >                      break;
 > 
 
 Why?
 
 Joerg