[Rpm-maint] [rpm-software-management/rpm] scripts/brp-strip: Rectify parallel stripping logic (PR #2149)

[Florian Festi]

> Thanks @ffesti for the inputs. Find my responses inline.
> 
> > This patch has several obvious issues:
> > [sshedi]: Yes indeed. I'm open for inputs
> 
> ```
> > Still runs everything in parallel when the sort -u obviously needs to be run as single process
> ```
> 
> [sshedi]: Yes, agreed. But since we are processing in batch of files and if elf is stripped, sed command will eliminate it. So sort command won't get files which are already stripped and if a file is stripped once, it won't reach till sort command in pipeline.

This is still prone to race conditions. Especially as the front part of the pipe line will be able to process much more files more quickly than the end.

> > number of arguments changed for no reason and without mentioning it in the commit message
> ```
> 
> [sshedi]: I believe you are talking about `-n256` option here. We can pass up to 2MB size in argument, so we can use 256 here. Considering PATH_MAX is 4096, 4096 * 256 = 1MB so there is still 1MB left. I will mention this in commit message.
> 
> ```
> ➜ $ getconf ARG_MAX
> 2097152 (2MB)
> ```
> 
> ```
>     > this basically disables the parallel processing and is the reason this change "fixes" the issue
> ```
> 
> [sshedi]: Can you please elaborate on this? How does this disable parallel processing?

Well, you have at most `number of files` / `maxargs` processes. If `maxargs` is bigger then the number of files there is only one process. `-n32` was chosen as a compromise between reducing the overhead of starting new processes and distributing the work load evenly over as much processors as possible. If you are testing with less than 256 files you are not testing parallel processing at all. The maximum allowable size of the arguments is not te limiting factor here.

> ```
> > the backslash escape for the ! of the find command disappeared for no reason which is probably causing the failure
> ```
> 
> [sshedi]: the ! is redundant in this case and it works perfectly fine without it, no need to escape it because we running in batch mode and not interactive mode. Correct me if I'm wrong here.

turns out the escaping is indeed not needed. This still makes me a bit uneasy.
 
> > and that's without me even trying to run it...
> > [sshedi]: Please give it a run :)
> 
> Thanks for reviewing.



-- 
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/2149#issuecomment-1211866129
You are receiving this because you are subscribed to this thread.

Message ID: <rpm-software-management/rpm/pull/2149/c1211866129 at github.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rpm.org/pipermail/rpm-maint/attachments/20220811/9cec1ea1/attachment.html>