[FFmpeg-devel] [PATCH] Alternative OS/2 patch

[Dave Yeo]

On 11/27/07 02:34 am, Diego Biurrun wrote:
> On Mon, Nov 26, 2007 at 06:11:54PM -0800, Dave Yeo wrote:
>> On 11/26/07 04:53 pm, Diego Biurrun wrote:
>>> On Fri, Nov 23, 2007 at 09:58:42PM -0800, Dave Yeo wrote:
>>>> On 11/08/07 09:16 pm, Dave Yeo wrote:
>>>>> Attached patch cleans up some of the nasty escaping by putting the 
>>>>> DESCRIPTION field into a separate variable. This has the extra advantage 
>>>>> that it makes it simpler to change the description. (adding things like 
>>>>> vendor or bldlevel info).
>>>>> Also makes config.mak more readable :)
>>> The DESCRIPTION variable is only used once.  How does that make things
>>> simpler?
>> The DESCRIPTION needs to be quoted (according to the docs with single 
>> quotes though double quotes do work) so we need more escaping for the 
>> quotes to be written to the DEF file. Separating it allows only the 
>> DESCRIPTION variable to contain escapes. It also makes it simpler to expand 
>> the DESCRIPTION to be expanded by anyone who is distributing FFmpeg.
> 
> I am against moving stuff into variables that is only used once.

Well simplest is just to remove it. Patch looks cleaner and it is easy 
enough for someone to add it back if they want to use the DESCRIPTION field.
Better to have the versioning in the file name anyways, see attached patch.
> 
>> --- configure	(revision 11093)
>> +++ configure	(working copy)
>> @@ -1217,6 +1219,35 @@
>>          ;;
>> +    os/2*)
>> +        DESCRIPTION="'\"\$(SLIBNAME_WITH_VERSION)\"'"
>> +        SHFLAGS='$(FULLNAME).def -Zdll -Zomf'
>> +        SLIB_CREATE_DEF_CMD='echo LIBRARY $(FULLNAME) INITINSTANCE TERMINSTANCE > $(FULLNAME).def; \
>> +          echo DESCRIPTION $(DESCRIPTION) >> $(FULLNAME).def; \
>> +          echo PROTMODE >> $(FULLNAME).def; \
>> +          echo CODE PRELOAD MOVEABLE DISCARDABLE >> $(FULLNAME).def; \
>> +          echo DATA PRELOAD MOVEABLE MULTIPLE NONSHARED >> $(FULLNAME).def; \
>> +          echo EXPORTS >> $(FULLNAME).def; \
>> +          emxexp -o $(OBJS) >> $(FULLNAME).def'
>> +        SLIB_EXTRA_CMD='emximp -o $(LIBPREF)$(FULLNAME)_dll.a $(FULLNAME).def; emximp -o $(LIBPREF)$(FULLNAME)_dll.lib $(FULLNAME).def'
>> +        SLIB_INSTALL_EXTRA_CMD='install -m 644 $(LIBPREF)$(FULLNAME)_dll.lib $(LIBPREF)$(FULLNAME)_dll.a "$(LIBDIR)"'
>> +        SLIB_UNINSTALL_EXTRA_CMD='rm -f "$(LIBDIR)"/$(LIBPREF)$(FULLNAME)_dll.lib;rm -f "$(LIBDIR)"/$(LIBPREF)$(FULLNAME)_dll.a'
> 
> nit: Please keep the same order of _dll.lib and _dll.a in the last three
> lines.

Fixed in attached patch.
> 
> I am starting to think that we should introduce a variable for DEF
> files, MinGW could use it as well.

Generally in OS/2 only makefiles (and some Windows makefiles) you'd have 
a rule something like DEFFILE to create the def file. Not really sure 
how a variable would work as currently we need to run commands to 
extract the exports from the objects. I'd imagine there are also similar 
tools available for Windows but since GCC seems to handle it fine right 
now...
The main reason to use a DEF file on platforms that don't need it would 
be to limit the exports to public ones. Some projects come with DEF 
files defined for this reason, eg see zlib.
Anyways the talk of libossupport reminded me of how the patch was 
broken, it depended on short library names. Stupid 8.3 limitation for 
the kernel DLL loader, something OS/2 developers routinely bitch about 
especially since ver 2.0 handled longname DLLs fine.
Anyways tweeked the patch to handle longer names and at the same time 
added $LIBMAJOR to the DLL that we link against. Should be good until 
the major version number gets to 100 :) Only small drawback is it non 
obvious which DLL gets linked against.
The only other conflict is right now I'm exporting by ordinal number so 
adding an export (unless at the end) will break the dll. Exporting by 
name would allow a newer DLL to replace an older one and keep on working 
as long as the API is stable during a major version.

> 
> Otherwise the patch looks OK.
> 
> Diego

Dave
-------------- next part --------------
A non-text attachment was scrubbed...
Name: alt_os2.diff
Type: text/x-patch
Size: 3290 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20071203/55f0b705/attachment.bin>