Bug 519395

Created attachment 358711 [details]
proposed patch

Stepan, I agree with your proposal on adopting the new buildroot, but I don't agree on your "nice find" changes.

Rationale:
* Your proposal adds adds further external commands (pipe, xargs), where using find's internal -exec is more efficient.
* Your proposal is sensitive to white space quoting, due to using xargs
(The original construct also is, due to missing quotes)
* -size 0 complies to POSIX, -empty doesn't.

May-be you should explain which issues you are trying to solve. I don't see them.

(In reply to comment #2)
> May-be you should explain which issues you are trying to solve. I don't see
> them.

Well, I have to admit that I'm not trying to solve any real problem, I was just replacing things I did not like with "nicer" patterns.  I wanted to make the cpanspec-generated code as educative as possible, because it gets replicated many times.

Thank you, Ralf, for your comment, you made me to think more about the changes.
Rationale follows.

> I agree with your proposal on adopting the new buildroot,

Great.  Even FPG mention that this is the best of the allowed alternatives.

> * Your proposal adds further external commands (pipe, xargs), where using
> find's internal -exec is more efficient.

Generally speaking, xargs is often more efficient than -exec, as it calls the command only once for many arguments.  That's why I prefer it.
But in this case, you are right: the rm command is called ony a few times, if ever, so it is more appropriate here.

Anyway, there is a way to combine both advantages: we can use
      find ... -exec rm -f {} +
The plus terminator in find is defined by POSIX.

> * -size 0 complies to POSIX, -empty doesn't.

Right.

> * Your proposal is sensitive to white space quoting, due to using xargs

Right.  To fix it, I'd have to use "-print0|xargs -0", another non-POSIX feature.

> (The original construct also is, due to missing quotes)

Right, should be fixed.  I think that the values in @MACROS should contain quotes and then would be used unquoted.

To sum up, you convinced me to back out my changes to first two instances of "find", except for perhaps changing the -exec delimiter to +.

But I think the situation is different with the last instance of find:

   find $RPM_BUILD_ROOT -depth -type d -exec rmdir {} 2>/dev/null \;

Note that the stderr redirection does not apply to rmdir, but to the whole find command.  Consequently, any error output from find is discarded.

At the very least, we should make this clear by reordering the arguments:

   find $RPM_BUILD_ROOT -depth -type d -exec rmdir {} \; 2>/dev/null

But I should it would be better to overcome this limitation by using the non-POSIX predicate -empty (-size 0 does not work for directories).
That means I still support my original proposal.

I'll post an updated patch tomorrow.

(In reply to comment #3)
> Anyway, there is a way to combine both advantages: we can use
>       find ... -exec rm -f {} +
> The plus terminator in find is defined by POSIX.

That may be so but you still need a relatively recent version of find to use it. If cpanspec is changed to use the plus terminator, the generated spec files will be incompatible with EL-5, let alone EL-4.

(In reply to comment #4)
> >       find ... -exec rm -f {} +
...
> will be incompatible with EL-5, let alone EL-4.  

Right, so I will not use it.  Thank you for warning me.

OTOH, I have just checked that -empty (to detect an empty directory) is present on RHEL 4.5, so we can safely use it.

Well, -empty has been present on many OSes for ages (even on non-Linux system).
Nevertheless it's not part of POSIX. I.e. your patch adds a portability regression.

However, I guess, I have to emphasize my actual points:
* You are fixing anything - Don't "try to fix" what is not broken.
* You are adding new regressions,

(In reply to comment #6)
> * You are fixing anything - Don't "try to fix" what is not broken.
> * You are adding new regressions,  

That is a good angle of view, and you convinced me to back out some of the originally proposed changes.

But the command
   find $RPM_BUILD_ROOT -depth -type d -exec rmdir {} 2>/dev/null \;
is very confusing and its current state.
The first impression is, naturally, that it means "run rmdir with stderr redirected to /dev/null".  Only after some concentration, the reader realizes that the redirection applies to the whole find command.  Changing the position of the redirection is the minimum that should be done (see the end of comment #3).

But, still, running "find 2>/dev/null" is not nice.
What can be done about it?

It would be nice if we were able to instruct find to execute rmdir redirecting its stderr; but all ways I can imagine are too complicated.

So I want to do a trade: sacrifice a tiny bit of portability in order to see the error messages from find.  Does that sound fair?

Created attachment 359104 [details]
proposed patch 2

This updated patch contains 3 fixes:
1) better buildroot
2) buildroot now can contains spaces -- see the middle of comment #3
3) "find 2>/dev/null" fix -- see end of comment #3 and comment #7

Is this patch OK?

This bug appears to have been reported against 'rawhide' during the Fedora 12 development cycle.
Changing version to '12'.

More information and reason for this action is here:
http://fedoraproject.org/wiki/BugZappers/HouseKeeping

This message is a reminder that Fedora 12 is nearing its end of life.
Approximately 30 (thirty) days from now Fedora will stop maintaining
and issuing updates for Fedora 12.  It is Fedora's policy to close all
bug reports from releases that are no longer maintained.  At that time
this bug will be closed as WONTFIX if it remains open with a Fedora 
'version' of '12'.

Package Maintainer: If you wish for this bug to remain open because you
plan to fix it in a currently maintained version, simply change the 'version' 
to a later Fedora version prior to Fedora 12's end of life.

Bug Reporter: Thank you for reporting this issue and we are sorry that 
we may not be able to fix it before Fedora 12 is end of life.  If you 
would still like to see this bug fixed and are able to reproduce it 
against a later version of Fedora please change the 'version' of this 
bug to the applicable version.  If you are unable to change the version, 
please add a comment here and someone will do it for you.

Although we aim to fix as many bugs as possible during every release's 
lifetime, sometimes those efforts are overtaken by events.  Often a 
more recent Fedora release includes newer upstream software that fixes 
bugs or makes them obsolete.

The process we are following is described here: 
http://fedoraproject.org/wiki/BugZappers/HouseKeeping

Fedora 12 changed to end-of-life (EOL) status on 2010-12-02. Fedora 12 is 
no longer maintained, which means that it will not receive any further 
security or bug fix updates. As a result we are closing this bug.

If you can reproduce this bug against a currently maintained version of 
Fedora please feel free to reopen this bug against that version.

Thank you for reporting this bug and we are sorry it could not be fixed.