[pacman-dev] Adding a new config file to a package (e.g. mercurial file conflict)
I did an -Syu on my Arch box today, which updated Mercurial, and the new package contains a /etc/mercurial/hgrc file. I had already created one on my system, and the old package did not contain one, so I got a file conflict. (54/54) checking package integrity [######################] 100% (54/54) checking for file conflicts [######################] 100% error: failed to commit transaction (conflicting files) mercurial: /etc/mercurial/hgrc exists in filesystem Errors occurred, no packages were upgraded. So instead, I had to -Sf the package, which installs the file with a pacnew: (1/1) checking package integrity [######################] 100% (1/1) checking available disk space [######################] 100% (1/1) upgrading mercurial [######################] 100% warning: /etc/mercurial/hgrc installed as /etc/mercurial/hgrc.pacnew This behavior is a bit...odd. We don't like people using -f, and they are trained to assume the worst, so most would not expect the backup logic to still kick into effect. A pactest is below, but I'm asking for thoughts on expected behavior in this case. Many possibilities listed for completeness, some of which are just plain dumb. 1) Keep as is with file conflict error, require -Sf, keep backing up file to pacnew 2) Keep as is with file conflict error, require -Sf, don't backup (this is stupid) 3) Don't error at all, treat as a normal backup case except pretend old file is the same as /dev/null (aka empty). 4) Same as (3), but make the pacnew warning message slightly different, e.g. "%s is new in package, installed as %s" or something. 5) Something like (3) or (4) but don't even treat it the same as an empty file, treat it as never matching anything. My vote is 3 or 4, not that we are a democracy. :) -Dan self.description = "Upgrade a package, with a file entering the pkg in 'backup'" lp = pmpkg("dummy") lp.files = ["usr/bin/dummy"] self.addpkg2db("local", lp) p = pmpkg("dummy", "1.0-2") p.files = ["usr/bin/dummy", "etc/dummy.conf"] p.backup = ["etc/dummy.conf"] self.addpkg(p) self.args = "-U %s" % p.filename() self.addrule("PACMAN_RETCODE=0") self.addrule("PKG_VERSION=dummy|1.0-2") self.addrule("!FILE_PACSAVE=etc/dummy.conf") self.addrule("FILE_PACNEW=etc/dummy.conf") self.addrule("FILE_EXIST=etc/dummy.conf")
On Thu, Jul 14, 2011 at 12:28:31PM -0500, Dan McGee wrote:
I did an -Syu on my Arch box today, which updated Mercurial, and the new package contains a /etc/mercurial/hgrc file. I had already created one on my system, and the old package did not contain one, so I got a file conflict.
(54/54) checking package integrity [######################] 100% (54/54) checking for file conflicts [######################] 100% error: failed to commit transaction (conflicting files) mercurial: /etc/mercurial/hgrc exists in filesystem Errors occurred, no packages were upgraded.
So instead, I had to -Sf the package, which installs the file with a pacnew:
(1/1) checking package integrity [######################] 100% (1/1) checking available disk space [######################] 100% (1/1) upgrading mercurial [######################] 100% warning: /etc/mercurial/hgrc installed as /etc/mercurial/hgrc.pacnew
This behavior is a bit...odd. We don't like people using -f, and they are trained to assume the worst, so most would not expect the backup logic to still kick into effect. A pactest is below, but I'm asking for thoughts on expected behavior in this case. Many possibilities listed for completeness, some of which are just plain dumb.
1) Keep as is with file conflict error, require -Sf, keep backing up file to pacnew 2) Keep as is with file conflict error, require -Sf, don't backup (this is stupid)
3) Don't error at all, treat as a normal backup case except pretend old file is the same as /dev/null (aka empty).
This is where I sit. You shouldn't have to force the install, and the new file from the backup array should be dropped in place with a .pacnew extension. IMO, you get the best user experience going this route -- you don't need to go out of your way to do the upgrade, and there's notification about the new file being dropped in. I don't think there's any need to create a different message. It's sort of a special case, but I don't think it needs any special handling. d
4) Same as (3), but make the pacnew warning message slightly different, e.g. "%s is new in package, installed as %s" or something. 5) Something like (3) or (4) but don't even treat it the same as an empty file, treat it as never matching anything.
My vote is 3 or 4, not that we are a democracy. :)
-Dan
self.description = "Upgrade a package, with a file entering the pkg in 'backup'"
lp = pmpkg("dummy") lp.files = ["usr/bin/dummy"] self.addpkg2db("local", lp)
p = pmpkg("dummy", "1.0-2") p.files = ["usr/bin/dummy", "etc/dummy.conf"] p.backup = ["etc/dummy.conf"] self.addpkg(p)
self.args = "-U %s" % p.filename()
self.addrule("PACMAN_RETCODE=0") self.addrule("PKG_VERSION=dummy|1.0-2") self.addrule("!FILE_PACSAVE=etc/dummy.conf") self.addrule("FILE_PACNEW=etc/dummy.conf") self.addrule("FILE_EXIST=etc/dummy.conf")
On 07/14/2011 07:44 PM, Dave Reisner wrote:
On Thu, Jul 14, 2011 at 12:28:31PM -0500, Dan McGee wrote:
3) Don't error at all, treat as a normal backup case except pretend old file is the same as /dev/null (aka empty). This is where I sit. You shouldn't have to force the install, and the new file from the backup array should be dropped in place with a .pacnew extension. IMO, you get the best user experience going this route -- you don't need to go out of your way to do the upgrade, and there's notification about the new file being dropped in. I don't think there's any need to create a different message. It's sort of a special case, but I don't think it needs any special handling.
d
Agreed, this sounds like the right thing to do.
The bulk of this commit is adding new tests to ensure the new behavior works without disrupting old behavior. This is a relatively sane maneuver when a package adds a conf file (e.g. '/etc/mercurial/hgrc') that was not previously in the package, but it is placed in the backup array. In essence, we can treat the existing file as having always been a part of the package and do our normal compare/install as pacnew logic checks. Signed-off-by: Dan McGee <dan@archlinux.org> --- Focus mainly on the one new check block in conflict.c - the rest is a bit of simplification to the alpm_needbackup() call since we always fetch the package backup list from a package anyway, as well as adding 3 tests. lib/libalpm/add.c | 4 ++-- lib/libalpm/backup.c | 7 +++---- lib/libalpm/backup.h | 2 +- lib/libalpm/conflict.c | 16 ++++++++++++++++ lib/libalpm/remove.c | 2 +- test/pacman/tests/fileconflict003.py | 2 +- test/pacman/tests/upgrade027.py | 22 ++++++++++++++++++++++ test/pacman/tests/upgrade028.py | 22 ++++++++++++++++++++++ test/pacman/tests/upgrade029.py | 24 ++++++++++++++++++++++++ 9 files changed, 92 insertions(+), 9 deletions(-) create mode 100644 test/pacman/tests/upgrade027.py create mode 100644 test/pacman/tests/upgrade028.py create mode 100644 test/pacman/tests/upgrade029.py diff --git a/lib/libalpm/add.c b/lib/libalpm/add.c index e182746..5e1bb8d 100644 --- a/lib/libalpm/add.c +++ b/lib/libalpm/add.c @@ -255,7 +255,7 @@ static int extract_single_file(alpm_handle_t *handle, struct archive *archive, alpm_backup_t *backup; /* go to the backup array and see if our conflict is there */ /* check newpkg first, so that adding backup files is retroactive */ - backup = _alpm_needbackup(entryname, alpm_pkg_get_backup(newpkg)); + backup = _alpm_needbackup(entryname, newpkg); if(backup) { /* if we force hash_orig to be non-NULL retroactive backup works */ hash_orig = ""; @@ -264,7 +264,7 @@ static int extract_single_file(alpm_handle_t *handle, struct archive *archive, /* check oldpkg for a backup entry, store the hash if available */ if(oldpkg) { - backup = _alpm_needbackup(entryname, alpm_pkg_get_backup(oldpkg)); + backup = _alpm_needbackup(entryname, oldpkg); if(backup) { hash_orig = backup->hash; needbackup = 1; diff --git a/lib/libalpm/backup.c b/lib/libalpm/backup.c index becc7be..728c1d0 100644 --- a/lib/libalpm/backup.c +++ b/lib/libalpm/backup.c @@ -58,16 +58,15 @@ int _alpm_split_backup(const char *string, alpm_backup_t **backup) /* Look for a filename in a alpm_pkg_t.backup list. If we find it, * then we return the full backup entry. */ -alpm_backup_t *_alpm_needbackup(const char *file, const alpm_list_t *backup_list) +alpm_backup_t *_alpm_needbackup(const char *file, alpm_pkg_t *pkg) { const alpm_list_t *lp; - if(file == NULL || backup_list == NULL) { + if(file == NULL || pkg == NULL) { return NULL; } - /* run through the backup list and parse out the hash for our file */ - for(lp = backup_list; lp; lp = lp->next) { + for(lp = alpm_pkg_get_backup(pkg); lp; lp = lp->next) { alpm_backup_t *backup = lp->data; if(strcmp(file, backup->name) == 0) { diff --git a/lib/libalpm/backup.h b/lib/libalpm/backup.h index 39c01a6..0b84a68 100644 --- a/lib/libalpm/backup.h +++ b/lib/libalpm/backup.h @@ -24,7 +24,7 @@ #include "alpm.h" int _alpm_split_backup(const char *string, alpm_backup_t **backup); -alpm_backup_t *_alpm_needbackup(const char *file, const alpm_list_t *backup_list); +alpm_backup_t *_alpm_needbackup(const char *file, alpm_pkg_t *pkg); void _alpm_backup_free(alpm_backup_t *backup); alpm_backup_t *_alpm_backup_dup(const alpm_backup_t *backup); diff --git a/lib/libalpm/conflict.c b/lib/libalpm/conflict.c index 8cf1619..eda6ba1 100644 --- a/lib/libalpm/conflict.c +++ b/lib/libalpm/conflict.c @@ -549,6 +549,22 @@ alpm_list_t *_alpm_db_find_fileconflicts(alpm_handle_t *handle, free(rpath); } + /* is the file unowned and in the backup list of the new package? */ + if(!resolved_conflict && _alpm_needbackup(filestr, p1)) { + alpm_list_t *local_pkgs = _alpm_db_get_pkgcache(handle->db_local); + int found = 0; + for(k = local_pkgs; k && !found; k = k->next) { + if(_alpm_filelist_contains(alpm_pkg_get_files(k->data), filestr)) { + found = 1; + } + } + if(!found) { + _alpm_log(handle, ALPM_LOG_DEBUG, + "file was unowned but in new backup list: %s\n", path); + resolved_conflict = 1; + } + } + if(!resolved_conflict) { conflicts = add_fileconflict(handle, conflicts, ALPM_FILECONFLICT_FILESYSTEM, path, p1->name, NULL); diff --git a/lib/libalpm/remove.c b/lib/libalpm/remove.c index 5d2256d..b15dbaa 100644 --- a/lib/libalpm/remove.c +++ b/lib/libalpm/remove.c @@ -255,7 +255,7 @@ static void unlink_file(alpm_handle_t *handle, alpm_pkg_t *info, } } else { /* if the file needs backup and has been modified, back it up to .pacsave */ - alpm_backup_t *backup = _alpm_needbackup(fileobj->name, alpm_pkg_get_backup(info)); + alpm_backup_t *backup = _alpm_needbackup(fileobj->name, info); if(backup) { if(nosave) { _alpm_log(handle, ALPM_LOG_DEBUG, "transaction is set to NOSAVE, not backing up '%s'\n", file); diff --git a/test/pacman/tests/fileconflict003.py b/test/pacman/tests/fileconflict003.py index 89696fc..749e5a9 100644 --- a/test/pacman/tests/fileconflict003.py +++ b/test/pacman/tests/fileconflict003.py @@ -1,4 +1,4 @@ -self.description = "FS#8156" +self.description = "FS#8156- conflict between directory and incoming symlink" p1 = pmpkg("pkg1") p1.files = ["test/", diff --git a/test/pacman/tests/upgrade027.py b/test/pacman/tests/upgrade027.py new file mode 100644 index 0000000..99087f3 --- /dev/null +++ b/test/pacman/tests/upgrade027.py @@ -0,0 +1,22 @@ +self.description = "Upgrade a package, with a file entering the pkg in 'backup' (changed)" + +self.filesystem = ["etc/dummy.conf"] + +lp = pmpkg("dummy") +lp.files = ["usr/bin/dummy"] +self.addpkg2db("local", lp) + +p = pmpkg("dummy", "1.0-2") +p.files = ["usr/bin/dummy", + "etc/dummy.conf*"] +p.backup = ["etc/dummy.conf"] +self.addpkg(p) + +self.args = "-U %s" % p.filename() + +self.addrule("PACMAN_RETCODE=0") +self.addrule("PKG_VERSION=dummy|1.0-2") +self.addrule("FILE_PACNEW=etc/dummy.conf") +self.addrule("!FILE_PACSAVE=etc/dummy.conf") +self.addrule("!FILE_PACORIG=etc/dummy.conf") +self.addrule("FILE_EXIST=etc/dummy.conf") diff --git a/test/pacman/tests/upgrade028.py b/test/pacman/tests/upgrade028.py new file mode 100644 index 0000000..18a10f5 --- /dev/null +++ b/test/pacman/tests/upgrade028.py @@ -0,0 +1,22 @@ +self.description = "Upgrade a package, with a file entering the pkg in 'backup' (unchanged)" + +self.filesystem = ["etc/dummy.conf"] + +lp = pmpkg("dummy") +lp.files = ["usr/bin/dummy"] +self.addpkg2db("local", lp) + +p = pmpkg("dummy", "1.0-2") +p.files = ["usr/bin/dummy", + "etc/dummy.conf"] +p.backup = ["etc/dummy.conf"] +self.addpkg(p) + +self.args = "-U %s" % p.filename() + +self.addrule("PACMAN_RETCODE=0") +self.addrule("PKG_VERSION=dummy|1.0-2") +self.addrule("!FILE_PACNEW=etc/dummy.conf") +self.addrule("!FILE_PACSAVE=etc/dummy.conf") +self.addrule("!FILE_PACORIG=etc/dummy.conf") +self.addrule("FILE_EXIST=etc/dummy.conf") diff --git a/test/pacman/tests/upgrade029.py b/test/pacman/tests/upgrade029.py new file mode 100644 index 0000000..c308f42 --- /dev/null +++ b/test/pacman/tests/upgrade029.py @@ -0,0 +1,24 @@ +self.description = "Upgrade a package, with an owned file entering the pkg in 'backup'" + +lp = pmpkg("dummy") +lp.files = ["usr/bin/dummy"] +self.addpkg2db("local", lp) + +lp2 = pmpkg("dummy2") +lp2.files = ["etc/dummy.conf"] +self.addpkg2db("local", lp2) + +p = pmpkg("dummy", "1.0-2") +p.files = ["usr/bin/dummy", + "etc/dummy.conf*"] +p.backup = ["etc/dummy.conf"] +self.addpkg(p) + +self.args = "-U %s" % p.filename() + +self.addrule("PACMAN_RETCODE=1") +self.addrule("PKG_VERSION=dummy|1.0-1") +self.addrule("!FILE_PACNEW=etc/dummy.conf") +self.addrule("!FILE_PACSAVE=etc/dummy.conf") +self.addrule("!FILE_PACORIG=etc/dummy.conf") +self.addrule("FILE_EXIST=etc/dummy.conf") -- 1.7.6
participants (4)
-
Dan McGee
-
Dan McGee
-
Dave Reisner
-
Jakob Gruber