- Pacman-dev - lists.archlinux.org

Re: [pacman-dev] patch submission help
by Jeff 18 Mar '09

18 Mar '09

On Tue, Mar 17, 2009 at 11:17:20AM +1000, Allan McRae wrote: > > Note that with the changes that are now in the pacmans git repo, there > is support for build() and package() functions in PKGBUILDs. When the > package() function is present, it is run under fakeroot but the build() > function is not. Otherwise it is backwards compatible. This can be dangerous, unless the pacman devs personally undertake the migration of all extant PKGBUILDs. Since I just realized the problem yesterday, I have not had time to think about the best fix, but I think the fix should be rather trivial at least in reference to my patch. I'd really like to see your branch and I could send you my patch (onlist/offlist your choice) as a matter of reference if you would like my input. > I like the idea of an additional check() function. However, I could > implement it slightly differently - instead of a makepkg.conf flag to > run the check() function or not, I think it may be better to add a > --check flag to makepkg so people specify when to run it. >From a technical standpoint, I prefer the --check arg better than putting an option override in a PKGBUILD, but then I also like the option override as a "hint" to the individual who is building packageX that the testsuite _should_ be run. I'll keep using my modified pacman-3.2.2 until I finish the current rebuild, but after that I'd be happy to test the changes. I have no interest in maintaining my own set of patches. :) As a side note, I've modified dozens of PKGBUILDs to implement this change, and I'm rather clueless as to why all the || return 1 statements exist since set -e is in use. Are they cruft left behind from an earlier version of makepkg that didn't bail on the first error? Also, my patch actually just runs run_build() with an argument (the arg being the name of the PKGBUILD function to run) so there was no code duplication. In run_build() I put in a simple: cd "${srcdir}/${pkgname}-${pkgver}" || cd "${srcdir}" as it seemed silly to have a cd command in every single PKGBUILD when a good number of tarballs follow the ${pkgname}-${pkgver} naming scheme. I don't know if you have added that feature or not. Especially with more than one function in PKGBUILD, cd would have to be duplicated (and still will for non-standard source tarballs). A function for determining that name might be useful, though, and would help in all cases except where the build dir != source dir (like glibc). Anyway, just food for thought! -- Jeff My other computer is an abacus.

2 1

[pacman-dev] [PATCH] Refactor testing for color message output
by Allan McRae 15 Mar '09

15 Mar '09

Signed-off-by: Allan McRae <allan(a)archlinux.org> --- scripts/makepkg.sh.in | 16 +++++++++++----- 1 files changed, 11 insertions(+), 5 deletions(-) diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 3ff256f..d44fe0d 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -69,6 +69,7 @@ IGNOREARCH=0 HOLDVER=0 PKGFUNC=0 SPLITPKG=0 +COLORMSG=0 # Forces the pkgver of the current PKGBUILD. Used by the fakeroot call # when dealing with svn/cvs/etc PKGBUILDs. @@ -80,7 +81,7 @@ PACMAN_OPTS= plain() { local mesg=$1; shift - if [ -t 2 -a ! "$USE_COLOR" = "n" -a "$(check_buildenv color)" = "y" ]; then + if [ COLORMSG -eq 1 ]; then printf "\033[1;1m ${mesg}\033[1;0m\n" "$@" >&2 else printf " ${mesg}\n" "$@" >&2 @@ -89,7 +90,7 @@ plain() { msg() { local mesg=$1; shift - if [ -t 2 -a ! "$USE_COLOR" = "n" -a "$(check_buildenv color)" = "y" ]; then + if [ COLORMSG -eq 1 ]; then printf "\033[1;32m==>\033[1;0m\033[1;1m ${mesg}\033[1;0m\n" "$@" >&2 else printf "==> ${mesg}\n" "$@" >&2 @@ -98,7 +99,7 @@ msg() { msg2() { local mesg=$1; shift - if [ -t 2 -a ! "$USE_COLOR" = "n" -a "$(check_buildenv color)" = "y" ]; then + if [ COLORMSG -eq 1 ]; then printf "\033[1;34m ->\033[1;0m\033[1;1m ${mesg}\033[1;0m\n" "$@" >&2 else printf " -> ${mesg}\n" "$@" >&2 @@ -107,7 +108,7 @@ msg2() { warning() { local mesg=$1; shift - if [ -t 2 -a ! "$USE_COLOR" = "n" -a "$(check_buildenv color)" = "y" ]; then + if [ COLORMSG -eq 1 ]; then printf "\033[1;33m==> $(gettext "WARNING:")\033[1;0m\033[1;1m ${mesg}\033[1;0m\n" "$@" >&2 else printf "==> $(gettext "WARNING:") ${mesg}\n" "$@" >&2 @@ -116,7 +117,7 @@ warning() { error() { local mesg=$1; shift - if [ -t 2 -a ! "$USE_COLOR" = "n" -a "$(check_buildenv color)" = "y" ]; then + if [ COLORMSG -eq 1 ]; then printf "\033[1;31m==> $(gettext "ERROR:")\033[1;0m\033[1;1m ${mesg}\033[1;0m\n" "$@" >&2 else printf "==> $(gettext "ERROR:") ${mesg}\n" "$@" >&2 @@ -1303,6 +1304,11 @@ if [ ! $(type -t gettext) ]; then } fi +# check if messages are to be printed using color +if [ -t 2 -a ! "$USE_COLOR" = "n" -a "$(check_buildenv color)" = "y" ]; then + COLORMSG=1 +fi + ARGLIST=$@ # Parse Command Line Options. -- 1.6.2

