Re: [PATCH] MNG detection fails with libjpeg-turbo

The Wanderer <[email protected]> Thu, 30 Nov 2017 23:21:46 -0500
Newsgroups	gmane.comp.video.mplayer.devel
Organization	This space intentionally left blank.
Message-ID	<[email protected]>
On 2017-11-30 at 16:23, Alexander Strasser wrote:

> Hi!
> 
> On 2017-11-26 15:36 -0500, The Wanderer wrote:
>>
>> Index: configure
>> ===================================================================
>> --- configure	(revision 38007)
>> +++ configure	(working copy)
>> @@ -177,6 +177,16 @@
>>    compile_check $TMPC $@
>>  }
>>  
>> +return_statement_check_broken() {
>> +  cat > $TMPC << EOF
>> +#include <$1>
>> +#include <$2>
>> +int main(void) { $3; return $4; }
>> +EOF
>> +  shift 4
>> +  compile_check $TMPC $@
>> +}
>> +
> 
> It has nothing to do with your patch, because the scheme is already
> established in MPlayer's configure. Anyway FWIW I really dislike the
> *_check_broken naming.

I don't particularly like it myself, but it was established, and I
didn't want to break with that - or at least not as part of this patch;
if that's going to be changed at all, it should be done in a separate
commit.

I mean, I do understand the rationale behind giving it that name, and
the mindset which probably led to that being done. I'm just not sure I
necessarily agree with them.

>>  yasm_check() {
>>    echo >> "$TMPLOG"
>>    cat "$TMPS" >> "$TMPLOG"
>> @@ -5303,7 +5313,7 @@
>>  if test "$_mng" = auto ; then
>>    _mng=no
>>    for mnglibs in '-lmng -lz' '-lmng -ljpeg -lz' ; do
>> -    return_statement_check libmng.h 'const char * p_ver = mng_version_text()' '!p_ver || p_ver[0] == 0' $mnglibs && _mng=yes
>> +    return_statement_check_broken stdio.h libmng.h 'const char * p_ver = mng_version_text()' '!p_ver || p_ver[0] == 0' $mnglibs && _mng=yes
> 
> Would you be OK with omitting the function defined in the previous hunk
> and changing this to use statement_check_broken ?

I would have no objection to it, as long as we understand what it's
doing and what the consequences will be.

My primary concern is: why was this put there in the first place?
Presumably it served some purpose at some point...

> I have done the changes locally and everything seems to work as well.
> 
> I don't think the return part adds any value, because the built
> executable is never run and thus its exit status isn't evaluated.
> 
> If it was added because of fear that linking could be spared because
> of some compiler optimizations, I don't think it will happen in this
> case. Also lots of other checks don't use the return_statement_check.
> So if there is a problem, it would better be fixed for all checks.

I'd be leery of dropping code whose intended purpose we don't know or
understand, but if it works equally well without that and we make sure
that our own reasoning is findable in case someone does turn up a
problem which could be the reason later on, then I have no strong objection.

> LGTM otherwise.
> 
> I intent to commit my local version with the modified configure check
> if I hear no objections.
> 
> Thank you for looking into this. I am not using vo mng and I didn't
> notice it won't build on many systems.

I don't generally use it myself, but I have a longstanding habit (dating
back to before the days when I was actually peripherally involved in
MPlayer development myself) of building with as many optional things
enabled as possible, so whenever the configure output lists something as
"disabled" and I don't actually know _why_ it can't be enabled that
bothers me.

There are still a few things like that, but I haven't managed to make
any headway on resolving the question in those cases. In this one, it
turned out to be relatively straightforward.

-- 
   The Wanderer

The reasonable man adapts himself to the world; the unreasonable one
persists in trying to adapt the world to himself. Therefore all
progress depends on the unreasonable man.         -- George Bernard Shaw

_______________________________________________
MPlayer-dev-eng mailing list
[email protected]
https://lists.mplayerhq.hu/mailman/listinfo/mplayer-dev-eng
signature.asc (application/pgp-signature, 833 B)