[MPlayer-cvslog] r34203 - in trunk: Changelog DOCS/tech/crosscompile.txt DOCS/tech/mingw-crossc

[Reimar Döffinger]

On Sun, Oct 16, 2011 at 10:09:06AM +0200, Ingo Brückl wrote:
> Reimar Döffinger wrote on Sat, 15 Oct 2011 21:45:41 +0200:
> 
> >> Author: ib
> >> Date: Sat Oct 15 15:20:52 2011
> >> New Revision: 34203
> >>
> >> Log:
> >> Port to Wine.
> 
> > I am not really happy about having all these wildly different things in
> > one single commit, particularly since I do not see any reason for them
> > to be.
> 
> Shall I revert? If so, which changes would you like to be in separate
> commits?

No, that does not really improve things.

> Having all changes necessary for the Wine port in one single commit made
> sense to me, but I see that Makefile and loader/com.h are not closely related
> to that.

No, merging things into a single commit unless necessary rarely makes
sense, it makes it harder to review, to do regression testing, means a
huge commit message that makes it bothersome to use svn blame to connect
some code to a _specific_ feature.

> > The osdep/priority.c seems like a code correctness change that can be
> > motivated without any need for wine to come in.
> 
> How to explain the defined(__WINE__) then?

I somehow managed to misread the diff.
I think the right fix for this is to use
#ifndef __OS2__

> >> +  ./configure --target=$(uname -m)-wine --cc=winegcc --windres=wrc
> 
> > That doesn't work on 64 bit Linux. I have not finished compiling yet,
> > but I think the only sensible command to suggest is
> > ./configure --target=i686-wine --cc="winegcc -m32" --windres=wrc
> 
> I'm not familiar with 64 bit Linux or non-Intel CPUs, but isn't there a
> 64-bit Wine as well and doesn't Wine run even on some non-Intel CPUs?

Difficult question but I'd answer it with
1) in general: "not really"
2) for Debian specifically: "no"

> What if my CPU isn't an i686, but an i[345]86?

Honestly, who cares about the exactly 0 people who will want to compile
MPlayer for wine on something older than a P3?
The number of users of 64 bit Linux is certainly higher.
But i386 should work just fine here for all of these.
For someone trying wine on e.g. PPC - well, I would expect them to be
able to figure out how to change that if they managed to get Wine to
work there...

> If really necessary, the "-m32" should be in the extra_cflags in the
> configure script.

Then you'd also have to add it to ldflags. And maybe asmflags if we
split that out (no idea). I don't see the point in going with a
difficult solution if the simple one works.
However this brings me to a related point: I doubt it is wise to
even give a command-line without --enable-gui.
Wine will _not_ be useful to test almost anything except the
_functionality_ of the GUI, unlike Wine it can't be really used to
test compilation for Windows since it won't be possible to detect
usage of e.g. Linux-only functionality.

> >> +#ifdef __WINE__
> >> +/**
> >> + * @brief Convert a Windows style file name into an Unix style one.
> >> + *
> >> + * @param filename pointer to the file name to be converted
> >> + *
> >> + * @return pointer to the converted file name
> 
> > You should say what the point of that code is.
> 
> That would be the first time to document the point of a function in addition
> to describing what it does, but I've added a comment where it is called.

Because this amount of code for such a fringe use is rather
questionable.
Without justification included in the code I wouldn't give it long time
to live.

> Is this the right place to explain Wine's drive mapping or why a Windows
> program can have Windows style file name arguments?

I doubt it. The more interesting explanation is why we care.
The wine port has really limited usefulness, thus such general
arguments are really quite irrelevant, the question is why do we
want this code.