2 3

[pacman-dev] [GIT] The official pacman repository branch, master, updated. v3.2.2-104-g9519d22
by dan＠archlinux.org 15 Mar '09

15 Mar '09

This is an automated email from the git hooks/post-receive script. It was generated because a ref change was pushed to the repository containing the project "The official pacman repository". The branch, master has been updated via 9519d22df7cef5b5a48a7d1ebee44a9e935e02b7 (commit) via de97282fbd78755cde71c1d064ec00b806efcca5 (commit) via 36863b968f562840461e60be4ed23abf635d6008 (commit) via 687f04ab4699440e3d8b371454b26039b6eb2772 (commit) via a864a50bc6a65a5c39a4727a07a3b269049a808e (commit) from d8d9ab8c870eb9f2df5e6e607b1da34e6e40e29e (commit) Those revisions listed above that are new to this repository have not appeared on any other notification email; so we list those revisions in full, below. - Log ----------------------------------------------------------------- commit 9519d22df7cef5b5a48a7d1ebee44a9e935e02b7 Author: Xavier Chantry <shiningxc(a)gmail.com> Date: Sun Mar 1 01:40:24 2009 +0100 be_files.c : use %jd instead of %ju for intmax_t Signed-off-by: Xavier Chantry <shiningxc(a)gmail.com> Signed-off-by: Dan McGee <dan(a)archlinux.org> commit de97282fbd78755cde71c1d064ec00b806efcca5 Author: Xavier Chantry <shiningxc(a)gmail.com> Date: Sun Mar 15 12:43:39 2009 +0100 Revert "popen does NOT require /bin/sh in a subchroot" This reverts commit 9558639d8009483fbf422b138d020745986f82f1. This change was wrong, popen does require /bin/sh in a subchroot. 1) pacman -S lilo -r root Notice no error 2) rm root/bin/sh ; pacman -S lilo -r root Notice an error : error: scriptlet failed to execute correctly Actually, we already get an explicit error here, when popen is run, so there is no need to check for bin/sh explicitely. Besides this check was problematic in some cases. For example, bash itself has a scriptlet, but only post_install and post_upgrade, no pre_install and pre_upgrade. However, since bash has a scriptlet, runscriptlet will also be called before bash is installed. It won't do anything since the scriptlet has no pre_install function. But if we keep the check, we will still get "error : no /bin/sh". Conflicts: lib/libalpm/trans.c Signed-off-by: Xavier Chantry <shiningxc(a)gmail.com> Signed-off-by: Dan McGee <dan(a)archlinux.org> commit 36863b968f562840461e60be4ed23abf635d6008 Author: Xavier Chantry <shiningxc(a)gmail.com> Date: Sun Mar 15 15:42:35 2009 +0100 runscriptlet : Flush open fds before fork() to avoid cloning buffers This is a bug I noticed 2 years ago : http://www.nabble.com/Re%3A-logging-output-crazy-to11437357.html#a11479679 I thought I fixed it with 57d77eab32c01cd7572a11f76480a3724d09c029 But the bug was still here. Reading man fork, this part caught my attention: * The child inherits copies of the parent's set of open file descriptors. Each file descriptor in the child refers to the same open file description (see open(2)) as the corresponding file descriptor in the parent. This means that the two descriptors share open file status flags, current file offset, and signal-driven I/O attributes (see the description of F_SETOWN and F_SETSIG in fcntl(2)). Since the open file descriptors are inherited, it is probably a good idea to flush them before forking. Signed-off-by: Xavier Chantry <shiningxc(a)gmail.com> Signed-off-by: Dan McGee <dan(a)archlinux.org> commit 687f04ab4699440e3d8b371454b26039b6eb2772 Author: Allan McRae <allan(a)archlinux.org> Date: Sat Mar 14 23:43:05 2009 +1000 Only check checksums if there is a source file Signed-off-by: Allan McRae <allan(a)archlinux.org> Signed-off-by: Dan McGee <dan(a)archlinux.org> commit a864a50bc6a65a5c39a4727a07a3b269049a808e Author: Allan McRae <allan(a)archlinux.org> Date: Tue Mar 3 04:13:08 2009 +1000 contrib/pacscripts - print install scripts from a package Prints the install script from a given package file or from a package in the pacman repo. Original-work-by: Giulio "giulivo" Fidente <giulivo.navigante(a)gmail.com> Improvements-by: Xavier Chantry <shiningxc(a)gmail.com> Signed-off-by: Allan McRae <allan(a)archlinux.org> Signed-off-by: Dan McGee <dan(a)archlinux.org> ----------------------------------------------------------------------- Summary of changes: contrib/Makefile.am | 1 + contrib/README | 3 + contrib/pacscripts | 132 ++++++++++++++++++++++++++++++++++++++++++++++++ lib/libalpm/be_files.c | 6 +- lib/libalpm/trans.c | 10 +--- scripts/makepkg.sh.in | 2 + 6 files changed, 144 insertions(+), 10 deletions(-) create mode 100755 contrib/pacscripts hooks/post-receive -- The official pacman repository

