wine*: Clean up Portfiles, dependencies and add some patches #374

casr · 2017-03-08T13:08:42Z

Description

After trying to help out with Trac#53244 I noticed that the Portfile had fallen behind with respect to how later versions of Wine were being built. As I got through it, I noticed some unnecessary dependencies and factored out X11 as a variant (there is now a native “Mac Driver”). A couple of patches have been added to fix various behaviour issues on macOS.

There are many small commits below as I thought with this many changes as much documenting would be wise so others can understand the reasoning behind each small change.

Some unresolved things:

As X11 is no longer a requirement, I believe that Wine would be better suited in the category ‘emulators’ (Yes, I’m aware what Wine stands for... ;)
Check on macOS versions below 10.12. Can buildbot take care of this?
X11 still remains on by default but maybe this is unnecessary.
When trace mode is enabled, MacPorts picks up that the ports will try and access git and that it is not listed as a build dependency. It seems that it is just used by Wine for outputting the version information but it is able to fallback to a predefined variable without Git, therefore, I am just ignoring this.

/cc relevant maintainers @ryandesign @jyrkiwahlstedt @jeremyhu

Tested on

macOS 10.12.3
Xcode 8.2.1

Verification

Have you

followed our Commit Message Guidelines?
checked that there aren't other open pull requests for the same change?
referenced existing tickets on Trac with full URL?
checked your Portfile with port lint?
tried a full install with sudo port -vst install?
tested basic functionality of all binary files?

casr · 2017-03-21T11:31:35Z

Resolved the upstream conflict with wine-devel's version bump.

casr · 2017-04-02T11:28:09Z

Resolved the upstream conflict with wine-devel and wine-crossover's version bump.

@ryandesign @jyrkiwahlstedt if you have any thoughts about getting this merged I would be happy to fix things up.

mojca

Your patches also concern the following tickets (in case you might want to mention that in commit messages):

Thanks a lot for taking so much work and for submitting these extensive patches. I hope that Ryan will have time to review them. They mostly look OK to me, but they should of course be tested on older OSes.

I hope you will also take a look at 64-bit support https://trac.macports.org/ticket/53651 ;)

mojca · 2017-04-14T02:13:42Z

x11/wine-crossover/Portfile

-                            --with-cups \
-                            --with-curses \
-                            --with-fontconfig \
-                            --with-freetype \


I probably understand your concerns, but why is this particular change needed?

MacPorts generally tries to create as reproducible builds as possible. Though that usually means using lots of --without-something and --disable-somethingelse to make sure that libraries are not opportunistically linked. I guess the flags above don't make any real difference, but ...

I guess you're always going to run that risk with either approach but that's where the trace option can help out.

I prefer that we tell the build system everything we do not want then enable optional extras. The rest of the procedure should then let the Wine developers adjust the parameters as they see fit (they should be wiser than us port maintainers ;) ) and we need to be vigilant about enabling/disabling dependencies as required.

mojca · 2017-04-14T02:29:25Z

x11/wine-crossover/Portfile

+                              --x-lib=${prefix}/lib
+}
+
+default_variants            +x11


I would be in favour of not making this a default variant (if it works flawlessly). But see also problems reported in https://trac.macports.org/ticket/49800.

I don't know whether we need a +quartz variant or not.

I thought it could still be useful to have it be built by the buildbot. The MacDriver is still the preferred way that it launches so you would have to explicitly ask for the X11 one to load. Of course, if you're building from scratch it is nice to not have to pull in all the X11 dependencies.

Not sure if +quartz would be a necessity, at least, not now. Maybe if in the future macOS gets a third output option then perhaps?

Ah, OK, if +x11 builds both GUIs, then this is even better. I thought that building with X11 excluded the native GUI and vice versa. In that case keeping X11 makes sense.

mojca · 2017-04-14T05:01:03Z

The port wants perl5.24 +universal which is most likely not necessary.

Full list of dependencies being asked for (I had wine-devel already installed from earlier):

 fftw-3
 fftw-3-single
 flac
 libsndfile
 openal-soft
 p5.24-xml-parser
 perl5.24
 portaudio
 pulseaudio
 soxr
 speexDSP

casr · 2017-04-14T09:05:40Z

I hope you will also take a look at 64-bit support https://trac.macports.org/ticket/53651 ;)

In case you missed it, I made the patch for that to work ;) I was waiting until this got merged in before producing a final patch.

The port wants perl5.24 +universal which is most likely not necessary.

