[pacman-dev] [PATCH] Introduce -D, --database
Nagy Gabor
ngaba at bibl.u-szeged.hu
Wed Sep 9 06:13:44 EDT 2009
> On Tue, Sep 8, 2009 at 10:30 PM, Dan McGee<dan at archlinux.org> wrote:
> > From: Nagy Gabor <ngaba at bibl.u-szeged.hu>
> >
> > The request of FS#12950 is implemented.
> >
> > On the backend side, I introduced a new function, alpm_db_set_pkgreason(),
> > to modify the install reason of a package in the local database. On the
> > front-end side, I introduced a new main operation, -D/--database, which has
> > two options, --asdeps and --asexplicit. I documented this in pacman manual.
> > I've created two pactests to test -D: database001.py and database002.py.
> >
> > Signed-off-by: Nagy Gabor <ngaba at bibl.u-szeged.hu>
>
> I sent this here because I have a few comments, and I'm guessing I
> dropped the original submission on the floor a long time ago.
>
> > ---
> > doc/pacman.8.txt | 12 +++++-
> > lib/libalpm/alpm.h | 16 +++++---
> > lib/libalpm/db.c | 38 ++++++++++++++++++
> > pactest/tests/database001.py | 11 +++++
> > pactest/tests/database002.py | 11 +++++
> > src/pacman/Makefile.am | 1 +
> > src/pacman/conf.h | 3 +-
> > src/pacman/database.c | 90 ++++++++++++++++++++++++++++++++++++++++++
> > src/pacman/pacman.c | 22 ++++++++--
> > src/pacman/pacman.h | 2 +
> > src/pacman/util.c | 2 +-
> > 11 files changed, 193 insertions(+), 15 deletions(-)
> > create mode 100644 pactest/tests/database001.py
> > create mode 100644 pactest/tests/database002.py
> > create mode 100644 src/pacman/database.c
> >
> > diff --git a/doc/pacman.8.txt b/doc/pacman.8.txt
> > index 7f92ec7..36b027c 100644
> > --- a/doc/pacman.8.txt
> > +++ b/doc/pacman.8.txt
> > @@ -28,6 +28,12 @@ front ends to be written (for instance, a GUI front end).
> >
> > Operations
> > ----------
> > +*-D, \--database*::
> > + Modify the package database. This options allows you to modify certain
> > + attributes of the installed packages in pacman's database. At the
> > + moment, you can only change the install reason. See the '\--asdeps'
> > + and '\--asexplicit' options below.
> > +
> > *-Q, \--query*::
> > Query the package database. This operation allows you to view installed
> > packages and their files, as well as meta-information about individual
> > @@ -101,13 +107,15 @@ Options
> > Install packages non-explicitly; in other words, fake their install reason
> > to be installed as a dependency. This is useful for makepkg and other
> > build from source tools that need to install dependencies before building
> > - the package.
> > + the package. This option can be also used in combination with '-D' to
> > + change the install reason of local packages without reinstalling.
> >
> > *\--asexplicit*::
> > Install packages explicitly; in other words, fake their install reason to
> > be explicitly installed. This is useful if you want to mark a dependency
> > as explicitly installed so it will not be removed by the '\--recursive'
> > - remove operation.
> > + remove operation. This option can be also used in combination with '-D'
> > + to change the install reason of local packages without reinstalling.
>
> Not so fond of this way of organizing (or disorganizing?) the manpage.
> We don't do this anywhere else.
--asdeps and --asexplicit are listed in "general options" section,
that's why I've chosen this solution. I am fine with moving them to -S,
-U and -D sections. Atm there is no -Q and -D options sections, so maybe
we should just remove the 2 added lines from --asdeps/--asexplicit, and
make the -D description more concrete, I dunno. With pacman -Rh,
--asdeps and --asexplicit are not listed (they clearly belongs to -S,
-D and -U only), so maybe creating new -D and -U sections is better
even if they will be too short. I let you choose. ;-)
> >
> > *-b, \--dbpath* <'path'>::
> > Specify an alternative database location (a typical default is
> > diff --git a/lib/libalpm/alpm.h b/lib/libalpm/alpm.h
> > index 661df45..caea28c 100644
> > --- a/lib/libalpm/alpm.h
> > +++ b/lib/libalpm/alpm.h
> > @@ -160,6 +160,15 @@ pmdb_t *alpm_option_get_localdb();
> > alpm_list_t *alpm_option_get_syncdbs();
> >
> > /*
> > + * Install reasons -- ie, why the package was installed
> > + */
> > +
> > +typedef enum _pmpkgreason_t {
> > + PM_PKG_REASON_EXPLICIT = 0, /* explicitly requested by the user */
> > + PM_PKG_REASON_DEPEND = 1 /* installed as a dependency for another package */
> > +} pmpkgreason_t;
> > +
> > +/*
> > * Databases
> > */
> >
> > @@ -182,6 +191,7 @@ alpm_list_t *alpm_db_get_pkgcache(pmdb_t *db);
> > pmgrp_t *alpm_db_readgrp(pmdb_t *db, const char *name);
> > alpm_list_t *alpm_db_get_grpcache(pmdb_t *db);
> > alpm_list_t *alpm_db_search(pmdb_t *db, const alpm_list_t* needles);
> > +int alpm_db_set_pkgreason(pmdb_t *db, const char *name, pmpkgreason_t reason);
>
> This seems silly to me. Why can't the signature be the standard
> int alpm_db_set_pkgreason(pmpkg_t *pkg, pmpkgreason_t reason);
> ? Just ensure there is a check that it isn't a package file, and it
> saves all the lookup junk since that is a public API anyway.
This was done for safety reason, I even didn't want to add the db
param, and force the use of local db (but see [*]). Well, probably the
front-end cannot do so much dirty things (store pmpkg_t, remove that
package, call alpm_db_set_pkgreason), so maybe I was too paranoid. But
I still think this is better. (The main problem is that after a pmpkg_t
is duplicated, it seems like the new pmpkg_t is the part of pkgcache,
but it is not, it is "detached".) The speed is not an issue here,
the only difference between your and my proposal is that front-end or
alpm scans pkgcache for name.
[*] One more weak reasoning: ~all alpm_db functions have db param.
But if I didn't convince you, of course I will change this.
> >
> > /*
> > * Packages
> > @@ -189,12 +199,6 @@ alpm_list_t *alpm_db_search(pmdb_t *db, const alpm_list_t* needles);
> >
> > /* Info parameters */
> >
> > -/* reasons -- ie, why the package was installed */
> > -typedef enum _pmpkgreason_t {
> > - PM_PKG_REASON_EXPLICIT = 0, /* explicitly requested by the user */
> > - PM_PKG_REASON_DEPEND = 1 /* installed as a dependency for another package */
> > -} pmpkgreason_t;
> > -
> > int alpm_pkg_load(const char *filename, unsigned short full, pmpkg_t **pkg);
> > int alpm_pkg_free(pmpkg_t *pkg);
> > int alpm_pkg_checkmd5sum(pmpkg_t *pkg);
> > diff --git a/lib/libalpm/db.c b/lib/libalpm/db.c
> > index 62c2e0a..75c505e 100644
> > --- a/lib/libalpm/db.c
> > +++ b/lib/libalpm/db.c
> > @@ -321,6 +321,44 @@ alpm_list_t SYMEXPORT *alpm_db_search(pmdb_t *db, const alpm_list_t* needles)
> > return(_alpm_db_search(db, needles));
> > }
> >
> > +/* Set install reason for a package in db
> > + * @param db pointer to the package database
> > + * @param name the name of the package
> > + * @param reason the new install reason
> > + * @return 0 on success, -1 on error (pm_errno is set accordingly)
> > + */
> > +int SYMEXPORT alpm_db_set_pkgreason(pmdb_t *db, const char *name, pmpkgreason_t reason)
> > +{
> > + ALPM_LOG_FUNC;
> > +
> > + /* Sanity checks */
> > + ASSERT(handle != NULL, RET_ERR(PM_ERR_HANDLE_NULL, -1));
> > + ASSERT(db != NULL && name != NULL, RET_ERR(PM_ERR_WRONG_ARGS, -1));
> > +
> > + pmpkg_t *pkg = _alpm_db_get_pkgfromcache(db, name);
> > + if(pkg == NULL) {
> > + RET_ERR(PM_ERR_PKG_NOT_FOUND, -1);
> > + }
> > +
> > + _alpm_log(PM_LOG_DEBUG, "setting install reason %u for %s/%s\n", reason, db->treename, name);
> > + /* read DESC */
> > + if(_alpm_db_read(db, pkg, INFRQ_DESC)) {
> > + return(-1);
> > + }
> > + if(pkg->reason == reason) {
> > + /* we are done */
> > + return(0);
> > + }
> > + /* set reason (in pkgcache) */
> > + pkg->reason = reason;
> This is scary- manual DESC level reading, direct ->reason accessors-
> don't we abstract this all away in package.c?
I don't agree here. pkg_get_reason would be equivalent here, but I
still think this is better. We want to read the _whole_ DESC file here,
because we want to write it back to hdd later (after changing reason).
Of course the same thing happens with pkg_get_reason, but imho this is
more suggestive. _alpm_db_read is intelligent, if DESC is already
loaded, it won't be loaded again.
However, there is a valid concern here: What if we move reason field to
another file, and we will forget about this... Then we should also
change the _alpm_db_write line too, so this is not a problem of
_alpm_db_read usage. (Btw, database*.py pactests would catch this
immediately.)
Bye
P.S.: I didn't get this mail from pacman-dev (only directly from Dan),
but I have "Avoid duplicate copies of messages? No" set in ML options.
(Why?) So I just forward my mail sent to him.
More information about the pacman-dev
mailing list