1 0

[pacman-dev] [PATCH 1/2] libalpm: sync changes to disk when appropriate
by Dan McGee 15 Mar '09

15 Mar '09

We don't make calls to fsync() or fdatasync() when we are attempting to do something transactionally, such as writing out one of our DB entries. Add a call to fdatasync() when we write DB entries, and also ensure we sync our log changes to disk whenever we close it. Another important thing to ensure is that we commit our database additions and removals as well by syncing the DB directory itself. Signed-off-by: Dan McGee <dan(a)archlinux.org> --- Just a slight refactor- moving the sync code to it's own function so we can call it on both an add and remove operation. -Dan lib/libalpm/be_files.c | 30 +++++++++++++++++++++++------- lib/libalpm/handle.c | 6 +++++- 2 files changed, 28 insertions(+), 8 deletions(-) diff --git a/lib/libalpm/be_files.c b/lib/libalpm/be_files.c index 0765b8d..3da58a3 100644 --- a/lib/libalpm/be_files.c +++ b/lib/libalpm/be_files.c @@ -27,11 +27,13 @@ #include <string.h> #include <stdint.h> /* uintmax_t, intmax_t */ #include <sys/stat.h> +#include <sys/types.h> #include <dirent.h> #include <ctype.h> #include <time.h> #include <limits.h> /* PATH_MAX */ #include <locale.h> /* setlocale */ +#include <fcntl.h> /* open, close */ /* libalpm */ #include "db.h" @@ -343,6 +345,21 @@ static char *get_pkgpath(pmdb_t *db, pmpkg_t *info) return(pkgpath); } +static int sync_db(pmdb_t *db) +{ + int fd; + /* by syncing the parent directory, we can be sure any database + * changes (adding or removing a package) are committed to disk. */ + fd = open(db->path, 0); + if(fd != -1) { + fsync(fd); + close(fd); + return 0; + } else { + return -1; + } +} + int _alpm_db_read(pmdb_t *db, pmpkg_t *info, pmdbinfrq_t inforeq) { FILE *fp = NULL; @@ -776,8 +793,6 @@ int _alpm_db_write(pmdb_t *db, pmpkg_t *info, pmdbinfrq_t inforeq) "%s\n\n", info->md5sum); } } - fclose(fp); - fp = NULL; } /* FILES */ @@ -804,8 +819,6 @@ int _alpm_db_write(pmdb_t *db, pmpkg_t *info, pmdbinfrq_t inforeq) } fprintf(fp, "\n"); } - fclose(fp); - fp = NULL; } /* DEPENDS */ @@ -848,8 +861,6 @@ int _alpm_db_write(pmdb_t *db, pmpkg_t *info, pmdbinfrq_t inforeq) } fprintf(fp, "\n"); } - fclose(fp); - fp = NULL; } /* INSTALL */ @@ -860,7 +871,11 @@ cleanup: free(pkgpath); if(fp) { + fflush(fp); + fdatasync(fileno(fp)); fclose(fp); + fp = NULL; + sync_db(db); } return(retval); @@ -868,7 +883,7 @@ cleanup: int _alpm_db_remove(pmdb_t *db, pmpkg_t *info) { - int ret = 0; + int ret; char *pkgpath = NULL; ALPM_LOG_FUNC; @@ -881,6 +896,7 @@ int _alpm_db_remove(pmdb_t *db, pmpkg_t *info) ret = _alpm_rmrf(pkgpath); free(pkgpath); + sync_db(db); if(ret != 0) { ret = -1; } diff --git a/lib/libalpm/handle.c b/lib/libalpm/handle.c index 813f439..488459e 100644 --- a/lib/libalpm/handle.c +++ b/lib/libalpm/handle.c @@ -63,8 +63,10 @@ void _alpm_handle_free(pmhandle_t *handle) /* close logfile */ if(handle->logstream) { + fflush(handle->logstream); + fdatasync(fileno(handle->logstream)); fclose(handle->logstream); - handle->logstream= NULL; + handle->logstream = NULL; } if(handle->usesyslog) { handle->usesyslog = 0; @@ -419,6 +421,8 @@ int SYMEXPORT alpm_option_set_logfile(const char *logfile) FREE(oldlogfile); } if(handle->logstream) { + fflush(handle->logstream); + fdatasync(fileno(handle->logstream)); fclose(handle->logstream); handle->logstream = NULL; } -- 1.6.2