Full list of dependencies being asked for (I had wine-devel already installed from earlier):
fftw-3
fftw-3-single
flac
libsndfile
openal-soft
p5.24-xml-parser
perl5.24
portaudio
pulseaudio
soxr
speexDSP

Do you mean these are additional direct dependencies that were not in the Porfile? I thought I check that all out but it has been quite a while now so I am not sure!

Finally, thank you for finding and linking all the relevant Trac tickets! I've gone and updated the commit messages :)

mojca · 2017-04-14T09:43:07Z

Do you mean these are additional direct dependencies that were not in the Porfile?

No, what I mean is that I already have wine-devel installed, so most dependencies should be installed already. You added a dependency on, say, openal-soft, so it's expected that your port pulls in a few additional dependencies (openal-soft and some of its dependencies). However I see no reason why perl5.24 or p5.24-xml-parser would have to be built with +universal unless wine or one of the libraries actually links against libperl.dylib. We need to figure out how to best prevent that. I simply listed all dependencies that were still missing on my machine just for double-checking if all of them make sense.

And sorry for not pay attention about the 64-bitness (I just went through the tickets).

casr · 2017-04-14T10:33:47Z

However I see no reason why perl5.24 or p5.24-xml-parser would have to be built with +universal unless wine or one of the libraries actually links against libperl.dylib.

Oh, I see. Yes, perhaps that extra configurability would be nice but I think it may also complicate things a whole lot. Could be tricky!

And sorry for not pay attention about the 64-bitness (I just went through the tickets).

Haha, not at all! I'm very grateful you found all those tickets I missed. And cross-linking it here should help some others out too :)

mojca · 2017-04-14T11:08:34Z

I would also suggest creating subports rather than having to keep three independent ports fully in sync (I happily volunteer to do that), but it's probably best if this PR gets merged first.

ryandesign · 2017-04-14T11:11:02Z

I would also suggest creating subports rather than having to keep three independent ports fully in sync (I happily volunteer to do that), but it's probably best if this PR gets merged first.

I'm not a big fan of subports being used in that manner. There are often differences in development versions, and especially in the crossover fork, that are handled well enough as separate ports.

ryandesign · 2017-04-14T11:12:11Z

However I see no reason why perl5.24 or p5.24-xml-parser would have to be built with +universal unless wine or one of the libraries actually links against libperl.dylib. We need to figure out how to best prevent that.

The only thing that occurs to me is depends_skip_archcheck but I don't know if it will actually prevent universal installation of those dependencies. If not, then MacPorts base doesn't have this feature yet.

mojca · 2017-04-14T11:13:12Z

depends_skip_archcheck is exactly what I wanted to suggest. It didn't work out-of-the-box for me for some reason, but I assume there is some straightforward way. Most likely one of the dependencies should declare that.

I suspect bison.

mojca · 2017-04-14T12:45:53Z

A build on 10.7 breaks some time pretty late in the build process:

/opt/local/bin/clang-mp-3.4 -m32 -c -o macro.lex.yy.o macro.lex.yy.c -I. -I../../include -D__WINESRC__ -D_REENTRANT -fPIC -Wall -pipe \
  -fno-strict-aliasing -Wdeclaration-after-statement -Wempty-body -Wignored-qualifiers \
  -Wstrict-prototypes -Wtype-limits -Wvla -Wwrite-strings -Wpointer-arith -fno-omit-frame-pointer \
  -I/opt/local/include -pipe -Os -arch i386 -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=0
../../tools/winegcc/winegcc -o winhlp32.exe.so -B../../tools/winebuild -m32 -fasynchronous-unwind-tables \
  -mwindows callback.o hlpfile.o macro.o string.o winhelp.o macro.lex.yy.o winhlp32.res -lshell32 \
  -lcomctl32 -lcomdlg32 -luser32 -lgdi32 ../../libs/port/libwine_port.a -Wb,-dshell32 -Wb,-dcomctl32 \
  -Wb,-dcomdlg32 -L/opt/local/lib -Wl,-headerpad_max_install_names -arch i386
Undefined symbols for architecture i386:
  "_yywrap", referenced from:
      _yylex in macro.lex.yy.o
ld: symbol(s) not found for architecture i386
clang: error: linker command failed with exit code 1 (use -v to see invocation)
winegcc: /opt/local/bin/clang-mp-3.4 failed
make[1]: *** [winhlp32.exe.so] Error 2
make[1]: Leaving directory `/path/to/wine-devel/work/wine-2.5/programs/winhlp32'
make: *** [programs/winhlp32] Error 2
make: *** Waiting for unfinished jobs....

But manually adding -lfl seems to help.

mojca · 2017-04-14T13:21:35Z

Indeed, manually adding -lfl ended up in successfully installing wine. Apparently some Makefile needs patching somewhere.

I still get random problems, but I don't think it is any worse than it was (there are some tickets on Trac):

$ wine test.exe 
Hello world.
$ wine: Unhandled page fault on read access to 0x00000000 at address 0x99bf4101 (thread 0032), starting debugger...
wine client error:32: write: Bad file descriptor
err:ntdll:RtlLeaveCriticalSection section 0x110940 is not acquired

Meanwhile wine 2.6 has been released.

Ionic · 2017-04-15T05:48:13Z

Just a general piece of advise: be careful with removing dependencies. WINE likes to load libraries dynamically, so unless you know the code and what might be used exactly, it's quite difficult to determine the real dependencies. otool won't really help.

casr · 2017-04-17T20:53:30Z

@Ionic oh no! Has something broken for you?

I read through the autotools code and the current build instructions as well as trying some non-trivial programmes to makes sure everything was working. I assumed that if I had removed too much the issue would be mentioned at the PR stage or very soon after merge and that it would be better to be more strict than loose.

casr · 2017-04-17T21:06:10Z

A build on 10.7 breaks some time pretty late in the build process:
…
But manually adding -lfl seems to help.

@mojca Was that broken before this PR? I haven't changed anything directly with that flag…

mojca · 2017-04-18T00:05:22Z

No, it wasn't broken before this PR. But that's either a new bug in wget's Makefiles or an old bug that has been exposed due to slightly changed configure options. It should be reported upstream.

Ionic · 2017-04-20T17:42:44Z

No, that was just a general comment. I haven't tried building and using your branch (also because I don't have a complex wine testing suite or the like...)

libflex breakages are probably caused by the updated flex port, since the current release of flex has a bug, but IIRC an important patch was backported so that software using flex shouldn't break any longer.

casr · 2017-04-22T14:09:56Z

Resolved the conflict with x11/wine/Portfile being bumped to 2.0.1. I removed the DXTC-to-ARGB patch as that has been included in 2.0.1's list of fixes.

FYI, I haven't done a new build for this.

mojca · 2017-04-25T09:54:14Z

@casr: In the meantime @ryandesign updated wine-devel to version 2.6, so this PR needs to be fixed.

@ryandesign: Any thoughts about these two wine-related PRs? I see that flex was reverted to an older version, so maybe my problem is gone as well, I need to check.

casr · 2017-04-25T10:06:16Z

@mojca resolved the conflict :)

casr · 2017-04-25T10:18:09Z

The only conflicts that are occurring are because of the fact that I bumped the revision number. As this PR seems in it for the long haul, I've just gone ahead and deleted that commit.

It will, of course, need to be reintegrated if this ever gets merged.

ryandesign · 2017-04-25T19:24:47Z

I am not particularly involved with GitHub pull requests, but taking a look at this now:

I don't know why you are reformatting the portfile comments.

I don't know why you are removing the comment that most libraries are dlopen'd. Can you provide a citation that shows this is no longer true?

Not using fontconfig seems good, assuming that works. I don't know if anyone will complain or will want a fontconfig variant to be available; if so we can add that later.

Removing build.target seems fine.

I guess "removing unnecessary flags" is fine.

The whitespace in the x11 variant does not conform to the modeline.

Updating the description is fine.

Removing the Tiger patch is fine since support for Tiger seems long gone.

Fullscreen flickering patch is fine I guess.

Removing configure flags that disable pulseaudio and udev: If these options/features are no longer supported then that's fine.

I disagree with removing the configure flags that make explicit what options we are enabling or disabling, even if they are the default behavior of the configure script. We want to be explicit, and we need the list of dependencies to match. If upstream decided to turn a feature on or off by default in a new version, we would either have unnecessary dependencies or opportunistically-used dependencies.

Adding openal-soft support is fine if that works.

The notes are hard wrapped; please allow MacPorts to line wrap to the width of the terminal. Also observe the statements about indentation made by the modeline.

The patchfiles should not be renamed. The old "patch-XXX.diff" patchfile naming scheme advocated by port lint seems to be falling out of favor, with "XXX.patch" taking its place. We need to update the lint warning to allow either form.

Since this PR changes installed files, a revision increase will be needed.

I'm not merging pull requests but the ports are openmaintainer so someone else can merge it when it's ready.

casr · 2017-04-26T08:25:47Z

@ryandesign Thank you for reviewing the commits.

I don't know why you are reformatting the portfile comments.

I don't know why you are removing the comment that most libraries are dlopen'd. Can you provide a citation that shows this is no longer true?

Agree on both points; I should not have made those changes. I have undone that.

The whitespace in the x11 variant does not conform to the modeline.

I have pushed it to 4 spaces.

I disagree with removing the configure flags that make explicit what options we are enabling or disabling, even if they are the default behavior of the configure script. We want to be explicit, and we need the list of dependencies to match. If upstream decided to turn a feature on or off by default in a new version, we would either have unnecessary dependencies or opportunistically-used dependencies.

Okay, so I've reverted that commit but then also added in all the other missing options. I have taken a best guess at the defaults but if there is a better way of establishing what ./configure eventually decides upon I would love to know it. (Demystifying automake scripts is not fun…). Wine's configure script seems to ignore --with/--without options on some platforms as well as I think is the case with OpenGL on macOS, for instance.

The notes are hard wrapped; please allow MacPorts to line wrap to the width of the terminal. Also observe the statements about indentation made by the modeline.

Oops! Fixed.

The patchfiles should not be renamed. The old "patch-XXX.diff" patchfile naming scheme advocated by port lint seems to be falling out of favor, with "XXX.patch" taking its place. We need to update the lint warning to allow either form.

Undone.

casr · 2017-04-29T21:26:29Z

Resolved wine-devel's bump to 2.7

casr · 2017-05-06T17:43:56Z

I'm not merging pull requests but the ports are openmaintainer so someone else can merge it when it's ready.

@mojca considering the above, perhaps you would be able to merge this in?

On Darwin, fontconfig has been turned off in favour of using Core Text since Wine 1.5.10. See Wine commit 9cb7a97981 Closes: https://trac.macports.org/ticket/34745

This appears to have been there from the dawn of this Portfile but I have found no mention of it as a dependency in Wine's documentation or when searching the Wine source archives for 'expat' or 'xmlwf' (git log -S).

Wine uses the native Security framework on macOS since 1.3.16 See Wine commit 45db3481f4d

Default build instructions since Wine 1.2 have been just 'make'. Before that it is was 'make depend'. See Wine commit 954514ec92

All of these things are handled by Wine's autoconf script. See Wine commit cd454fdc2ed for CoreServies See Wine commit 11ca05f6aea for zlib See Wine commit cc8eb6b7750 for Xcode broken compiler and Win16 See Wine commit 5ddaf34d69f for -Wl -no_pie

Closes: https://trac.macports.org/ticket/49800 Closes: https://trac.macports.org/ticket/39085

We already raise an error when Wine is fetched on systems less than 10.5 so this patch is unnecessary.

See: https://bugs.winehq.org/show_bug.cgi?id=34166

Wine 1.7.51 added support for XAudio2

Re: https://trac.macports.org/ticket/53343

See: #374 (comment) See: #374 (comment)

mojca · 2017-06-30T22:01:30Z

Can you grant permission for committers to change your branch?

See: #374 (comment) See: #374 (comment)

casr · 2017-07-01T11:08:21Z

@mojca Thank you for merging this :)

raimue assigned ryandesign and jyrkiwahlstedt Mar 16, 2017

mojca reviewed Apr 14, 2017

View reviewed changes

mojca added type: bugfix type: enhancement labels Apr 14, 2017

casr mentioned this pull request Apr 22, 2017

wine*: add 64-bit support #442

Closed

6 tasks

casr added 14 commits May 13, 2017 21:00

wine*: Remove fontconfig dependency from Darwin

2331a8e

On Darwin, fontconfig has been turned off in favour of using Core Text since Wine 1.5.10. See Wine commit 9cb7a97981 Closes: https://trac.macports.org/ticket/34745

wine*: Remove unused expat dependency

277f842

This appears to have been there from the dawn of this Portfile but I have found no mention of it as a dependency in Wine's documentation or when searching the Wine source archives for 'expat' or 'xmlwf' (git log -S).

wine*: Remove gnutls dependency from Darwin

428194e

Wine uses the native Security framework on macOS since 1.3.16 See Wine commit 45db3481f4d

wine*: Default make target is 'all'

63c4693

Default build instructions since Wine 1.2 have been just 'make'. Before that it is was 'make depend'. See Wine commit 954514ec92

wine*: X11 is an optional dependency

d002523

Closes: https://trac.macports.org/ticket/49800 Closes: https://trac.macports.org/ticket/39085

wine*: Update description

a5f9f5d

wine*: Remove Tiger patch

e4800a7

We already raise an error when Wine is fetched on systems less than 10.5 so this patch is unnecessary.

wine, wine-devel: Patch for full screen flickering

718bf4a

See: https://bugs.winehq.org/show_bug.cgi?id=34166

wine*: Remove PulseAudio and udev from configure

c738e31

wine*: Add openal-soft for XAudio2 support

302e6ce

Wine 1.7.51 added support for XAudio2

wine*: Add note about high DPI monitors

e4a5d62

Re: https://trac.macports.org/ticket/53343

wine*: Be explicit with requested flags

3c9aa61

See: #374 (comment) See: #374 (comment)

wine*: Bump revision

bebe4dc

mojca pushed a commit that referenced this pull request Jul 1, 2017

wine*: Be explicit with requested flags

4281bdd

See: #374 (comment) See: #374 (comment)

mojca closed this in 3daa6c7 Jul 1, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

wine*: Clean up Portfiles, dependencies and add some patches #374

wine*: Clean up Portfiles, dependencies and add some patches #374

casr commented Mar 8, 2017 •

edited

casr commented Mar 21, 2017

casr commented Apr 2, 2017 •

edited

mojca left a comment

mojca Apr 14, 2017

casr Apr 14, 2017

mojca Apr 14, 2017 •

edited

casr Apr 14, 2017

mojca Apr 14, 2017

mojca commented Apr 14, 2017 •

edited

casr commented Apr 14, 2017 •

edited

mojca commented Apr 14, 2017 •

edited

casr commented Apr 14, 2017

mojca commented Apr 14, 2017

ryandesign commented Apr 14, 2017

ryandesign commented Apr 14, 2017

mojca commented Apr 14, 2017 •

edited

mojca commented Apr 14, 2017 •

edited

mojca commented Apr 14, 2017

Ionic commented Apr 15, 2017

casr commented Apr 17, 2017

casr commented Apr 17, 2017

mojca commented Apr 18, 2017

Ionic commented Apr 20, 2017

casr commented Apr 22, 2017

mojca commented Apr 25, 2017

casr commented Apr 25, 2017

casr commented Apr 25, 2017

ryandesign commented Apr 25, 2017

casr commented Apr 26, 2017

casr commented Apr 29, 2017

casr commented May 6, 2017

mojca commented Jun 30, 2017

casr commented Jul 1, 2017

wine*: Clean up Portfiles, dependencies and add some patches #374

wine*: Clean up Portfiles, dependencies and add some patches #374

Conversation

casr commented Mar 8, 2017 • edited

Description

Tested on

Verification

casr commented Mar 21, 2017

casr commented Apr 2, 2017 • edited

mojca left a comment

Choose a reason for hiding this comment

mojca Apr 14, 2017

Choose a reason for hiding this comment

casr Apr 14, 2017

Choose a reason for hiding this comment

mojca Apr 14, 2017 • edited

Choose a reason for hiding this comment

casr Apr 14, 2017

Choose a reason for hiding this comment

mojca Apr 14, 2017

Choose a reason for hiding this comment

mojca commented Apr 14, 2017 • edited

casr commented Apr 14, 2017 • edited

mojca commented Apr 14, 2017 • edited

casr commented Apr 14, 2017

mojca commented Apr 14, 2017

ryandesign commented Apr 14, 2017

ryandesign commented Apr 14, 2017

mojca commented Apr 14, 2017 • edited

mojca commented Apr 14, 2017 • edited

mojca commented Apr 14, 2017

Ionic commented Apr 15, 2017

casr commented Apr 17, 2017

casr commented Apr 17, 2017

mojca commented Apr 18, 2017

Ionic commented Apr 20, 2017

casr commented Apr 22, 2017

mojca commented Apr 25, 2017

casr commented Apr 25, 2017

casr commented Apr 25, 2017

ryandesign commented Apr 25, 2017

casr commented Apr 26, 2017

casr commented Apr 29, 2017

casr commented May 6, 2017

mojca commented Jun 30, 2017

casr commented Jul 1, 2017

casr commented Mar 8, 2017 •

edited

casr commented Apr 2, 2017 •

edited

mojca Apr 14, 2017 •

edited

mojca commented Apr 14, 2017 •

edited

casr commented Apr 14, 2017 •

edited

mojca commented Apr 14, 2017 •

edited

mojca commented Apr 14, 2017 •

edited

mojca commented Apr 14, 2017 •

edited