[Pkg-xen-devel] [PATCH 3/9] d/shuffle-binaries: Fix binary shuffling script for cross-building

[Hans van Kranenburg]

On 12/1/20 8:18 PM, Elliott Mitchell wrote:
> On Mon, Nov 30, 2020 at 11:31:00PM +0100, Hans van Kranenburg wrote:
>> On 7/17/20 8:37 AM, Elliott Mitchell wrote:
>>
>>> so use `file` to detect scripts
>>> and `strings | grep` for identifying linked libraries.  Even though
>>> debhelper depends on file, make the requirement explicit.
>>
>> Why would we stop to depend on debhelper? I don't mind to have another
>> line with redundancy, but I'm just wondering what your thought behind
>> this would be.
> 
> As Diederik de Haas noted, the concern is the dependency is indirect.  If
> debhelper was to stop depending on file (unlikely at this time, but not
> impossible in the future), then the Xen build mysteriously breaks without
> any visible change.

Ah, yes, ack, fully agreed. debhelper uses it for something, and we use
it for something, it's not that debhelper depends on it because "if you
use debhelper, you also want file", it's not a meta-thing.

>>> Heavily simplify script inner loop.  While the core of that inner loop
>>> was often executed only once, it is best to shrink inner loops.
>>>
>>> With the reduced inner loop, less is being done inside pipes, so pipefail
>>> is lower value (the early commands in pipes are unlikely to fail).  This
>>> makes the script POSIX compliant so switch to /bin/sh.
>>>
>>> Remove the useless extra argument passed to the script.
>>
>> And that's 5? 6? 10? changes at once, and now I have to go dig into the
>> diff and find out which change belongs to which line in the comments.
>>
>> You already went through all of the thought work doing all of this. And
>> that's much appreciated. Now make it explicit.
>>
>> So, if you can split this out in a series of commits that just does one
>> logical change with one simple explanation, and even if there's just a
>> simple commit in the end that changes /bin/bash to /bin/sh with an
>> explanation of "yay, now we got rid of all the stuff that prevented us
>> from switching to /bin/sh for performance yay", no problem.
> 
> I felt it was a single change, "major rework of binary shuffle script".

Ok, yes, I just had no idea where to look for what, because the title
says "Fix something" but there's a whole bunch of extra stuff going on,
and it does not explain what the problem is, so I do not know which
piece of the change actually is part of fixing the problem that I do not
know about.

> I suppose some portions can be broken into separate commits.  Some will
> be heavily overlapping and highly ordered though.  Issue is it will be a
> bunch of tiny commits which cannot stand on their own, plus one big
> atomic commit which cannot be split.  I suspect several will get squashed
> back together on commit.
> 
> This will take a bit...

I think I did not totally mean it like that. While making changes and
getting lost in the editor seeing more and more things to fix, I usually
stop, make a list of things and immediately fix them one by one in
different commits, going back and reworking all of this now (while it is
in the end for future $whoevers who are using git log and blame) is
maybe not the best way to spend your time.

But at least split it in the necessary fix with explanation for "`ldd`
doesn't work with cross-builds", so we can see what's necessary to just
fix only that problem you ran into and how and why (because this is
interesting for someone else to learn from), and then a second commit
with WRHAAAAAA REWORK ALL THE THINGS with the rest is ok. I do not want
to annoy you with a lot of work if there is more interesting things to
work on (which I think you have a list of already) :)

The difficult thing when looking at it is that there are probably
changes to e.g. restructure a loop and then there are other changes
inside the loop code and then it becomes really hard to follow and
puzzle out what exact thing belongs to what.

Oh, wait and the file dependency was also in the same commit, at least
that's a simple one to split off.

Hans