1 1

[pacman-dev] [PATCH] Revert "popen does NOT require /bin/sh in a subchroot"
by Xavier Chantry 15 Mar '09

15 Mar '09

This reverts commit 9558639d8009483fbf422b138d020745986f82f1. This change was wrong, popen does require /bin/sh in a subchroot. 1) pacman -S lilo -r root Notice no error 2) rm root/bin/sh ; pacman -S lilo -r root Notice an error : error: scriptlet failed to execute correctly Actually, we already get an explicit error here, when popen is run, so there is no need to check for bin/sh explicitely. Besides this check was problematic in some cases. For example, bash itself has a scriptlet, but only post_install and post_upgrade, no pre_install and pre_upgrade. However, since bash has a scriptlet, runscriptlet will also be called before bash is installed. It won't do anything since the scriptlet has no pre_install function. But if we keep the check, we will still get "error : no /bin/sh". Conflicts: lib/libalpm/trans.c Signed-off-by: Xavier Chantry <shiningxc(a)gmail.com> --- lib/libalpm/trans.c | 7 ------- 1 files changed, 0 insertions(+), 7 deletions(-) diff --git a/lib/libalpm/trans.c b/lib/libalpm/trans.c index 4b83119..32c6d90 100644 --- a/lib/libalpm/trans.c +++ b/lib/libalpm/trans.c @@ -443,13 +443,6 @@ int _alpm_runscriptlet(const char *root, const char *installfn, return(0); } - /* NOTE: popen will use the PARENT's /bin/sh, not the chroot's */ - if(access("/bin/sh", X_OK)) { - /* not found */ - _alpm_log(PM_LOG_ERROR, _("No /bin/sh in parent environment, aborting scriptlet\n")); - return(0); - } - /* creates a directory in $root/tmp/ for copying/extracting the scriptlet */ snprintf(tmpdir, PATH_MAX, "%stmp/", root); if(access(tmpdir, F_OK) != 0) { -- 1.6.2

3 2

[pacman-dev] [PATCH] Only check checksums if there is a source file
by Allan McRae 14 Mar '09

14 Mar '09

