Opened 11 years ago

Closed 9 years ago

Last modified 9 years ago

#40266 closed defect (fixed)

wine, wine-devel: blacklist clang compilers producing buggy binaries

Reported by: Ionic (Mihai Moldovan) Owned by: macports-tickets@…
Priority: Normal Milestone:
Component: ports Version: 2.2.0
Keywords: haspatch Cc: ryandesign (Ryan Carsten Schmidt), jyrkiwahlstedt, cooljeanius (Eric Gallager), jeremyhu (Jeremy Huddleston Sequoia)
Port: wine wine-devel wine-crossover

Description

llvm/clang < 3.2 produces buggy wine binaries, so clang >= 3.2 is required.

Blacklist old versions of clang, as well as the current one shipped by Apple, as even this is a pre-release version in between 3.1 and 3.2, still featuring this bug.

As of version 1.5.x, wine includes the winemac.drv written in Objective C, using the Cocoa API. As those header files make use of the "blocks" C language extension by Apple, FSF GCC will fail as soon as reaching winemac.drv. Thus, blacklist FSF GCC.

Lastly, prefer newer, working versions of clang over apple-gcc-4.2 to not pull in old compilers and be compatible with Apple's move towards clang.

Attachments (3)

wine.diff (1.6 KB) - added by Ionic (Mihai Moldovan) 11 years ago.
wine Portfile diff.
wine-devel.diff (1.6 KB) - added by Ionic (Mihai Moldovan) 11 years ago.
wine-devel Portfile patch.
wine-Ports.patch (14.3 KB) - added by Ionic (Mihai Moldovan) 11 years ago.
Current set of changes against wine, wine-devel and wine-crossover.

Download all attachments as: .zip

Change History (36)

Changed 11 years ago by Ionic (Mihai Moldovan)

Attachment: wine.diff added

wine Portfile diff.

Changed 11 years ago by Ionic (Mihai Moldovan)

Attachment: wine-devel.diff added

wine-devel Portfile patch.

comment:1 Changed 11 years ago by cooljeanius (Eric Gallager)

If you're using compiler.whitelist, it will make it so only those compilers can be used, making the blacklisting mostly redundant. Perhaps you want compiler.fallback-append instead?

comment:2 Changed 11 years ago by cooljeanius (Eric Gallager)

Cc: egall@… added

Cc Me!

comment:3 in reply to:  description Changed 11 years ago by ryandesign (Ryan Carsten Schmidt)

Cc: jeremyhu@… added
Port: wine-crossover added

Presumably wine-crossover is also affected?

compiler.fallback could just be set to macports-clang-3.3; no reason to list older versions since we don't want them.

comment:4 in reply to:  1 ; Changed 11 years ago by Ionic (Mihai Moldovan)

Replying to egall@…:

If you're using compiler.whitelist, it will make it so only those compilers can be used, making the blacklisting mostly redundant. Perhaps you want compiler.fallback-append instead?

Yes, I do! Thanks and sorry, changed that locally. New patch after the next issue has been discussed.

Replying to ryandesign@…:

Presumably wine-crossover is also affected?

Oh, that's compiling from source, too. Then... yes and no. wine-crossover is using compiler.whitelist gcc-4.2 apple-gcc-4.2 which is fine for now (it's not including the winemac.drv yet, thus can use FSF GCC) and clang is automatically blacklisted, but it should be made consistent with the other wine ports. So patching is in order as well.

compiler.fallback could just be set to macports-clang-3.3; no reason to list older versions since we don't want them.

The general idea was to allow/prefer both mp-clang-3.2 and mp-clang-3.3, so as to not make wine(-devel/-crossover) pull in mp-clang-3.3, if mp-clang-3.2 is already installed on the user's system. Will compiler.fall macports-clang-3.3 automatically take care of using mp-clang-3.2 if already available and ignore mp-clang-3.3? From the MP source, it looked like this isn't the case.

comment:5 Changed 11 years ago by Ionic (Mihai Moldovan)

Btw, there's a new crossover-sources build (12.5.0) available which includes wine 1.6, so I'd rather bump + include the compiler fixes at once?

comment:6 in reply to:  4 ; Changed 11 years ago by jeremyhu (Jeremy Huddleston Sequoia)

Replying to ionic@…:

Replying to ryandesign@…:

compiler.fallback could just be set to macports-clang-3.3; no reason to list older versions since we don't want them.

