> On Dec 7, 2007 10:47 AM, Allan McRae <allan.mcrae@qimr.edu.au> wrote:
> > Hi all,
> >
> > Below is a patch that tries to improve the handling of changelog
> > dumping. Now changelogs should also get dumped when using "pacman -Qcp
> > <file>" syntax as well as "pacman -Qc <pkg>" (See
> > http://bugs.archlinux.org/task/7321)
> >
> > This is my first patch to the pacman/libalpm code base so it probably
> > needs lots of checking. I stared at the code for a couple of hours
> > before I started but I am probably using some libalpm functions wrongly.
> > I would pay particular attention to the entirely new
> > "alpm_pkg_get_changelog" function in libalpm/package.c.
> >
> > The code builds cleanly but I can't test much more than that at the
> > moment due to my testing box having what I think is RAM issues leaving
> > me only my work laptop...
>
> Thanks a lot. I will look this over tonight, but I don't see anything
> that would cause drastic breakage.
> I do wonder why we never had a pkg_get_changelog() /me shrugs
>
> Thanks for the patch, it's good for the motivation when new people
> come on board.

I also say thank you for this patch, but my vote: not apply (yet).

Imho I told everything in my last mail:
-my main reason: this patch doesn't follow our earlier "conventions" at all (no
problem, you are new here, and this can be fixed)
-I also silently referred to the following fact:
If I didn't misunderstand add.c "monster", now .CHANGELOG is extracted as
/var/lib/.../changelog in extract_single_file().
However, extract_single_file is called in case of !(trans->flags &
PM_TRANS_FLAG_DBONLY) only. So this just breaks the borderline between "package
install" and "record to database".
This is an other reason why pmpkg_t solution is clearer.

But I would like to say a contra for my preferred implementation, too:
-higher memory usage [but, if you measure the memory usage with -Qo, this is not
notable at all ;-]

Bye


----------------------------------------------------
SZTE Egyetemi Könyvtár - http://www.bibl.u-szeged.hu
This mail sent through IMP: http://horde.org/imp/


_______________________________________________
pacman-dev mailing list
pacman-dev@archlinux.org
http://archlinux.org/mailman/listinfo/pacman-dev

On Sun, Dec 09, 2007 at 12:43:37AM +0100, Xavier wrote:
>
> I believe you should use handle->db_local here rather than
> alpm_db_register_local();
>
<snip>
>
> You probably got this part from _alpm_pkg_load, I guess it's alright (except
> you probably want to add an archive_read_finish(archive); there).
>
> But you could also use _alpm_unpack, like in _alpm_runscriptlet.
>
> Btw, I couldn't get your patch to apply cleanly, because of many line breaks.
> So I started rewriting it from the start, but in the same time, I wanted to
> experiment reading directly from the archive, which resulted in the patch I
> submitted somewhere else in this thread.
> I don't know if it's better or not, but if it is, all the credits still go to
> you, for looking at the problem in the first place, and providing a good
> patch to work with. So thanks again.

Well, here is an updated patch, with the above suggestions. I also made
several other minor cleanups, and a few fixes to get it to work correctly.
Note : this requires a slight edit of _alpm_unpack to work correctly (return
1 when the file to extract, the changelog here, is not found), but this
should come as another patch.


>From 4def7e552f22b9ad26dd6193bdab40547bd5d46f Mon Sep 17 00:00:00 2001
From: Allan McRae <mcrae_allan at hotmail.com>
Date: Sat, 8 Dec 2007 19:18:49 +0100
Subject: [PATCH] Improve changelog handling.

Allows dumping of changelog for both installed packages and from package
files (See FS#7321).
Moves querying of changelog file in alpm backend.

Signed-off-by: Allan McRae <mcrae_allan at hotmail.com>

[Xavier : reuse _alpm_unpack function, several minor fixes and cleanups]
Signed-off-by: Chantry Xavier <shiningxc@gmail.com>
---
lib/libalpm/alpm.h | 1 +
lib/libalpm/package.c | 56 +++++++++++++++++++++++++++++++++++++++++++++++++
src/pacman/package.c | 28 ++++++++---------------
src/pacman/package.h | 2 +-
src/pacman/query.c | 9 +-------
5 files changed, 69 insertions(+), 27 deletions(-)

diff --git a/lib/libalpm/alpm.h b/lib/libalpm/alpm.h
index 1e18ad9..7f6c9eb 100644
--- a/lib/libalpm/alpm.h
+++ b/lib/libalpm/alpm.h
@@ -216,6 +216,7 @@ alpm_list_t *alpm_pkg_get_deltas(pmpkg_t *pkg);
alpm_list_t *alpm_pkg_get_replaces(pmpkg_t *pkg);
alpm_list_t *alpm_pkg_get_files(pmpkg_t *pkg);
alpm_list_t *alpm_pkg_get_backup(pmpkg_t *pkg);
+alpm_list_t *alpm_pkg_get_changelog(pmpkg_t *pkg);
unsigned short alpm_pkg_has_scriptlet(pmpkg_t *pkg);

unsigned long alpm_pkg_download_size(pmpkg_t *newpkg, pmdb_t *db_local);
diff --git a/lib/libalpm/package.c b/lib/libalpm/package.c
index 172456d..3e5369d 100644
--- a/lib/libalpm/package.c
+++ b/lib/libalpm/package.c
@@ -491,6 +491,62 @@ alpm_list_t SYMEXPORT *alpm_pkg_get_backup(pmpkg_t *pkg)
return pkg->backup;
}