Signed-off-by: Allan McRae <allan(a)archlinux.org> --- scripts/makepkg.sh.in | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-) diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 2ff98f1..3ff256f 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -547,6 +547,8 @@ generate_checksums() { } check_checksums() { + [ ${#source[@]} -eq 0 ] && return 0 + if [ ! $(type -p openssl) ]; then error "$(gettext "Cannot find openssl.")" exit 1 # $E_MISSING_PROGRAM -- 1.6.2

1 0

[pacman-dev] delay writes a fsync
by Allan McRae 14 Mar '09

14 Mar '09

Hi, You have probably heard about this: http://linux.slashdot.org/article.pl?sid=09/03/11/2031231 Here is the link to the bug report that describes the cause of the problem: https://bugs.edge.launchpad.net/ubuntu/+source/linux/+bug/317781/comments/54 A key part of the bug report comment... Applications are expected to use fsync() or fdatasync(), and if that impacts their performance too much, to use a single berkdb or other binary database file, and not do something stupid with hundreds of tiny text files that only hold a few bytes of data in each text file. Hmmm... something sound familiar there? So what should we do? Maybe, add some sort of syncing after a db write. But then after each file or just at the end of the transaction? Allan

8 19

[pacman-dev] delta support in libalpm
by Henning Garus 14 Mar '09

14 Mar '09

Hi, I have been looking through the current delta implementation in libalpm and have put some thought into changing makepkg/repo-add to support delta creation. However, I'm running into some problems, mostly due to md5sums and gzip. The current implementation works as follows. On a sync operation it is checked, whether a valid delta path exists and if the summed filesize of the deltas is smaller than the filesize of the whole download. When this is the case the deltas are downloaded and applied to the old file. After that the patched file is treated as if it was downloaded normally, this includes a check of the md5sum. Gzip files have a header, that has a timestamp, which will screw with this md5sum. When a patch is applied to a gzipped file by xdelta, xdelta will unzip the file, apply the patch and then rezip the file. The author of xdelta was obviously aware of the problems with the timestamp, because he decided to leave it empty. The same can be achieved by the -n option of gzip. But there comes the next problem, xdelta uses zlib for compression, gzip implements compression itself. And files created by gzip can differ from files created by zlib. Bsdtar uses zlib as well, but writes the timestamp and there is no option to prevent this (at least none that I can see). There are four ways around this, that I can think of: 1. create the package, then create the delta, apply the delta to the old version, remove the original new package and present the patched package as output I think this sucks, this ties delta creation to makepkg (more about that later) and has an incredibly huge and useless overhead (countless unzips and rezips and applying the patch). 2. create the package, but don't compress it with bsdtar, use gzip -n instead. This means we have to use gzip again, in libalpm, when we apply the delta. Seems better than 1, but makes makepkg and libalpm rely on gzip. Not sure if this is a good thing, especially for libalpm. 3. save the md5sums of the unzipped tars in the synchdb and change libalpm to check those Seems reasonable, but I don't see a way to do this with libarchive, so this would require using zlib directly and pacman would lose the ability to handle to handle tar.bz2 4. Skip checking the md5sum for deltas OK during the initial synch, as long as we trust xdelta to do its job (the md5sums of both the old and the new file are in the delta file). But the created package will have the wrong md5sum and can't be used to reinstall, etc. which makes this look like a bad idea. In a previous mail Xavier toyed with the idea to put delta creation into repo-add, I have given this some thought, as it seems nice in principle, but there are drawbacks. For Arch this would mean creating deltas on Gerolde, which seems to be fairly strained already, according to the dev list. Furthermore this introduces some new variables to repo-add (at least repo location and an output location) this would be manageable, but doesn't look very nice. Delta creation in makepkg seems somehow ok (its already in there after all). But what I would really like is a separate tool for delta creation, which would allow the separation of building packages and creating deltas and setting up a separated delta server. This leaves us with options 2 and 3 and I am not really sure, which way to go. looking forward to your comments

6 40

[pacman-dev] Checking for VSC/SCM tools in makepkg
by Allan McRae 09 Mar '09

09 Mar '09

Hi, Am I missing something here or should these sorts of tests cause a failure: devel_check() { ... # Also do a brief check to make sure we have the VCS tool available. ... [ $(type -p darcs) ] || return 0 ... [ $(type -p svn) ] || return 0 etc. At the moment, these tests just seem useless to me. Making this an error would require a decent change in makepkg as at the moment devel_check() is called before dependency resolution is done. In fact, I don't think I have ever managed to build an SCM package in a clean chroot and this may be the cause. If no-one points out something obvious I am missing, then I will file a bug report so this is not forgotten. Allan

2 4

[pacman-dev] Useless comments...
by Allan McRae 09 Mar '09

09 Mar '09

Hi, if (you don't want to read a semi rant) exit 0; I have noticed a number of useless comments in makepkg and have decided that they are worse than no comments. Examples: 1) This inspired the rant! download_sources # we can only check checksums if we have all files check_checksums Huh, why are we not checking for all files then? Because download_sources exits when it fails... 2) and there are a lot of these: # fix flyspray feature request #2978 # fix flyspray bug #5923 # Fixes FS#10039 # fix flyspray #6246 #fix flyspray feature request #5223 # fix flyspray bug #5973 and I am guilty here... but I was at the airport with no internet access so I had no idea if these are important. And most of these appeared to be non-obscure features/fixes so did not need a comment justifying their inclusion. 3) overly obvious comments # do we have a changelog? if [ -f "$startdir/ChangeLog" ]; then If you do not understand that test, then leave the code alone... I will patch these out eventually but I like to share my frustration. I am just a generous guy! Allan

4 3