The general idea was to allow/prefer both mp-clang-3.2 and mp-clang-3.3, so as to not make wine(-devel/-crossover) pull in mp-clang-3.3, if mp-clang-3.2 is already installed on the user's system. Will compiler.fall macports-clang-3.3 automatically take care of using mp-clang-3.2 if already available and ignore mp-clang-3.3? From the MP source, it looked like this isn't the case.

Yes, unfortunately base isn't smart enough to pick the first installed fallback. It picks the first available (possibly available after installing it) fallback.

So since macports-clang-3.3 is already in compiler.fallback with base 2.2, the compiler.whitelist line is actually unnecessary =).

Btw, there's a new crossover-sources build (12.5.0) available which includes wine 1.6, so I'd rather bump + include the compiler fixes at once?

I still haven't gotten around to testing 12.5.0. Feel free to bump it if it works for you. wine 1.6 doesn't work for me, so I haven't checked if crossover fixed that or not in 12.5.

comment:7 in reply to:  6 ; Changed 11 years ago by Ionic (Mihai Moldovan)

Replying to jeremyhu@…:

Yes, unfortunately base isn't smart enough to pick the first installed fallback. It picks the first available (possibly available after installing it) fallback.

Ouch, that's "bad". In that case, let's pull in mp-clang-3.3 indefinitely. :/

So since macports-clang-3.3 is already in compiler.fallback with base 2.2, the compiler.whitelist line is actually unnecessary =).

True, just had a look at portconfigure.tcl. However, apple-gcc-4.2 is preferred over mp-clang-3.3, so adding compiler.fallback macports-clang-3.3 doesn't seem unnecessary if trying to avoid the old apple-gcc-4.2 port.

I still haven't gotten around to testing 12.5.0. Feel free to bump it if it works for you. wine 1.6 doesn't work for me, so I haven't checked if crossover fixed that or not in 12.5.

I'll bump and play with that locally, making sure it works before batch-uploading patches for all three wine versions later "today".

comment:8 in reply to:  6 ; Changed 11 years ago by Ionic (Mihai Moldovan)

Replying to jeremyhu@…:

wine 1.6 doesn't work for me, so I haven't checked if crossover fixed that or not in 12.5.

One more thing... what exactly doesn't work for you? I.e., what should I be looking for especially while testing?

comment:9 in reply to:  7 ; Changed 11 years ago by jeremyhu (Jeremy Huddleston Sequoia)

Replying to ionic@…:

However, apple-gcc-4.2 is preferred over mp-clang-3.3, so adding compiler.fallback macports-clang-3.3 doesn't seem unnecessary if trying to avoid the old apple-gcc-4.2 port.

If that's the case, you should add gcc-4.2 and apple-gcc-4.2 to compiler.blacklist (and I think llvm-gcc fails as well, so you can shorten that to *gcc-4.2)

comment:10 in reply to:  9 ; Changed 11 years ago by Ionic (Mihai Moldovan)

Replying to jeremyhu@…:

If that's the case, you should add gcc-4.2 and apple-gcc-4.2 to compiler.blacklist (and I think llvm-gcc fails as well, so you can shorten that to *gcc-4.2)

Blacklisting gcc-4.2 and apple-gcc-4.2 is semantically incorrect, as both versions compile sane wine binaries. I want to prefer mp-clang-3.3 over (apple-)gcc-4.2 only, not lead other developers and users into thinking those compilers won't work. (I.e., (apple-)gcc-4.2 usage should be stripped to a minimum, only when really required and new clang versions aren't working with a port, to conform with Apple's compiler policy.)

llvm-gcc on the other hand is breaking wine, yes. Those are already blacklisted. :)

comment:11 Changed 11 years ago by jeremyhu (Jeremy Huddleston Sequoia)

Ok, if you really want to do that, then I think this is what you want:

# Reorder gcc-4.2 to the back
compiler.fallback-delete gcc-4.2 apple-gcc-4.2
compiler.fallback-append gcc-4.2 apple-gcc-4.2
Last edited 11 years ago by jeremyhu (Jeremy Huddleston Sequoia) (previous) (diff)

comment:12 Changed 11 years ago by jeremyhu (Jeremy Huddleston Sequoia)

Also, the default compiler.fallback actually does choose Apple's recommended compiler as the first option based on Xcode version (although I think Apple may have been pushing for clang adoption in Xcode 4.0 and 4.1, but we started using it with 4.2)

That being said, I think you may be right regarding promoting macports-clang-3.3 over macports-llvm-gcc-4.2 in the next version of base, but I want to audit all the ports blacklists first and update any "clang" options to be versioned (if necessary) first or changed to "*clang*" if they should skip 3.3 as well. Once that's done, we can reorder those.

comment:13 in reply to:  11 ; Changed 11 years ago by Ionic (Mihai Moldovan)

Replying to jeremyhu@…:

# Reorder gcc-4.2 to the back
compiler.fallback-delete gcc-4.2 apple-gcc-4.2
compiler.blacklist-append gcc-4.2 apple-gcc-4.2

compiler.blacklist-append? Did you mean compiler.fallback-append? :)
That's a very nice idea, thanks!

Also, the default compiler.fallback actually does choose Apple's recommended compiler as the first option based on Xcode version (although I think Apple may have been pushing for clang adoption in Xcode 4.0 and 4.1, but we started using it with 4.2)

IIRC, Xcode 4.0 had llvm-gcc-4.2 as the default compiler set, same thing for 4.1. 4.2 then switched to clang as the default compiler. I guess the short llvm-gcc phase was really more for testing whether LLVM would behave well with optimized code or a lot of developers are reporting bugs.

That being said, I think you may be right regarding promoting macports-clang-3.3 over macports-llvm-gcc-4.2 in the next version of base, but I want to audit all the ports blacklists first and update any "clang" options to be versioned (if necessary) first or changed to "*clang*" if they should skip 3.3 as well. Once that's done, we can reorder those.

Sounds reasonable. I'm all for it.

Last edited 11 years ago by Ionic (Mihai Moldovan) (previous) (diff)

comment:14 in reply to:  13 Changed 11 years ago by jeremyhu (Jeremy Huddleston Sequoia)

Replying to ionic@…:

compiler.blacklist-append? Did you mean compiler.fallback-append? :)

Yes, edited ><

Also, the default compiler.fallback actually does choose Apple's recommended compiler as the first option based on Xcode version (although I think Apple may have been pushing for clang adoption in Xcode 4.0 and 4.1, but we started using it with 4.2)

IIRC, Xcode 4.0 had llvm-gcc-4.2 as the default compiler set, same thing for 4.1. 4.2 then switched to clang as the default compiler. I guess the short llvm-gcc phase was really more for testing whether LLVM would behave well with optimized code or a lot of developers are reporting bugs.

There are multiple reasons for llvm-gcc's existence, but yes it was a transitionary product.

comment:15 in reply to:  10 Changed 11 years ago by ryandesign (Ryan Carsten Schmidt)

Replying to ionic@…:

Blacklisting gcc-4.2 and apple-gcc-4.2 is semantically incorrect, as both versions compile sane wine binaries. I want to prefer mp-clang-3.3 over (apple-)gcc-4.2 only, not lead other developers and users into thinking those compilers won't work. (I.e., (apple-)gcc-4.2 usage should be stripped to a minimum, only when really required and new clang versions aren't working with a port, to conform with Apple's compiler policy.)

I don't understand why we should make the wine ports prefer clang over gcc-4.2, when the rest of MacPorts does not (on those systems where gcc-4.2 is the default).

comment:16 Changed 11 years ago by Ionic (Mihai Moldovan)

The general idea is not to pull a "useless" compiler just for wine, when every thing else is moving into the mp-clang-3.3 direction as far as fallback goes. I'm fine leaving that out, though, if you'd prefer.

comment:17 in reply to:  8 Changed 11 years ago by Ionic (Mihai Moldovan)

Replying to ionic@…:

Replying to jeremyhu@…:

wine 1.6 doesn't work for me, so I haven't checked if crossover fixed that or not in 12.5.

One more thing... what exactly doesn't work for you? I.e., what should I be looking for especially while testing?

I've bumped wine-crossover locally, put in the compiler blacklist stuff, built wine-crossover and it works fine for me (12.5.0). Can't see any problem with it?

The only difference is that there's no winecfg script wrapper, but /opt/local/bin/wine winecfg works fine, so that's no issue either.

comment:18 Changed 11 years ago by jeremyhu (Jeremy Huddleston Sequoia)

Push it then. It's openmaintainer ;)

comment:19 in reply to:  18 ; Changed 11 years ago by cooljeanius (Eric Gallager)

Replying to jeremyhu@…:

Push it then. It's openmaintainer ;)

I don't think Ionic has commit access yet, judging by his non-macports.org email address... unless he has a second email that I don't know about?

comment:20 in reply to:  19 Changed 11 years ago by Ionic (Mihai Moldovan)

Replying to egall@…:

I don't think Ionic has commit access yet, judging by his non-macports.org email address... unless he has a second email that I don't know about?

I don't, but I'm also waiting on Ryan's take regarding compiler selection. If he's not happy with it, I'll leave stuff out.

This said, I'm going to rebuild wine-crossover once more as to check something else, but afterwards I'll put a new patch revision on here.

comment:21 Changed 11 years ago by jeremyhu (Jeremy Huddleston Sequoia)

Can you attach your current set of diffs?

Changed 11 years ago by Ionic (Mihai Moldovan)

Attachment: wine-Ports.patch added

Current set of changes against wine, wine-devel and wine-crossover.

comment:22 Changed 11 years ago by Ionic (Mihai Moldovan)

Tried testing dialog intensive and DX applications, everything seems to work just fine on my 10.8 system.

comment:23 Changed 11 years ago by ryandesign (Ryan Carsten Schmidt)

I have not tried the patch but I have read it and it looks plausible. I'd like to refrain however from introducing the archcheck portgroup into the wine-crossover port. The archcheck portgroup dates from before MacPorts base had the ability to check dependencies' architectures. We should instead be removing the archcheck portgroup from the wine and wine-devel ports. Other than that, if you think the patch is correct, I can commit it (or anyone else should feel free to also).

comment:24 Changed 11 years ago by Ionic (Mihai Moldovan)

I agree regarding the archcheck stuff.

Unfortunately, my HFS+ file system is broken and I'm unable to test any changes I make, thus I won't submit new stuff. The current patchset worked fine for me, but I'd also like to remove the archcheck bits. Can't give any ETA for my system to be fully operational again. :(

comment:25 Changed 10 years ago by ryandesign (Ryan Carsten Schmidt)

I handled updating wine-crossover to a newer version in #40102.

I haven't adjusted the compiler blacklisting yet. When you say that old clang produces buggy binaries, how do I confirm that that is so? What issue do you see, and how do I reproduce it on my system (assuming I have compiled wine with an old clang)?

comment:26 in reply to:  description Changed 10 years ago by ryandesign (Ryan Carsten Schmidt)

Replying to ionic@…:

As of version 1.5.x, wine includes the winemac.drv written in Objective C, using the Cocoa API. As those header files make use of the "blocks" C language extension by Apple, FSF GCC will fail as soon as reaching winemac.drv. Thus, blacklist FSF GCC.

Confirmed build failure of wine @1.6.1 with macports-gcc-4.5 and macports-gcc-4.8; blacklisted macports-gcc-* in wine ports in r114265.

comment:27 Changed 9 years ago by Ionic (Mihai Moldovan)

comment:28 Changed 9 years ago by jeremyhu (Jeremy Huddleston Sequoia)

Port: llvm-3.2 llvm-3.3 added; wine wine-devel wine-crossover removed

I'd prefer we take the llvm changes into our compilers: http://llvm.org/bugs/show_bug.cgi?id=9707

comment:29 Changed 9 years ago by Ionic (Mihai Moldovan)

Port: llvm-3.1 wine wine-devel wine-crossover added; llvm-3.2 llvm-3.3 removed

3.2 and 3.3 are not affected.

2.9 and 3.0 are already blacklisted, because they fail to build wine.

This leaves Apple clang 3.2 and below and our 3.1 clang. If you want to backport, backport to 3.1.

But Apple clang < 426 (i.e., including 3.2, because Apple shipped a pre-release version which has the bug) has to be blacklisted in any case.

Last edited 9 years ago by Ionic (Mihai Moldovan) (previous) (diff)

comment:30 Changed 9 years ago by jeremyhu (Jeremy Huddleston Sequoia)

Port: llvm-3.1 removed

Thanks. I read too quickly and misread Mihai's comment in the wine bug. I think llvm-3.1 is old enough that we can just blacklist it. 3.2 and 3.3 are still quite heavily used even though I think 3.4 is quite mature now and 3.5 is out.

comment:31 Changed 9 years ago by jeremyhu (Jeremy Huddleston Sequoia)

Resolution: fixed
Status: newclosed

Blacklisted in r128293 and revbumped in r128294

comment:32 Changed 9 years ago by Ionic (Mihai Moldovan)

Thanks. I would have pushed that with < 426, should have looked up the versions numbers in Wikipedia (500 comes directly after 425... how intuitive.)

comment:33 Changed 9 years ago by jeremyhu (Jeremy Huddleston Sequoia)

Those are internal project version numbers and don't really have much meaning to external users other than "increasing is newer"

Note: See TracTickets for help on using tickets.