[Rpm-maint] [PATCH] Unbreak short-circuited binary builds

[Panu Matilainen]

On 01/05/2017 02:50 PM, Mark Wielaard wrote:
> On Thu, Jan 05, 2017 at 01:47:28PM +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.
>
> Sorry about that. I didn't even know rpmbuild --short-circuit existed.
> Looks like there are lots of subtle ways this can get broken. Might be
> good to add a testcase for it.

Yup, testcase wouldn't hurt. OTOH this is the first time ever something 
broke it :) It's also such a crazy misfeature... I mean, it creates 
deliberately broken packages because its strictly for non-production use 
only. I think today was the first time I ever had an actual use-case for 
it personally, while debugging an unrelated problem.

>
>> 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.
>
>> @@ -1572,6 +1573,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 might already exist  */
>> +	if ((llen = readlink(linkpath, ltarget, sizeof(ltarget)-1)) != -1) {
>> +	    ltarget[llen] = '\0';
>> +	    if (rstreq(ltarget, targetpath)) {
>> +		exists = 1;
>> +		break;
>> +	    }
>> +	}
>
> Allocating PATH_MAX size on the stack might be a bit much
> (and on some systems PATH_MAX might not exist). Might be
> good to use lstat first to see if it is a symbolic link
> and use the st_size i+1 as buffer size. I would malloc/free
> it to not accidentially blow up the stack.

Yeah, using PATH_MAX is broken but then rpm is getting away with it (on 
stack and all) in many other places so one more here and there... but 
good point using lstat() there, will change it to use that, thanks.

Other than that - does it look like doing the right thing to you? I'm 
really not familiar with the debuginfo business at all :)

	- Panu -

>
> Cheers,
>
> Mark
>