Re: [Rpm-maint] [PATCH v2] Unbreak short-circuited binary builds (RhBug:1434235)

2017-03-24 Thread Panu Matilainen 

On 03/23/2017 04:37 PM, Mark Wielaard wrote:

On Wed, 2017-03-22 at 14:37 +0200, Panu Matilainen wrote:

Guess there's some unhandled corner left still :)


Yeah, that was because my fix was bogus :{

It didn't construct the linkpath correctly (because it was relative). It
checked with readlink (), but that doesn't fully expand the links, it
should have used canonicalize_file_name () on both the link and the
target (because the target is itself a symlink). And even then it
wouldn't have worked because we don't know which duplicate the compat
link is targeting, so the first existing target would always win.

So back to the drawing board. The problem is the compat links in case
there are duplicate build-ids. We don't want the compat links to
"duplicate" the duplication search already done for the buildids already
done. So in this new patch instead of saying we want to create a compat
link when calling addNewBuildIDSymlink, we ask to search (and return)
the number of duplicates found. We can then use that to create the
compat links directly (with the correct duplication counter).

Attached is your original patch plus my don't do duplication search for
compat links change. That makes the testsuite pass and the popt
short-circuit example work for me.


Confirmed, applied, and many many thanks.

- Panu -

___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint

Re: [Rpm-maint] [PATCH v2] Unbreak short-circuited binary builds (RhBug:1434235)

2017-03-23 Thread Mark Wielaard 
On Wed, 2017-03-22 at 14:37 +0200, Panu Matilainen wrote:
> Guess there's some unhandled corner left still :)

Yeah, that was because my fix was bogus :{

It didn't construct the linkpath correctly (because it was relative). It
checked with readlink (), but that doesn't fully expand the links, it
should have used canonicalize_file_name () on both the link and the
target (because the target is itself a symlink). And even then it
wouldn't have worked because we don't know which duplicate the compat
link is targeting, so the first existing target would always win.

So back to the drawing board. The problem is the compat links in case
there are duplicate build-ids. We don't want the compat links to
"duplicate" the duplication search already done for the buildids already
done. So in this new patch instead of saying we want to create a compat
link when calling addNewBuildIDSymlink, we ask to search (and return)
the number of duplicates found. We can then use that to create the
compat links directly (with the correct duplication counter).

Attached is your original patch plus my don't do duplication search for
compat links change. That makes the testsuite pass and the popt
short-circuit example work for me.

Cheers,

Mark
From e6c0dbf9c6e56bfae6a596929b69d4474c3d69f1 Mon Sep 17 00:00:00 2001
From: Panu Matilainen 
Date: Thu, 5 Jan 2017 13:47:28 +0200
Subject: [PATCH] Unbreak short-circuited binary builds

Commit bbfe1f86b2e4b5c0bd499d9f3dd9de9c9c20fff2 broke short-circuited
binary builds (which can be handy for testing when working on large
packages), eg:
 rpmbuild -bi foo.spec; rpmbuild -bb --short-circuit foo.spec

The problem is that in a short-circuited build all the links already
exist and point to the right place, but the code doesn't realize this
and creates new links instead, which leaves the old links unowned
in the buildroot which ultimately causes the build to fail with
"Installed (but unpackaged) file(s) found" for the previously created
build-id links.

When checking for pre-existing links see if they already point to
the right file and in that case just reuse it instead of creating new ones.
Keep track of duplicate build-ids found by noticing existing links that
point to different targets. But don't do this for compat links, they should
just point to the last (duplicate) main build-id symlink found.

Signed-off-by: Mark Wielaard 
---
 build/files.c | 71 ++-
 1 file changed, 51 insertions(+), 20 deletions(-)

diff --git a/build/files.c b/build/files.c
index cca14b9..93021d1 100644
--- a/build/files.c
+++ b/build/files.c
@@ -1592,11 +1592,12 @@ exit:
 
 static int addNewIDSymlink(FileList fl,
 			   char *targetpath, char *idlinkpath,
-			   int isDbg, int isCompat)
+			   int isDbg, int *dups)
 {
 const char *linkerr = _("failed symlink");
 int rc = 0;
 int nr = 0;
+int exists = 0;
 char *origpath, *linkpath;
 
 if (isDbg)
@@ -1606,6 +1607,26 @@ static int addNewIDSymlink(FileList fl,
 origpath = linkpath;
 
 while (faccessat(AT_FDCWD, linkpath, F_OK, AT_SYMLINK_NOFOLLOW) == 0) {
+/* We don't care about finding dups for compat links, they are
+	   OK as is.  Otherwise we will need to double check if
+	   existing link points to the correct target. */
+	if (dups == NULL)
+	  {
+	exists = 1;
+	break;
+	  }
+
+	char ltarget[PATH_MAX];
+	ssize_t llen;
+	/* In short-circuited builds the link might already exist  */
+	if ((llen = readlink(linkpath, ltarget, sizeof(ltarget)-1)) != -1) {
+	ltarget[llen] = '\0';
+	if (rstreq(ltarget, targetpath)) {
+		exists = 1;
+		break;
+	}
+	}
+
 	if (nr > 0)
 	free(linkpath);
 	nr++;
@@ -1613,21 +1634,16 @@ static int addNewIDSymlink(FileList fl,
 		  isDbg ? ".debug" : "");
 }
 
-char *symtarget = targetpath;
-if (nr > 0 && isCompat)
-	rasprintf (, "%s.%d", targetpath, nr);
-
-if (symlink(symtarget, linkpath) < 0) {
+if (!exists && symlink(targetpath, linkpath) < 0) {
 	rc = 1;
 	rpmlog(RPMLOG_ERR, "%s: %s -> %s: %m\n",
-	   linkerr, linkpath, symtarget);
+	   linkerr, linkpath, targetpath);
 } else {
 	fl->cur.isDir = 0;
 	rc = addFile(fl, linkpath, NULL);
 }
 
-/* Don't warn (again) if this is a compat id-link, we retarget it. */
-if (nr > 0 && !isCompat) {
+if (nr > 0) {
 	/* Lets see why there are multiple build-ids. If the original
 	   targets are hard linked, then it is OK, otherwise warn
 	   something fishy is going on. Would be nice to call
@@ -1656,8 +1672,8 @@ static int addNewIDSymlink(FileList fl,
 	free(origpath);
 if (nr > 0)
 	free(linkpath);
-if (nr > 0 && isCompat)
-	free(symtarget);
+if (dups != NULL)
+  *dups = nr;
 
 return rc;
 }
@@ -1897,6 +1913,7 @@ static int generateBuildIDs(FileList fl)
 			|| (rc = addFile(fl, buildidsubdir, NULL)) == 0) {
 			char *linkpattern, *targetpattern;
 			char *linkpath, *targetpath;
+

Re: [Rpm-maint] [PATCH v2] Unbreak short-circuited binary builds (RhBug:1434235)

2017-03-22 Thread Panu Matilainen 

On 03/22/2017 01:34 PM, Mark Wielaard wrote:

On Tue, 2017-03-21 at 14:15 +0200, Panu Matilainen wrote:

Commit bbfe1f86b2e4b5c0bd499d9f3dd9de9c9c20fff2 broke short-circuited
binary builds (which can be handy for testing when working on large
packages), eg:
 rpmbuild -bi foo.spec; rpmbuild -bb --short-circuit foo.spec

The problem is that in a short-circuited build all the links already
exist and point to the right place, but the code doesn't realize this
and creates new links instead, which leaves the old links unowned
in the buildroot which ultimately causes the build to fail with
"Installed (but unpackaged) file(s) found" for the previously created
build-id links.

When checking for pre-existing links see if they already point to
the right file and in that case just reuse it instead of creating new
ones, IFF %install did not execute (which means we must be in
 --short-circuited build)


It bothered me that we need to special case the --short-circuit case
since in theory your original fix should have worked.

The reason it didn't work for duplicate build-ids when using compat
links is that the compat links themselves are symlinks. So when checking
that the existing link points to the expected target we need to resolve
both the link path and the target path before comparing.

Which is what the attached patch (applied on top of your original patch)
does. With that I get zero fail again on make check.


Good. However --short-circuit is doesn't *quite* work with this version.

Using popt-1.16-7.fc24.src.rpm from Fedora as a test case, doing

$ rpm -i  popt-1.16-7.fc24.src.rpm
$ rpmbuild -bi ~/rpmbuild/SPECS/popt.spec
$ rpmbuild -bl --short-circuit ~/rpmbuild/SPECS/popt.spec

With unpatched git master I get some duplicate warnings plus:
error: Installed (but unpackaged) file(s) found:
   /usr/lib/.build-id/ef/2ba32deaf4d979c4f58538ba9c7594ff2fd7dc
   /usr/lib/debug/.build-id/ef/2ba32deaf4d979c4f58538ba9c7594ff2fd7dc
   /usr/lib/debug/.build-id/ef/2ba32deaf4d979c4f58538ba9c7594ff2fd7dc.debug

With this patch I only get:
error: Installed (but unpackaged) file(s) found:
   /usr/lib/debug/.build-id/49/bc01fadb22c6483481c0fea95e9a6013936b41

Guess there's some unhandled corner left still :)

- Panu -
___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint

[Rpm-maint] [PATCH v2] Unbreak short-circuited binary builds (RhBug:1434235)

2017-03-21 Thread Panu Matilainen 
Commit bbfe1f86b2e4b5c0bd499d9f3dd9de9c9c20fff2 broke short-circuited
binary builds (which can be handy for testing when working on large
packages), eg:
 rpmbuild -bi foo.spec; rpmbuild -bb --short-circuit foo.spec

The problem is that in a short-circuited build all the links already
exist and point to the right place, but the code doesn't realize this
and creates new links instead, which leaves the old links unowned
in the buildroot which ultimately causes the build to fail with
"Installed (but unpackaged) file(s) found" for the previously created
build-id links.

When checking for pre-existing links see if they already point to
the right file and in that case just reuse it instead of creating new
ones, IFF %install did not execute (which means we must be in
 --short-circuited build)
---

V2: Only permit pre-existing build-id links on short-circuited builds

 build/files.c | 25 ++---
 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/build/files.c b/build/files.c
index 85cb984..86f1cec 100644
--- a/build/files.c
+++ b/build/files.c
@@ -1592,11 +1592,12 @@ exit:
 
 static int addNewIDSymlink(FileList fl,
   char *targetpath, char *idlinkpath,
-  int isDbg, int isCompat)
+  int isDbg, int isCompat, int didInstall)
 {
 const char *linkerr = _("failed symlink");
 int rc = 0;
 int nr = 0;
+int exists = 0;
 char *origpath, *linkpath;
 
 if (isDbg)
@@ -1606,6 +1607,16 @@ static int addNewIDSymlink(FileList fl,
 origpath = linkpath;
 
 while (faccessat(AT_FDCWD, linkpath, F_OK, AT_SYMLINK_NOFOLLOW) == 0) {
+   char ltarget[PATH_MAX];
+   ssize_t llen;
+   /* In short-circuited builds the link could already exist  */
+   if (!didInstall && (llen = readlink(linkpath, ltarget, 
sizeof(ltarget)-1)) != -1) {
+   ltarget[llen] = '\0';
+   if (rstreq(ltarget, targetpath)) {
+   exists = 1;
+   break;
+   }
+   }
if (nr > 0)
free(linkpath);
nr++;
@@ -1617,7 +1628,7 @@ static int addNewIDSymlink(FileList fl,
 if (nr > 0 && isCompat)
rasprintf (, "%s.%d", targetpath, nr);
 
-if (symlink(symtarget, linkpath) < 0) {
+if (!exists && symlink(symtarget, linkpath) < 0) {
rc = 1;
rpmlog(RPMLOG_ERR, "%s: %s -> %s: %m\n",
   linkerr, linkpath, symtarget);
@@ -1662,7 +1673,7 @@ static int addNewIDSymlink(FileList fl,
 return rc;
 }
 
-static int generateBuildIDs(FileList fl)
+static int generateBuildIDs(FileList fl, int didInstall)
 {
 int rc = 0;
 int i;
@@ -1878,7 +1889,7 @@ static int generateBuildIDs(FileList fl)
  buildidsubdir, [i][2]);
rasprintf(, targetpattern, paths[i]);
rc = addNewIDSymlink(fl, targetpath, linkpath,
-isDbg, 0);
+isDbg, 0, didInstall);
 
/* We might want to have a link from the debug
   build_ids dir to the main one. We create it
@@ -1910,7 +1921,7 @@ static int generateBuildIDs(FileList fl)
  "../../../.build-id%s/%s",
  subdir, [i][2]);
rc = addNewIDSymlink(fl, targetpath, linkpath,
-0, 1);
+0, 1, didInstall);
}
 
if (rc == 0 && isDbg
@@ -1948,7 +1959,7 @@ static int generateBuildIDs(FileList fl)
rasprintf(, "../../../../..%s",
  targetstr);
rc = addNewIDSymlink(fl, targetpath,
-linkpath, 0, 0);
+linkpath, 0, 0, 
didInstall);
free(targetstr);
}
}
@@ -2384,7 +2395,7 @@ static rpmRC processPackageFiles(rpmSpec spec, 
rpmBuildPkgFlags pkgFlags,
goto exit;
 
 #if HAVE_LIBDW
-if (generateBuildIDs () != 0) {
+if (generateBuildIDs (, didInstall) != 0) {
rpmlog(RPMLOG_ERR, _("Generating build-id links failed\n"));
fl.processingFailed = 1;
goto exit;
-- 
2.9.3

___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint