Created attachment 33389 [details] [review]
Patch for mklib to exit with error status if link failed

Created attachment 33390 [details] [review]
Patch to fix linkage of X11 libGL on cygwin

Those all look good to me, although I wish someone would go through mklib and make it exit everywhere it could fail.

Reviewed-by: Dan Nicholson <dbn.lists@gmail.com>

If Brian doesn't pick these up, I can commit them soon.

Created attachment 33393 [details] [review]
Make mklib propogate all errors

Bonus patch :-)

(In reply to comment #3)
> Those all look good to me, although I wish someone would go through mklib and
> make it exit everywhere it could fail.

This would be a good idea, the build has been broken on cygwin for some time,
but my tinderbox didn't notify me because of mklib ignoring the errors.

I did consider something like the attached, but that might not be a good idea.

I've commited patches 1, 2 and 4.

I think that the 3rd patch will result in multiply defined symbols in the normal build since libmesa.a already contains the glsl libs.  See line 32 of src/mesa/Makefile.

I think the cygwin code in mklib needs to use expand_archives() as the linux target does.

(In reply to comment #4)
> Created an attachment (id=33393) [details]
> Make mklib propogate all errors
> 
> Bonus patch :-)

Nice. Unfortunately, I think trap on ERR is an extension and won't work for many shells. It doesn't appear to be POSIX let along bourne compatible.

http://www.opengroup.org/onlinepubs/000095399/utilities/trap.html

Here's the default ubuntu/debian shell, dash:

$ trap 'echo foo' ERR
trap: 1: ERR: bad trap

I think we'll have to revert that patch and go with the brute force method of checking all the significant commands. You can do it a little simpler, though:

command that could fail || exit $?

I reverted d6f55492af3cb82b0113fe6beac0f3494b6e2956 (patch 4) since it already caused two separate build errors on the mailing list.

Created attachment 33404 [details] [review]
Cygwin linkage fix patch

(In reply to comment #5)
> I think that the 3rd patch will result in multiply defined symbols in the
> normal build since libmesa.a already contains the glsl libs.  See line 32 of
> src/mesa/Makefile.
> 
> I think the cygwin code in mklib needs to use expand_archives() as the linux
> target does.

Yes, that patch is complete nonsense.  Thanks for the clue as to what's going, updated patch attached.

I think you still need a 'rm -rf ${LIBNAME}.obj' command like the other targets.  That'll get rid of the temp directory made by the expand_archives function.

Created attachment 33442 [details] [review]
Cygwin linkage fix patch

(In reply to comment #9)
> I think you still need a 'rm -rf ${LIBNAME}.obj' command like the other
> targets.  That'll get rid of the temp directory made by the expand_archives
> function.

Doh! Updated patch attached.

OK, last patch committed.  Thanks.