Re: [PATCH] Add --foreign-architecture and --print-foreign-architectures options
Hi!
Thanks for the patch! Mostly comments about style issues. Also could
you document the options in the man page? :)
On Thu, 2009-08-27 at 13:25:55 -0700, Steve Langasek wrote:
> Two new options to dpkg, needed for multiarch:
>
> --foreign-architecture lets you specify that packages for the named
> architecture should be installable without the use of --force-architecture
>
> --print-foreign-architectures prints out a space-separated list of all
> architectures so configured, so that front-ends can query the list.
> diff --git a/src/enquiry.c b/src/enquiry.c
> index 5f773f8..cb5f410 100644
> --- a/src/enquiry.c
> +++ b/src/enquiry.c
> @@ -395,6 +395,20 @@ printinstarch(const char *const *argv)
> printarch(argv);
> }
>
> +void
> +printforeignarchs(const char *const *argv)
> +{
> + struct architecture *archp;
> + if (*argv)
> + badusage(_("--%s takes no arguments"), cipaction->olong);
Blank line after definition of local variables.
> + for (archp = foreign_archs; archp; archp = archp->next) {
> + if (printf("%s ",archp->arch) == EOF) werr("stdout");
> + }
> + if (printf("\n") == EOF) werr("stdout");
> + if (fflush(stdout)) werr("stdout");
> +}
> +
Spaces after commas. And just do printf()s and fflush() and check for
errors at the end with ferror() and abort with werr(). Arguably this is
how the current code is doing it, but I've been meaning to fix it for
some time, and this is a good excuse, so I'm working on it locally for
the current code base.
> void cmpversions(const char *const *argv) {
> struct relationinfo {
> const char *string;
> diff --git a/src/main.c b/src/main.c
> index d237054..b2cce63 100644
> --- a/src/main.c
> +++ b/src/main.c
> @@ -152,6 +152,7 @@ usage(void)
>
> const char thisname[]= "dpkg";
> const char architecture[]= ARCHITECTURE;
> +struct architecture *foreign_archs = NULL;
Better foreign_arches, well sounds that way to me, but then I'm not a
native speaker. :)
> const char printforhelp[]= N_(
> "Type dpkg --help for help about installing and deinstalling packages [*];\n"
> "Use `dselect' or `aptitude' for user-friendly package management;\n"
> @@ -315,6 +316,15 @@ static void setpipe(const struct cmdinfo *cip, const char *value) {
> *pipe_head = pipe_new;
> }
>
> +static void set_foreign_arch(const struct cmdinfo *cip, const char *value) {
Braces and return type on their own lines.
> + struct architecture *arch_new;
> +
> + arch_new = nfmalloc(sizeof(struct architecture));
> + arch_new->arch = m_strdup(value);
> + arch_new->next = foreign_archs;
> + foreign_archs = arch_new;
> +}
> +
Try to match the file indent size, in this case 2 spaces. In cmdinfos
you are passing foreign_archs as an argument but then hardcoding it
here, take it from cip instead.
> static void setforce(const struct cmdinfo *cip, const char *value) {
> const char *comma;
> size_t l;
> @@ -426,6 +436,7 @@ static const struct cmdinfo cmdinfos[]= {
> ACTION( "print-installation-architecture", 0, act_printinstarch, printinstarch ),
> ACTION( "predep-package", 0, act_predeppackage, predeppackage ),
> ACTION( "compare-versions", 0, act_cmpversions, cmpversions ),
> + ACTION( "print-foreign-architectures", 0, act_foreignarchs, printforeignarchs ),
Place it after print-installation-architecture.
> /*
> ACTION( "command-fd", 'c', act_commandfd, commandfd ),
> */
> @@ -462,6 +473,7 @@ static const struct cmdinfo cmdinfos[]= {
> { "licence", 0, 0, NULL, NULL, showcopyright, 0 },
> /* US spelling. */
> { "license", 0, 0, NULL, NULL, showcopyright, 0 },
> + { "foreign-architecture", 0,1, NULL, NULL, set_foreign_arch, 0, &foreign_archs },
And this one before status-fd.
> ACTIONBACKEND( "build", 'b', BACKEND),
> ACTIONBACKEND( "contents", 'c', BACKEND),
> ACTIONBACKEND( "control", 'e', BACKEND),
> diff --git a/src/main.h b/src/main.h
> index eb5bfce..825d39a 100644
> --- a/src/main.h
> +++ b/src/main.h
> @@ -61,7 +61,7 @@ enum action { act_unset, act_install, act_unpack, act_avail, act_configure,
> act_forgetold,
> act_getselections, act_setselections, act_clearselections,
> act_assertepoch, act_assertlongfilenames, act_assertmulticonrep,
> - act_commandfd };
> + act_commandfd, act_foreignarchs };
Place this on a line by its own after act_printarch.
> enum conffopt {
> cfof_prompt = 001,
> @@ -102,6 +102,12 @@ extern const char *instdir;
> extern struct pkginqueue *ignoredependss;
> extern const char architecture[];
>
> +struct architecture {
> + struct architecture *next;
> + const char *arch;
> +};
Better use ‘name’ instead of ‘arch’ for the member.
> +extern struct architecture *foreign_archs;
> +
> /* from archives.c */
>
> void archivefiles(const char *const *argv);
> @@ -130,6 +136,7 @@ void assertmulticonrep(const char *const *argv);
> void predeppackage(const char *const *argv);
> void printarch(const char *const *argv);
> void printinstarch(const char *const *argv);
> +void printforeignarchs(const char *const *argv);
Better something like print_foreign_arches().
> void cmpversions(const char *const *argv) DPKG_ATTR_NORET;
>
> /* Intended for use as a comparison function for qsort when
> diff --git a/src/processarc.c b/src/processarc.c
> index 0b8d248..75e6427 100644
> --- a/src/processarc.c
> +++ b/src/processarc.c
> @@ -216,10 +216,21 @@ void process_archive(const char *filename) {
>
> if (pkg->available.architecture && *pkg->available.architecture &&
> strcmp(pkg->available.architecture,"all") &&
> - strcmp(pkg->available.architecture,architecture))
> - forcibleerr(fc_architecture,
> - _("package architecture (%s) does not match system (%s)"),
> - pkg->available.architecture,architecture);
> + strcmp(pkg->available.architecture,architecture)) {
> + struct architecture *archp;
> + int archok = 0;
> +
> + for (archp = foreign_archs; archp; archp = archp->next) {
> + if (!strcmp(pkg->available.architecture,archp->arch)) {
> + archok = 1;
> + break;
> + }
> + }
> + if (!archok)
> + forcibleerr(fc_architecture,
> + _("package architecture (%s) does not match system (%s)"),
> + pkg->available.architecture,architecture);
> + }
>
> if (!pkg->installed.valid) blankpackageperfile(&pkg->installed);
> assert(pkg->available.valid);
Use strcmp() == 0 instead. Spaces after comma. And move the foreign
comparator into a function of its own. And do not call it yet, I'd
like to add that part only later on once it's safe to install foreign
architecture packages. Or rather, split it into another patch that
we'll keep at the end of the branch. And thinking about it maybe add
a warning on --foreign-architecture stating it's currently a no-op.
The rationale is that I'd like to be able to incrementally commit the
different pieces of the puzzle so that we can start testing those w/o
breaking the repository or producing a dpkg more lax than it should be.
thanks,
guillem
Reply to: