[pacman-dev] [PATCH] Improve changelog handling
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... Cheers, Allan From 47affca982ec2d449bd6d96846172052dbdd3da6 Mon Sep 17 00:00:00 2001 From: Allan McRae <mcrae_allan@hotmail.com> Date: Sat, 8 Dec 2007 02:35:00 +1000 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@hotmail.com> --- lib/libalpm/alpm.h | 1 + lib/libalpm/package.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++++ src/pacman/package.c | 33 ++++++++++------------ src/pacman/package.h | 2 +- src/pacman/query.c | 9 +----- 5 files changed, 89 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..bbdb95d 100644 --- a/lib/libalpm/package.c +++ b/lib/libalpm/package.c @@ -491,6 +491,77 @@ 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)); + + int ret = ARCHIVE_OK; + alpm_list_t *cl = NULL; + char clfile[PATH_MAX]; + char *tmpfile = NULL; + struct archive *archive; + struct archive_entry *entry; + int fd = -1; + char line[PATH_MAX]; + + if(pkg->origin == PKG_FROM_CACHE) { + snprintf(clfile, PATH_MAX, "%s/%s/%s-%s/changelog", + alpm_option_get_dbpath(), + alpm_db_get_name(alpm_db_register_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; + + if((archive = archive_read_new()) == NULL) { + RET_ERR(PM_ERR_LIBARCHIVE_ERROR, NULL); + } + + archive_read_support_compression_all(archive); + archive_read_support_format_all(archive); + + if (archive_read_open_filename(archive, pkgfile, + ARCHIVE_DEFAULT_BYTES_PER_BLOCK) != ARCHIVE_OK) { + RET_ERR(PM_ERR_PKG_OPEN, NULL); + } + + while((ret = archive_read_next_header(archive, &entry)) == ARCHIVE_OK) { + const char *entry_name = archive_entry_pathname(entry); + + if(strcmp(entry_name, ".CHANGELOG") == 0) { + tmpfile = strdup("/tmp/alpm_XXXXXX"); + fd = mkstemp(tmpfile); + archive_read_data_into_fd(archive, fd); + break; + } + } + snprintf(clfile, PATH_MAX, "%s", tmpfile); + } + + FILE* fp = fopen(clfile, "r"); + if(fp == NULL){ + return cl; + } + + while(!feof(fp)) { + fgets(line, (int)PATH_MAX, fp); + cl = alpm_list_add(cl, strdup(line)); + } + fclose(fp); + + if(pkg->origin == PKG_FROM_FILE) { + unlink(tmpfile); + FREE(tmpfile); + close(fd); + } + + 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..00c3535 100644 --- a/src/pacman/package.c +++ b/src/pacman/package.c @@ -232,29 +232,26 @@ 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]; - - if((fp = fopen(clfile, "r")) == NULL) - { - fprintf(stderr, _("error: no changelog available for '%s'.\n"), pkgname); + alpm_list_t *changelog = NULL; + alpm_list_t *i; + char line[PATH_MAX]; + + changelog = alpm_pkg_get_changelog(pkg); + if(changelog == NULL) { + fprintf(stderr, _("error: no changelog available for '%s'.\n"), alpm_pkg_get_name(pkg)); return; - } - else - { - while(!feof(fp)) - { - fgets(line, (int)PATH_MAX, fp); - printf("%s", line); - line[0] = '\0'; + } else { + for(i = changelog; i; i = alpm_list_next(i)) { + snprintf(line, PATH_MAX-1, "%s", (char*)alpm_list_getdata(i)); + fprintf(stdout, "%s\n", line); } - fclose(fp); - return; } + + FREELIST(changelog); } /* vim: set ts=2 sw=2 noet: */ 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
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...
Cheers, Allan
From 47affca982ec2d449bd6d96846172052dbdd3da6 Mon Sep 17 00:00:00 2001 From: Allan McRae <mcrae_allan@hotmail.com> Date: Sat, 8 Dec 2007 02:35:00 +1000 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@hotmail.com> --- lib/libalpm/alpm.h | 1 + lib/libalpm/package.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++++ src/pacman/package.c | 33 ++++++++++------------ src/pacman/package.h | 2 +- src/pacman/query.c | 9 +----- 5 files changed, 89 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..bbdb95d 100644 --- a/lib/libalpm/package.c +++ b/lib/libalpm/package.c @@ -491,6 +491,77 @@ 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)); + + int ret = ARCHIVE_OK; + alpm_list_t *cl = NULL; + char clfile[PATH_MAX]; + char *tmpfile = NULL; + struct archive *archive; + struct archive_entry *entry; + int fd = -1; + char line[PATH_MAX]; + + if(pkg->origin == PKG_FROM_CACHE) { + snprintf(clfile, PATH_MAX, "%s/%s/%s-%s/changelog", + alpm_option_get_dbpath(), + alpm_db_get_name(alpm_db_register_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; + + if((archive = archive_read_new()) == NULL) { + RET_ERR(PM_ERR_LIBARCHIVE_ERROR, NULL); + } + + archive_read_support_compression_all(archive); + archive_read_support_format_all(archive); + + if (archive_read_open_filename(archive, pkgfile, + ARCHIVE_DEFAULT_BYTES_PER_BLOCK) != ARCHIVE_OK) { + RET_ERR(PM_ERR_PKG_OPEN, NULL); + } + + while((ret = archive_read_next_header(archive, &entry)) == ARCHIVE_OK) { + const char *entry_name = archive_entry_pathname(entry); + + if(strcmp(entry_name, ".CHANGELOG") == 0) { + tmpfile = strdup("/tmp/alpm_XXXXXX"); + fd = mkstemp(tmpfile); + archive_read_data_into_fd(archive, fd); + break; + } + } + snprintf(clfile, PATH_MAX, "%s", tmpfile); + } + + FILE* fp = fopen(clfile, "r"); + if(fp == NULL){ + return cl; + } + + while(!feof(fp)) { + fgets(line, (int)PATH_MAX, fp); + cl = alpm_list_add(cl, strdup(line)); + } + fclose(fp); + + if(pkg->origin == PKG_FROM_FILE) { + unlink(tmpfile); + FREE(tmpfile); + close(fd); + } + + 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..00c3535 100644 --- a/src/pacman/package.c +++ b/src/pacman/package.c @@ -232,29 +232,26 @@ 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]; - - if((fp = fopen(clfile, "r")) == NULL) - { - fprintf(stderr, _("error: no changelog available for '%s'.\n"), pkgname); + alpm_list_t *changelog = NULL; + alpm_list_t *i; + char line[PATH_MAX]; + + changelog = alpm_pkg_get_changelog(pkg); + if(changelog == NULL) { + fprintf(stderr, _("error: no changelog available for '%s'.\n"), alpm_pkg_get_name(pkg)); return; - } - else - { - while(!feof(fp)) - { - fgets(line, (int)PATH_MAX, fp); - printf("%s", line); - line[0] = '\0'; + } else { + for(i = changelog; i; i = alpm_list_next(i)) { + snprintf(line, PATH_MAX-1, "%s", (char*)alpm_list_getdata(i)); + fprintf(stdout, "%s\n", line); } - fclose(fp); - return; } + + FREELIST(changelog); }
/* vim: set ts=2 sw=2 noet: */ 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
Well, personally I don't know too much about this changelog stuff so I may be totally wrong, but if I understood your patch correctly, there is a changelog file in our (local?)db or a .CHANGELOG file in the package (wow, db <-> package format again). Probably this patch is good, but this doesn't follow our usual methods to handle these things. Since you said that you are new in pacman hacking, I can suggest you to give a look at depends or desc handling for example. These thinks are usually _loaded_ in be_files.c (on demand) and in package.c (but hopefully the 2nd will be changed in the future), and these helper low-lever functions fill in pmpkg_t accordingly. So my suggestion: implement a .changelog field to pmpkg_t and follow our usual method to query it, and modify _alpm_db_read and maybe _alpm_db_write. Since this is a new file (descfile) we need a new INFRQ_CHANGELOG infolevel. If this is implemented, we can "install" changelog to the localdb with the help of pmpkg_t, not extract .CHANGELOG as changelog... (well, imho all low level localdb manipulations should be done with _alpm_db_read and _alpm_db_write <- see also: PM_TRANS_FLAG_DBONLY) Bye ---------------------------------------------------- SZTE Egyetemi Könyvtár - http://www.bibl.u-szeged.hu This mail sent through IMP: http://horde.org/imp/
On Dec 7, 2007 11:37 AM, Nagy Gabor <ngaba@bibl.u-szeged.hu> wrote:
So my suggestion: implement a .changelog field to pmpkg_t and follow our usual method to query it, and modify _alpm_db_read and maybe _alpm_db_write. Since this is a new file (descfile) we need a new INFRQ_CHANGELOG infolevel.
I actually prefer Allan's method much much more. It is very clean and functions the way an accessor should. If changing an accessor changes like 4 files and 7 different functions, it's garbage.
On Fri, Dec 07, 2007 at 12:01:51PM -0600, Aaron Griffin wrote:
On Dec 7, 2007 11:37 AM, Nagy Gabor <ngaba@bibl.u-szeged.hu> wrote:
So my suggestion: implement a .changelog field to pmpkg_t and follow our usual method to query it, and modify _alpm_db_read and maybe _alpm_db_write. Since this is a new file (descfile) we need a new INFRQ_CHANGELOG infolevel.
I actually prefer Allan's method much much more. It is very clean and functions the way an accessor should. If changing an accessor changes like 4 files and 7 different functions, it's garbage.
I'm not sure which one I prefer. Sure, Allan's method is practical and easy, (I believe the code could be even more elegant by reusing the existing _alpm_unpack function), but doesn't it go in the opposite direction of having every code specific to the db behind be_files.c? And I don't know if reading the archive from various places in the code is really elegant.
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.
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/
Allan McRae wrote:
+ if(strcmp(entry_name, ".CHANGELOG") == 0) { + tmpfile = strdup("/tmp/alpm_XXXXXX"); + fd = mkstemp(tmpfile); + archive_read_data_into_fd(archive, fd); + break; + } + } + snprintf(clfile, PATH_MAX, "%s", tmpfile); + } + + FILE* fp = fopen(clfile, "r"); + if(fp == NULL){ + return cl; + } + + while(!feof(fp)) { + fgets(line, (int)PATH_MAX, fp); + cl = alpm_list_add(cl, strdup(line)); + } + fclose(fp); + + if(pkg->origin == PKG_FROM_FILE) { + unlink(tmpfile); + FREE(tmpfile); + close(fd); + }
One small suggestion, has anyone worked out how to read a file directly from an archive using libarchive rather than extracting to a temp file then reading the temp file? It would be useful here and where we read .PKGINFO Andrew
On Dec 7, 2007 12:20 PM, Andrew Fyfe <andrew@neptune-one.net> wrote:
One small suggestion, has anyone worked out how to read a file directly from an archive using libarchive rather than extracting to a temp file then reading the temp file? It would be useful here and where we read .PKGINFO
Yeah but we'd have to return an fd, which is fine, but adds some extra complexity we may not want.
On Fri, Dec 07, 2007 at 12:28:25PM -0600, Aaron Griffin wrote:
On Dec 7, 2007 12:20 PM, Andrew Fyfe <andrew@neptune-one.net> wrote:
One small suggestion, has anyone worked out how to read a file directly from an archive using libarchive rather than extracting to a temp file then reading the temp file? It would be useful here and where we read .PKGINFO
Yeah but we'd have to return an fd, which is fine, but adds some extra complexity we may not want.
I was thinking exactly the same as Andrew, why not just using ssize_t archive_read_data(struct archive *, void *buff, size_t len); I don't understand what you are saying however, about returning a fd.
On Dec 7, 2007 12:35 PM, Xavier <shiningxc@gmail.com> wrote:
On Fri, Dec 07, 2007 at 12:28:25PM -0600, Aaron Griffin wrote:
On Dec 7, 2007 12:20 PM, Andrew Fyfe <andrew@neptune-one.net> wrote:
One small suggestion, has anyone worked out how to read a file directly from an archive using libarchive rather than extracting to a temp file then reading the temp file? It would be useful here and where we read .PKGINFO
Yeah but we'd have to return an fd, which is fine, but adds some extra complexity we may not want.
I was thinking exactly the same as Andrew, why not just using ssize_t archive_read_data(struct archive *, void *buff, size_t len);
I don't understand what you are saying however, about returning a fd.
I'll take a look at this tonight if I have time to work out an acceptable set of changes if possible. -Dan
On Dec 7, 2007 12:35 PM, Xavier <shiningxc@gmail.com> wrote:
On Fri, Dec 07, 2007 at 12:28:25PM -0600, Aaron Griffin wrote:
On Dec 7, 2007 12:20 PM, Andrew Fyfe <andrew@neptune-one.net> wrote:
One small suggestion, has anyone worked out how to read a file directly from an archive using libarchive rather than extracting to a temp file then reading the temp file? It would be useful here and where we read .PKGINFO
Yeah but we'd have to return an fd, which is fine, but adds some extra complexity we may not want.
I was thinking exactly the same as Andrew, why not just using ssize_t archive_read_data(struct archive *, void *buff, size_t len);
I don't understand what you are saying however, about returning a fd.
Oh man, ignore what I said - I was overthinking that to the parsing routines and all that jazz - in this case we're returning the changelog as an alpm_list_t of strings. Yes, in this case it would definitely be better to read direct from the archive. HOWEVER, you can't read line by line... what you need to do is make some sort of "archive_readline" function that reads one char at a time until a newline char. It's annoying, but that's the best we can do until libarchive gets a "readline" function.
On Fri, Dec 07, 2007 at 12:54:36PM -0600, Aaron Griffin wrote:
I was thinking exactly the same as Andrew, why not just using ssize_t archive_read_data(struct archive *, void *buff, size_t len);
I don't understand what you are saying however, about returning a fd.
Oh man, ignore what I said - I was overthinking that to the parsing routines and all that jazz - in this case we're returning the changelog as an alpm_list_t of strings. Yes, in this case it would definitely be better to read direct from the archive. HOWEVER, you can't read line by line... what you need to do is make some sort of "archive_readline" function that reads one char at a time until a newline char. It's annoying, but that's the best we can do until libarchive gets a "readline" function.
Oh ok, yes it looks like you are right. However, there is a point that is unclear to me : why exactly do we need to read line by line?
On Dec 7, 2007 5:35 PM, Xavier <shiningxc@gmail.com> wrote:
On Fri, Dec 07, 2007 at 12:54:36PM -0600, Aaron Griffin wrote:
I was thinking exactly the same as Andrew, why not just using ssize_t archive_read_data(struct archive *, void *buff, size_t len);
I don't understand what you are saying however, about returning a fd.
Oh man, ignore what I said - I was overthinking that to the parsing routines and all that jazz - in this case we're returning the changelog as an alpm_list_t of strings. Yes, in this case it would definitely be better to read direct from the archive. HOWEVER, you can't read line by line... what you need to do is make some sort of "archive_readline" function that reads one char at a time until a newline char. It's annoying, but that's the best we can do until libarchive gets a "readline" function.
Oh ok, yes it looks like you are right. However, there is a point that is unclear to me : why exactly do we need to read line by line?
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
Aaron Griffin wrote:
On Dec 7, 2007 5:35 PM, Xavier <shiningxc@gmail.com> wrote:
On Fri, Dec 07, 2007 at 12:54:36PM -0600, Aaron Griffin wrote:
I was thinking exactly the same as Andrew, why not just using ssize_t archive_read_data(struct archive *, void *buff, size_t len);
I don't understand what you are saying however, about returning a fd.
Oh man, ignore what I said - I was overthinking that to the parsing routines and all that jazz - in this case we're returning the changelog as an alpm_list_t of strings. Yes, in this case it would definitely be better to read direct from the archive. HOWEVER, you can't read line by line... what you need to do is make some sort of "archive_readline" function that reads one char at a time until a newline char. It's annoying, but that's the best we can do until libarchive gets a "readline" function.
Oh ok, yes it looks like you are right. However, there is a point that is unclear to me : why exactly do we need to read line by line?
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
I initially tried something like this, using archive_read_data to get the data, given it is suggested where pkginfo is read. However, I was finding it it hard to produce output in a consistent format with the PKG_FROM_CACHE case in order to pass to dump_pkg_chagnelog. This was likely to be more to do with my lack of experience with C "strings" rather than it not being possible... Allan
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 ? :
From 2be1e8094574a4071f14df9d6347c65c5422776d Mon Sep 17 00:00:00 2001 From: Chantry Xavier <shiningxc@gmail.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. Original patch from Allan McRae, I changed get_changelog to read directly from the archive instead of extracting to a temp file, and this function now returns a big string, instead of a list of lines. Signed-off-by: Chantry Xavier <shiningxc@gmail.com> --- lib/libalpm/alpm.h | 1 + lib/libalpm/package.c | 80 +++++++++++++++++++++++++++++++++++++++++++++++++ src/pacman/package.c | 28 ++++++----------- src/pacman/package.h | 2 +- src/pacman/query.c | 9 +----- 5 files changed, 93 insertions(+), 27 deletions(-) diff --git a/lib/libalpm/alpm.h b/lib/libalpm/alpm.h index 1e18ad9..c931b07 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); +char *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..3d7e74d 100644 --- a/lib/libalpm/package.c +++ b/lib/libalpm/package.c @@ -491,6 +491,86 @@ alpm_list_t SYMEXPORT *alpm_pkg_get_backup(pmpkg_t *pkg) return pkg->backup; } +char SYMEXPORT *alpm_pkg_get_changelog(pmpkg_t *pkg) +{ + ALPM_LOG_FUNC; + + /* Sanity checks */ + ASSERT(handle != NULL, return(NULL)); + ASSERT(pkg != NULL, return(NULL)); + + int ret = ARCHIVE_OK; + char clfile[PATH_MAX]; + FILE *fp = NULL; + struct archive *archive = NULL; + struct archive_entry *entry; + char *changelog = NULL, *p = NULL; + int bufsize = 512, size = 0, n = 0; + int done = 0, found = 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)); + fp = fopen(clfile, "r"); + if(fp != NULL) { + found = 1; + } + } else if(pkg->origin == PKG_FROM_FILE) { + const char *pkgfile = pkg->origin_data.file; + + if((archive = archive_read_new()) == NULL) { + RET_ERR(PM_ERR_LIBARCHIVE_ERROR, NULL); + } + + archive_read_support_compression_all(archive); + archive_read_support_format_all(archive); + + if (archive_read_open_filename(archive, pkgfile, + ARCHIVE_DEFAULT_BYTES_PER_BLOCK) != ARCHIVE_OK) { + RET_ERR(PM_ERR_PKG_OPEN, NULL); + } + + while((ret = archive_read_next_header(archive, &entry)) == ARCHIVE_OK) { + const char *entry_name = archive_entry_pathname(entry); + + if(strcmp(entry_name, ".CHANGELOG") == 0) { + found = 1; + break; + } + } + } + + if(!found) { + return(NULL); + } + + 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) = '\0'; + } + } + + if(pkg->origin == PKG_FROM_CACHE) { + fclose(fp); + } else if(pkg->origin == PKG_FROM_FILE) { + archive_read_finish(archive); + } + + return(changelog); +} + 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..fcade3b 100644 --- a/src/pacman/package.c +++ b/src/pacman/package.c @@ -232,29 +232,21 @@ 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]; + char *changelog = alpm_pkg_get_changelog(pkg); - if((fp = fopen(clfile, "r")) == NULL) - { - fprintf(stderr, _("error: no changelog available for '%s'.\n"), pkgname); - return; - } - else - { - while(!feof(fp)) - { - fgets(line, (int)PATH_MAX, fp); - printf("%s", line); - line[0] = '\0'; - } - fclose(fp); + if(changelog == NULL) { + fprintf(stderr, _("error: no changelog available for '%s'.\n"), + alpm_pkg_get_name(pkg)); return; + } else { + fprintf(stdout, "%s", changelog); } + + free(changelog); } /* vim: set ts=2 sw=2 noet: */ 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
Xavier wrote:
diff --git a/lib/libalpm/package.c b/lib/libalpm/package.c index 172456d..3d7e74d 100644 --- a/lib/libalpm/package.c +++ b/lib/libalpm/package.c @@ -491,6 +491,86 @@ alpm_list_t SYMEXPORT *alpm_pkg_get_backup(pmpkg_t *pkg) return pkg->backup; }
+char SYMEXPORT *alpm_pkg_get_changelog(pmpkg_t *pkg) +{ + ALPM_LOG_FUNC; + + /* Sanity checks */ + ASSERT(handle != NULL, return(NULL)); + ASSERT(pkg != NULL, return(NULL)); + + int ret = ARCHIVE_OK; + char clfile[PATH_MAX]; + FILE *fp = NULL; + struct archive *archive = NULL; + struct archive_entry *entry; + char *changelog = NULL, *p = NULL; + int bufsize = 512, size = 0, n = 0; + int done = 0, found = 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)); + fp = fopen(clfile, "r"); + if(fp != NULL) { + found = 1; + } + } else if(pkg->origin == PKG_FROM_FILE) { + const char *pkgfile = pkg->origin_data.file; + + if((archive = archive_read_new()) == NULL) { + RET_ERR(PM_ERR_LIBARCHIVE_ERROR, NULL); + } + + archive_read_support_compression_all(archive); + archive_read_support_format_all(archive); + + if (archive_read_open_filename(archive, pkgfile, + ARCHIVE_DEFAULT_BYTES_PER_BLOCK) != ARCHIVE_OK) { + RET_ERR(PM_ERR_PKG_OPEN, NULL); + } + + while((ret = archive_read_next_header(archive, &entry)) == ARCHIVE_OK) { + const char *entry_name = archive_entry_pathname(entry); + + if(strcmp(entry_name, ".CHANGELOG") == 0) { + found = 1; + break; + } + } + } + + if(!found) { + return(NULL);
You're missing an archive_read_finish(archive) before the return. Andrew
On Sat, Dec 08, 2007 at 07:28:05PM +0000, Andrew Fyfe wrote:
You're missing an archive_read_finish(archive) before the return.
Good catch, I fixed this locally. I'm still not sure what is better between the tempfile or this method. And I would also like to try the db_read way.
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) = '\0'; + } + } + <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
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) = '\0'; + } + } + <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
Thanks to Allan for inspiring all this work on what was one little TODO item in the codebase. :) Change changelog handling so we can now dump a changelog from both installed packages and package files (fixes FS#7371). We do this by moving all of the machinery to the backend where it should have been in the first place. The changelog reading is now done through a open/read/close interface similar to the fopen/fread/fclose functions (can you guess how it is done?). It is buffered by the frontend, so programs using the library can read as much or as little as they want at a time. Unfortunately, I could not implement a changelog_feof function due to some shortcomings of libarchive. However, I left the stub code in there, commented out, in case it becomes possible later or anyone wants to take a stab at it. Original-work-by: Allan McRae <mcrae_allan@hotmail.com> Improved-by: Chantry Xavier <shiningxc@gmail.com> Signed-off-by: Dan McGee <dan@archlinux.org> --- lib/libalpm/alpm.h | 5 ++ lib/libalpm/package.c | 109 +++++++++++++++++++++++++++++++++++++++++++++++++ src/pacman/package.c | 38 +++++++++-------- src/pacman/package.h | 2 +- src/pacman/query.c | 9 +---- 5 files changed, 137 insertions(+), 26 deletions(-) diff --git a/lib/libalpm/alpm.h b/lib/libalpm/alpm.h index 1e18ad9..c98adc5 100644 --- a/lib/libalpm/alpm.h +++ b/lib/libalpm/alpm.h @@ -216,6 +216,11 @@ 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); +void *alpm_pkg_changelog_open(pmpkg_t *pkg); +size_t alpm_pkg_changelog_read(void *ptr, size_t size, + const pmpkg_t *pkg, const void *fp); +/*int alpm_pkg_changelog_feof(const pmpkg_t *pkg, void *fp);*/ +int alpm_pkg_changelog_close(const pmpkg_t *pkg, void *fp); 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..efb54b0 100644 --- a/lib/libalpm/package.c +++ b/lib/libalpm/package.c @@ -491,6 +491,115 @@ alpm_list_t SYMEXPORT *alpm_pkg_get_backup(pmpkg_t *pkg) return pkg->backup; } +/** + * Open a package changelog for reading. Similar to fopen in functionality, + * except that the returned 'file stream' could really be from an archive + * as well as from the database. + * @param pkg the package to read the changelog of (either file or db) + * @return a 'file stream' to the package changelog + */ +void SYMEXPORT *alpm_pkg_changelog_open(pmpkg_t *pkg) +{ + ALPM_LOG_FUNC; + + /* Sanity checks */ + ASSERT(handle != NULL, return(NULL)); + ASSERT(pkg != NULL, return(NULL)); + + if(pkg->origin == PKG_FROM_CACHE) { + char clfile[PATH_MAX]; + 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)); + return fopen(clfile, "r"); + } else if(pkg->origin == PKG_FROM_FILE) { + struct archive *archive = NULL; + struct archive_entry *entry; + const char *pkgfile = pkg->origin_data.file; + int ret = ARCHIVE_OK; + + if((archive = archive_read_new()) == NULL) { + RET_ERR(PM_ERR_LIBARCHIVE_ERROR, NULL); + } + + archive_read_support_compression_all(archive); + archive_read_support_format_all(archive); + + if (archive_read_open_filename(archive, pkgfile, + ARCHIVE_DEFAULT_BYTES_PER_BLOCK) != ARCHIVE_OK) { + RET_ERR(PM_ERR_PKG_OPEN, NULL); + } + + while((ret = archive_read_next_header(archive, &entry)) == ARCHIVE_OK) { + const char *entry_name = archive_entry_pathname(entry); + + if(strcmp(entry_name, ".CHANGELOG") == 0) { + return(archive); + } + } + /* we didn't find a changelog */ + archive_read_finish(archive); + errno = ENOENT; + } + return(NULL); +} + +/** + * Read data from an open changelog 'file stream'. Similar to fread in + * functionality, this function takes a buffer and amount of data to read. + * @param ptr a buffer to fill with raw changelog data + * @param size the size of the buffer + * @param pkg the package that the changelog is being read from + * @param fp a 'file stream' to the package changelog + * @return the number of characters read, or 0 if there is no more data + */ +size_t SYMEXPORT alpm_pkg_changelog_read(void *ptr, size_t size, + const pmpkg_t *pkg, const void *fp) +{ + size_t ret = 0; + if(pkg->origin == PKG_FROM_CACHE) { + ret = fread(ptr, 1, size, (FILE*)fp); + } else if(pkg->origin == PKG_FROM_FILE) { + ret = archive_read_data((struct archive*)fp, ptr, size); + } + return(ret); +} + +/* +int SYMEXPORT alpm_pkg_changelog_feof(const pmpkg_t *pkg, void *fp) +{ + int ret = 0; + if(pkg->origin == PKG_FROM_CACHE) { + ret = feof((FILE*)fp); + } else if(pkg->origin == PKG_FROM_FILE) { + // note: this doesn't quite work, no feof in libarchive + ret = archive_read_data((struct archive*)fp, NULL, 0); + } + return(ret); +} +*/ + +/** + * Close a package changelog for reading. Similar to fclose in functionality, + * except that the 'file stream' could really be from an archive as well as + * from the database. + * @param pkg the package that the changelog was read from + * @param fp a 'file stream' to the package changelog + * @return whether closing the package changelog stream was successful + */ +int SYMEXPORT alpm_pkg_changelog_close(const pmpkg_t *pkg, void *fp) +{ + int ret = 0; + if(pkg->origin == PKG_FROM_CACHE) { + ret = fclose((FILE*)fp); + } else if(pkg->origin == PKG_FROM_FILE) { + ret = archive_read_finish((struct archive *)fp); + } + return(ret); +} + 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..ba57214 100644 --- a/src/pacman/package.c +++ b/src/pacman/package.c @@ -34,6 +34,8 @@ #include "package.h" #include "util.h" +#define CLBUF_SIZE 4096 + /* Display the content of a package * * level: <0 - sync package [-Si] @@ -232,28 +234,30 @@ 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]; + void *fp = NULL; - if((fp = fopen(clfile, "r")) == NULL) - { - fprintf(stderr, _("error: no changelog available for '%s'.\n"), pkgname); + if((fp = alpm_pkg_changelog_open(pkg)) == NULL) { + /* TODO after string freeze use pm_fprintf */ + fprintf(stderr, _("error: no changelog available for '%s'.\n"), + alpm_pkg_get_name(pkg)); return; - } - else - { - while(!feof(fp)) - { - fgets(line, (int)PATH_MAX, fp); - printf("%s", line); - line[0] = '\0'; + } else { + /* allocate a buffer to get the changelog back in chunks */ + char buf[CLBUF_SIZE]; + int ret = 0; + while((ret = alpm_pkg_changelog_read(buf, CLBUF_SIZE, pkg, fp))) { + if(ret < CLBUF_SIZE) { + /* if we hit the end of the file, we need to add a null terminator */ + *(buf + ret) = '\0'; + } + printf("%s", buf); } - fclose(fp); - return; + alpm_pkg_changelog_close(pkg, fp); + printf("\n"); } } 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
On Sat, Dec 08, 2007 at 11:43:41PM -0600, Dan McGee wrote:
> Thanks to Allan for inspiring all this work on what was one little TODO item
> in the codebase. :)
>
> Change changelog handling so we can now dump a changelog from both installed
> packages and package files (fixes FS#7371). We do this by moving all of the
> machinery to the backend where it should have been in the first place.
>
> The changelog reading is now done through a open/read/close interface
> similar to the fopen/fread/fclose functions (can you guess how it is done?).
> It is buffered by the frontend, so programs using the library can read as
> much or as little as they want at a time.
>
That's an interesting way, it gives more control to the frontend, and avoids
this memory problem.
> @@ -232,28 +234,30 @@ 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];
> + void *fp = NULL;
>
> - if((fp = fopen(clfile, "r")) == NULL)
> - {
> - fprintf(stderr, _("error: no changelog available for '%s'.\n"), pkgname);
> + if((fp = alpm_pkg_changelog_open(pkg)) == NULL) {
> + /* TODO after string freeze use pm_fprintf */
> + fprintf(stderr, _("error: no changelog available for '%s'.\n"),
> + alpm_pkg_get_name(pkg));
> return;
> - }
> - else
> - {
> - while(!feof(fp))
> - {
> - fgets(line, (int)PATH_MAX, fp);
> - printf("%s", line);
> - line[0] = '\0';
> + } else {
> + /* allocate a buffer to get the changelog back in chunks */
> + char buf[CLBUF_SIZE];
> + int ret = 0;
> + while((ret = alpm_pkg_changelog_read(buf, CLBUF_SIZE, pkg, fp))) {
> + if(ret < CLBUF_SIZE) {
> + /* if we hit the end of the file, we need to add a null terminator */
> + *(buf + ret) = '\0';
> + }
> + printf("%s", buf);
> }
> - fclose(fp);
> - return;
> + alpm_pkg_changelog_close(pkg, fp);
> + printf("\n");
> }
> }
>
I see you added a \n there, this wasn't done previously.
I had a look at the changelogs available in my local db, to see how they
ended :
> find /var/lib/pacman/local -name changelog | xargs tail -n1
==> /var/lib/pacman/local/openoffice-base-2.3.1-1/changelog <==
- added ChangeLog
==> /var/lib/pacman/local/rtorrent-0.7.9-1/changelog <==
* Print to the log when close_on_diskspace gets triggered.
==> /var/lib/pacman/local/cmus-2.2.0-3/changelog <==
Added flac and libmad
==> /var/lib/pacman/local/docbook-xml-4.5-1/changelog <==
==> /var/lib/pacman/local/powertop-1.9-1/changelog <==
==> /var/lib/pacman/local/fdm-1.4-1/changelog <==
==> /var/lib/pacman/local/glibc-2.7-7/changelog <==
- added ChangeLog%
So openoffice-base and glibc don't even end with a newline.
cmus and rtorrent end with just one (though that rtorrent package is a custom
one).
docbook-xml, powertop and fdm already have two newlines (so one
empty line).
So in the case of openoffice-base and glibc, adding an extra newline at the
end is necessary. But in the case of docbook-xml, powertop and fdm. it'll
result in several empty lines which isn't very nice either.
Maybe instead of adding an extra newline, we could maybe put a simple
guideline for changelog somewhere, stating that they should end with just one
newline. Then maybe makepkg could correct these automatically. Or it could
be added as a simple check to namcap, if that makes sense and is possible.
Anyway, this is a lot of noise about a really minor and unimportant issue, so
I apologize if you think this is not worth worrying about.
On Dec 9, 2007 7:11 AM, Xavier <shiningxc@gmail.com> wrote:
> On Sat, Dec 08, 2007 at 11:43:41PM -0600, Dan McGee wrote:
> > Thanks to Allan for inspiring all this work on what was one little TODO item
> > in the codebase. :)
> >
> > Change changelog handling so we can now dump a changelog from both installed
> > packages and package files (fixes FS#7371). We do this by moving all of the
> > machinery to the backend where it should have been in the first place.
> >
> > The changelog reading is now done through a open/read/close interface
> > similar to the fopen/fread/fclose functions (can you guess how it is done?).
> > It is buffered by the frontend, so programs using the library can read as
> > much or as little as they want at a time.
> >
>
> That's an interesting way, it gives more control to the frontend, and avoids
> this memory problem.
>
>
> > @@ -232,28 +234,30 @@ 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];
> > + void *fp = NULL;
> >
> > - if((fp = fopen(clfile, "r")) == NULL)
> > - {
> > - fprintf(stderr, _("error: no changelog available for '%s'.\n"), pkgname);
> > + if((fp = alpm_pkg_changelog_open(pkg)) == NULL) {
> > + /* TODO after string freeze use pm_fprintf */
> > + fprintf(stderr, _("error: no changelog available for '%s'.\n"),
> > + alpm_pkg_get_name(pkg));
> > return;
> > - }
> > - else
> > - {
> > - while(!feof(fp))
> > - {
> > - fgets(line, (int)PATH_MAX, fp);
> > - printf("%s", line);
> > - line[0] = '\0';
> > + } else {
> > + /* allocate a buffer to get the changelog back in chunks */
> > + char buf[CLBUF_SIZE];
> > + int ret = 0;
> > + while((ret = alpm_pkg_changelog_read(buf, CLBUF_SIZE, pkg, fp))) {
> > + if(ret < CLBUF_SIZE) {
> > + /* if we hit the end of the file, we need to add a null terminator */
> > + *(buf + ret) = '\0';
> > + }
> > + printf("%s", buf);
> > }
> > - fclose(fp);
> > - return;
> > + alpm_pkg_changelog_close(pkg, fp);
> > + printf("\n");
> > }
> > }
> >
>
> I see you added a \n there, this wasn't done previously.
> I had a look at the changelogs available in my local db, to see how they
> ended :
> > find /var/lib/pacman/local -name changelog | xargs tail -n1
> ==> /var/lib/pacman/local/openoffice-base-2.3.1-1/changelog <==
> - added ChangeLog
> ==> /var/lib/pacman/local/rtorrent-0.7.9-1/changelog <==
> * Print to the log when close_on_diskspace gets triggered.
>
> ==> /var/lib/pacman/local/cmus-2.2.0-3/changelog <==
> Added flac and libmad
>
> ==> /var/lib/pacman/local/docbook-xml-4.5-1/changelog <==
>
>
> ==> /var/lib/pacman/local/powertop-1.9-1/changelog <==
>
>
> ==> /var/lib/pacman/local/fdm-1.4-1/changelog <==
>
>
> ==> /var/lib/pacman/local/glibc-2.7-7/changelog <==
> - added ChangeLog%
>
> So openoffice-base and glibc don't even end with a newline.
> cmus and rtorrent end with just one (though that rtorrent package is a custom
> one).
> docbook-xml, powertop and fdm already have two newlines (so one
> empty line).
>
> So in the case of openoffice-base and glibc, adding an extra newline at the
> end is necessary. But in the case of docbook-xml, powertop and fdm. it'll
> result in several empty lines which isn't very nice either.
>
> Maybe instead of adding an extra newline, we could maybe put a simple
> guideline for changelog somewhere, stating that they should end with just one
> newline. Then maybe makepkg could correct these automatically. Or it could
> be added as a simple check to namcap, if that makes sense and is possible.
>
> Anyway, this is a lot of noise about a really minor and unimportant issue, so
> I apologize if you think this is not worth worrying about.
Want to make a ChangeLog.proto to stick in contrib/ ? You could make a
few sample changelog entries and make one of them state that the file
should end with one blank line.
I tested with the glibc package (both package and installed in my
local db) thus the reason I added the newline. :)
-Dan
On Sun, Dec 09, 2007 at 11:15:54AM -0600, Dan McGee wrote:
Want to make a ChangeLog.proto to stick in contrib/ ? You could make a few sample changelog entries and make one of them state that the file should end with one blank line.
I tested with the glibc package (both package and installed in my local db) thus the reason I added the newline. :)
I'm not sure how it should be formatted. I didn't even know there was a changelog format. At least, there is one described in the gnu emacs manual : http://www.gnu.org/software/emacs/manual/html_node/emacs/Format-of-ChangeLog... By following this style, I got nice colors in both vim and emacs ;) I also saw Aaron followed a similar style for cmus changelog: --------------------------------------------------- 2007-10-29 Aaron Griffin <aaronmgriffin@gmail.com> * 2.2.0-3 Removed arts support (FS#8341) Added flac and libmad --------------------------------------------------- Though it's missing a space or tab for the entry, and that seems to be important, both in the emacs manual, and for vim/emacs syntax colors. So here is my proposal for ChangeLog.proto : --------------------------------------------------- 2007-12-01 Your Name <youremail@domain.com> * 1.1-1 new upstream release. change 1. change 2. 2007-11-01 Your Name <youremail@domain.com> * 1.0-5 added ChangeLog. the last line should end with just one newline. you can cat the file to check it displays fine. --------------------------------------------------- The name and email for each new release might look a bit repetitive, since they will likely often be the same. But well, finally, I don't think it's a big problem, it does happen that a package is edited by several devs. And it can also change maintainers. And since this is apparently some sort of standard, it's probably not a bad idea to follow it, at least for the prototype. People are then free to do what they want.
On Dec 9, 2007 1:36 PM, Xavier <shiningxc@gmail.com> wrote:
On Sun, Dec 09, 2007 at 11:15:54AM -0600, Dan McGee wrote:
Want to make a ChangeLog.proto to stick in contrib/ ? You could make a few sample changelog entries and make one of them state that the file should end with one blank line.
I tested with the glibc package (both package and installed in my local db) thus the reason I added the newline. :)
I'm not sure how it should be formatted. I didn't even know there was a changelog format. At least, there is one described in the gnu emacs manual : http://www.gnu.org/software/emacs/manual/html_node/emacs/Format-of-ChangeLog...
By following this style, I got nice colors in both vim and emacs ;) I also saw Aaron followed a similar style for cmus changelog:
--------------------------------------------------- 2007-10-29 Aaron Griffin <aaronmgriffin@gmail.com> * 2.2.0-3 Removed arts support (FS#8341) Added flac and libmad ---------------------------------------------------
Though it's missing a space or tab for the entry, and that seems to be important, both in the emacs manual, and for vim/emacs syntax colors. So here is my proposal for ChangeLog.proto :
--------------------------------------------------- 2007-12-01 Your Name <youremail@domain.com>
* 1.1-1 new upstream release. change 1. change 2.
2007-11-01 Your Name <youremail@domain.com>
* 1.0-5 added ChangeLog. the last line should end with just one newline. you can cat the file to check it displays fine. ---------------------------------------------------
The name and email for each new release might look a bit repetitive, since they will likely often be the same. But well, finally, I don't think it's a big problem, it does happen that a package is edited by several devs. And it can also change maintainers. And since this is apparently some sort of standard, it's probably not a bad idea to follow it, at least for the prototype. People are then free to do what they want.
I'm +1 on the standard format and making a ChangeLog.proto, as we discussed this on IRC already. However, I'd like to see what other people think too. -Dan
Xavier wrote:
On Sun, Dec 09, 2007 at 11:15:54AM -0600, Dan McGee wrote:
Want to make a ChangeLog.proto to stick in contrib/ ? You could make a few sample changelog entries and make one of them state that the file should end with one blank line.
I tested with the glibc package (both package and installed in my local db) thus the reason I added the newline. :)
I'm not sure how it should be formatted. I didn't even know there was a changelog format. At least, there is one described in the gnu emacs manual : http://www.gnu.org/software/emacs/manual/html_node/emacs/Format-of-ChangeLog...
By following this style, I got nice colors in both vim and emacs ;) I also saw Aaron followed a similar style for cmus changelog:
--------------------------------------------------- 2007-10-29 Aaron Griffin <aaronmgriffin@gmail.com> * 2.2.0-3 Removed arts support (FS#8341) Added flac and libmad ---------------------------------------------------
Though it's missing a space or tab for the entry, and that seems to be important, both in the emacs manual, and for vim/emacs syntax colors. So here is my proposal for ChangeLog.proto :
--------------------------------------------------- 2007-12-01 Your Name <youremail@domain.com>
* 1.1-1 new upstream release. change 1. change 2.
2007-11-01 Your Name <youremail@domain.com>
* 1.0-5 added ChangeLog. the last line should end with just one newline. you can cat the file to check it displays fine. ---------------------------------------------------
The name and email for each new release might look a bit repetitive, since they will likely often be the same. But well, finally, I don't think it's a big problem, it does happen that a package is edited by several devs. And it can also change maintainers. And since this is apparently some sort of standard, it's probably not a bad idea to follow it, at least for the prototype. People are then free to do what they want.
This has also been discussed in http://bugs.archlinux.org/task/7231
On Sat, Dec 08, 2007 at 12:35:15AM +0100, Xavier <shiningxc@gmail.com> wrote:
Oh ok, yes it looks like you are right. However, there is a point that is unclear to me : why exactly do we need to read line by line?
it's just the way i did it. of course reading it at once it also correct. - VMiklos
participants (9)
-
Aaron Griffin
-
Allan McRae
-
Allan McRae
-
Andrew Fyfe
-
Dan McGee
-
Dan McGee
-
Miklos Vajna
-
Nagy Gabor
-
Xavier