+alpm_list_t SYMEXPORT *alpm_pkg_get_changelog(pmpkg_t *pkg)
+{
+ ALPM_LOG_FUNC;
+
+ /* Sanity checks */
+ ASSERT(handle != NULL, return(NULL));
+ ASSERT(pkg != NULL, return(NULL));
+
+ alpm_list_t *cl = NULL;
+ char clfile[PATH_MAX];
+ char tmpdir[PATH_MAX];
+ char line[PATH_MAX];
+ FILE* fp = NULL;
+ int clean_tmpdir = 0;
+
+ if(pkg->origin == PKG_FROM_CACHE) {
+ snprintf(clfile, PATH_MAX, "%s/%s/%s-%s/changelog",
+ alpm_option_get_dbpath(),
+ alpm_db_get_name(handle->db_local),
+ alpm_pkg_get_name(pkg),
+ alpm_pkg_get_version(pkg));
+ } else if(pkg->origin == PKG_FROM_FILE) {
+ const char *pkgfile = pkg->origin_data.file;
+
+ snprintf(tmpdir, PATH_MAX, "%stmp/alpm_XXXXXX", handle->root);
+ if(mkdtemp(tmpdir) == NULL) {
+ _alpm_log(PM_LOG_ERROR, _("could not create temp directory
"));
+ return(NULL);
+ } else {
+ clean_tmpdir = 1;
+ }
+
+ snprintf(clfile, PATH_MAX, "%s/.CHANGELOG", tmpdir);
+ if(_alpm_unpack(pkgfile, tmpdir, ".CHANGELOG")) {
+ goto cleanup;
+ }
+ }
+
+ fp = fopen(clfile, "r");
+ if(fp == NULL){
+ return(NULL);
+ }
+
+ while(fgets(line, PATH_MAX, fp)) {
+ cl = alpm_list_add(cl, strdup(line));
+ }
+ fclose(fp);
+
+cleanup:
+ if(clean_tmpdir && _alpm_rmrf(tmpdir)) {
+ _alpm_log(PM_LOG_WARNING, _("could not remove tmpdir %s
"), tmpdir);
+ }
+
+ return(cl);
+}
+
unsigned short SYMEXPORT alpm_pkg_has_scriptlet(pmpkg_t *pkg)
{
ALPM_LOG_FUNC;
diff --git a/src/pacman/package.c b/src/pacman/package.c
index ac3f820..295b9b3 100644
--- a/src/pacman/package.c
+++ b/src/pacman/package.c
@@ -232,28 +232,20 @@ void dump_pkg_files(pmpkg_t *pkg)
fflush(stdout);
}

-/* Display the changelog of an installed package
+/* Display the changelog of a package
*/
-void dump_pkg_changelog(char *clfile, const char *pkgname)
+void dump_pkg_changelog(pmpkg_t *pkg)
{
- FILE* fp = NULL;
- char line[PATH_MAX+1];
+ alpm_list_t *i;
+ alpm_list_t *changelog = alpm_pkg_get_changelog(pkg);

- if((fp = fopen(clfile, "r")) == NULL)
- {
- fprintf(stderr, _("error: no changelog available for '%s'.
"), pkgname);
- return;
- }
- else
- {
- while(!feof(fp))
- {
- fgets(line, (int)PATH_MAX, fp);
- printf("%s", line);
- line[0] = '';
+ if(changelog == NULL) {
+ fprintf(stderr, _("error: no changelog available for '%s'.
"), alpm_pkg_get_name(pkg));
+ } else {
+ for(i = changelog; i; i = alpm_list_next(i)) {
+ fprintf(stdout, "%s", (char *)alpm_list_getdata(i));
}
- fclose(fp);
- return;
+ FREELIST(changelog);
}
}

diff --git a/src/pacman/package.h b/src/pacman/package.h
index 0e4bb0f..7dfc054 100644
--- a/src/pacman/package.h
+++ b/src/pacman/package.h
@@ -28,7 +28,7 @@ void dump_pkg_sync(pmpkg_t *pkg, const char *treename);

void dump_pkg_backups(pmpkg_t *pkg);
void dump_pkg_files(pmpkg_t *pkg);
-void dump_pkg_changelog(char *clfile, const char *pkgname);
+void dump_pkg_changelog(pmpkg_t *pkg);

#endif /* _PM_PACKAGE_H */

diff --git a/src/pacman/query.c b/src/pacman/query.c
index 8a8765b..1307077 100644
--- a/src/pacman/query.c
+++ b/src/pacman/query.c
@@ -312,14 +312,7 @@ static void display(pmpkg_t *pkg)
dump_pkg_files(pkg);
}
if(config->op_q_changelog) {
- char changelog[PATH_MAX];
- /* TODO should be done in the backend- no raw DB stuff up front */
- snprintf(changelog, PATH_MAX, "%s/%s/%s-%s/changelog",
- alpm_option_get_dbpath(),
- alpm_db_get_name(db_local),
- alpm_pkg_get_name(pkg),
- alpm_pkg_get_version(pkg));
- dump_pkg_changelog(changelog, alpm_pkg_get_name(pkg));
+ dump_pkg_changelog(pkg);
}
if(!config->op_q_info && !config->op_q_list && !config->op_q_changelog) {
if (!config->quiet) {
--
1.5.3.7


_______________________________________________
pacman-dev mailing list
pacman-dev@archlinux.org
http://archlinux.org/mailman/listinfo/pacman-dev

Xavier wrote:
> On Fri, Dec 07, 2007 at 05:56:32PM -0600, Aaron Griffin wrote:
>
>> True, we don't need to. The current function is returning a
>> alpm_list_t that contains lines, but i guess it's not really needed.
>> We can return the full text as one big string, and leave it up to
>> front ends to do some sort of parsing if they wish
>>
>>
>
> Something like this ? :
>
>
I like the idea of not creating a temporary file.

<snip>
> +
> + while(!done) {
> + changelog = realloc(changelog, size + bufsize);
> + p = changelog + size;
> + size += bufsize;
> + if(pkg->origin == PKG_FROM_CACHE) {
> + n = fread(p, 1, bufsize, fp);
> + } else if(pkg->origin == PKG_FROM_FILE) {
> + n = archive_read_data(archive, p, bufsize);
> + }
> + if(n != bufsize) {
> + done = 1;
> + *(p+n) = '';
> + }
> + }
> +
<snip>

I'm concerned all the realloc statements may get expensive if the
changelog is too long. How about sticking to the alpm_list_t return
type and just dumping each buffer read into the list? This would
require changing the dump_pkg_changelog to not put a newline after the
ouput of every list item if PKG_FROM_FILE. Would that be any better?

Allan



_______________________________________________
pacman-dev mailing list
pacman-dev@archlinux.org
http://archlinux.org/mailman/listinfo/pacman-dev

On Dec 8, 2007 7:55 PM, Allan McRae <mcrae_allan@hotmail.com> wrote:
> Xavier wrote:
> > On Fri, Dec 07, 2007 at 05:56:32PM -0600, Aaron Griffin wrote:
> >
> >> True, we don't need to. The current function is returning a
> >> alpm_list_t that contains lines, but i guess it's not really needed.
> >> We can return the full text as one big string, and leave it up to
> >> front ends to do some sort of parsing if they wish
> >>
> >>
> >
> > Something like this ? :
> >
> >
> I like the idea of not creating a temporary file.
>
> <snip>
> > +
> > + while(!done) {
> > + changelog = realloc(changelog, size + bufsize);
> > + p = changelog + size;
> > + size += bufsize;
> > + if(pkg->origin == PKG_FROM_CACHE) {
> > + n = fread(p, 1, bufsize, fp);
> > + } else if(pkg->origin == PKG_FROM_FILE) {
> > + n = archive_read_data(archive, p, bufsize);
> > + }
> > + if(n != bufsize) {
> > + done = 1;
> > + *(p+n) = '';
> > + }
> > + }
> > +
> <snip>
>
> I'm concerned all the realloc statements may get expensive if the
> changelog is too long. How about sticking to the alpm_list_t return
> type and just dumping each buffer read into the list? This would
> require changing the dump_pkg_changelog to not put a newline after the
> ouput of every list item if PKG_FROM_FILE. Would that be any better?
>
> Allan

This was exactly what i was thinking- get chunks at a time from the
backend. I was thinking that the front end would allocate a buffer
(this is similar to how other libs and even things like a glibc read
work) and pass the buffer and size to the backend, and then the
backend would fill it.

I'll see what I can hack together- nothing like having 18 different
people working on the same patch.

-Dan

_______________________________________________
pacman-dev mailing list
pacman-dev@archlinux.org
http://archlinux.org/mailman/listinfo/pacman-dev