Bug 577951 - Review Request: mingw-wine-gecko - MinGW Gecko library required for Wine
Review Request: mingw-wine-gecko - MinGW Gecko library required for Wine
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Michael Cronenworth
Fedora Extras Quality Assurance
:
: 675422 (view as bug list)
Depends On:
Blocks: 573530
  Show dependency treegraph
 
Reported: 2010-03-29 16:13 EDT by Andreas Bierfert
Modified: 2012-03-21 14:16 EDT (History)
13 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2012-03-21 14:16:51 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
mike: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Andreas Bierfert 2010-03-29 16:13:52 EDT
Spec URL: http://fedora.lowlatency.de/review/mingw32-wine-gecko.spec
SRPM URL: http://fedora.lowlatency.de/review/mingw32-wine-gecko-1.0.0-3.fc13.src.rpm
Description: MinGW Gecko library required for Wine
Comment 2 Erik van Pienbroek 2010-03-29 16:44:39 EDT
Hi,

I'll leave the review up to somebody else as this package was initially created by me, but I was wondering if you already have patches ready for wine itself to use this package (as wine expects a .CAB file while this package provides a folder containing the gecko files) ?
Comment 3 Richard W.M. Jones 2010-03-29 16:58:21 EDT
I might have a look at this tomorrow, but I'm interested to
know why wine needs a Win32-cross-compiled library?
Comment 4 Erik van Pienbroek 2010-03-29 17:10:22 EDT
See the discussion on the fedora-mingw mailing list from november last year: http://lists.fedoraproject.org/pipermail/mingw/2009-November/002267.html
Comment 5 Andreas Bierfert 2010-03-30 01:08:21 EDT
Also take a look here:
http://wiki.winehq.org/Gecko



(In reply to comment #2)
> by me, but I was wondering if you already have patches ready for wine itself to
> use this package (as wine expects a .CAB file while this package provides a
> folder containing the gecko files) ?    

Yes, I have a patch sitting here which adds this to latest wine.
Comment 6 Orion Poplawski 2010-05-21 18:42:43 EDT
I'm seeing a build failure on F-13 now, though F-14 (rawhide) seem okay.

http://koji.fedoraproject.org/koji/taskinfo?taskID=2202453
Comment 7 Andreas Bierfert 2010-07-02 11:39:57 EDT
https://koji.fedoraproject.org/koji/taskinfo?taskID=2290258

Just did another build on F-13 seems to work again (probably was a bug in the mingw stack). It would be nice if someone could review the package. It works quite well from me an we are very very close to the 1.2 release of wine and this would be a cool thing to have...
Comment 8 Nerijus Baliūnas 2010-07-06 19:14:13 EDT
(In reply to comment #5)
> 
> (In reply to comment #2)
> > by me, but I was wondering if you already have patches ready for wine itself to
> > use this package (as wine expects a .CAB file while this package provides a
> > folder containing the gecko files) ?    
> 
> Yes, I have a patch sitting here which adds this to latest wine.    

Has the patch been sent upstream and did wine upstream accept it?
Comment 9 Andreas Bierfert 2010-07-07 00:29:50 EDT
It has not been submitted upstream yet. It however is included in the latest wine builds in updates-testing. However once it has been tested a bit more I will send it upstream.
Comment 10 Nerijus Baliūnas 2010-07-13 09:14:03 EDT
http://fedora.lowlatency.de/review/mingw32-wine-gecko-1.0.0-3.fc13.src.rpm does not build here on F13:

Creating Resource file: module.res
i686-pc-mingw32-windres -O coff --use-temp-file -DSQLITE_SECURE_DELETE=1 -DSQLITE_THREADSAFE=1 -DSQLITE_CORE=1 -DSQLITE_ENABLE_FTS3=1 -DOSTYPE=\"WINNT\" -DOSARCH=WINNT --include-dir /home/nerijus/rpmbuild/BUILD/wine-mozilla/db/sqlite3/src --include-dir /home/nerijus/rpmbuild/BUILD/wine-mozilla/db/sqlite3/src --include-dir . --include-dir ../../../dist/include --include-dir ../../../dist/include/nsprpub --include-dir /home/nerijus/rpmbuild/BUILD/wine-mozilla/wine_gecko/dist/include/nspr --include-dir /home/nerijus/rpmbuild/BUILD/wine-mozilla/wine_gecko/dist/include/nss -o module.res /home/nerijus/rpmbuild/BUILD/wine-mozilla/wine_gecko/db/sqlite3/src/module.rc
i686-pc-mingw32-windres: /home/nerijus/rpmbuild/BUILD/wine-mozilla/wine_gecko/db/sqlite3/src/module.rc:54: syntax error
make[5]: *** [module.res] Error 1

$ rpm -qf /usr/bin/i686-pc-mingw32-windres
mingw32-binutils-2.19.51.0.14-1.fc12.i686

module.rc from line 53:
1 VERSIONINFO
 FILEVERSION    1,9,2,3845.95833333333
 PRODUCTVERSION 1,9,2,3845.95833333333
Comment 11 Stephen Kitt 2010-08-05 01:19:54 EDT
Applying the following patch should fix the build (it truncates the last component of the generated versions):

--- wine-gecko-1.0.0.orig/config/version_win.pl
+++ wine-gecko-1.0.0/config/version_win.pl
@@ -54,8 +54,8 @@ sub daysFromBuildID
     $d || die("Unrecognized buildid string.");
 
     my $secondstodays = 60 * 60 * 24;
-    return (POSIX::mktime(00, 00, 00, $d, $m - 1, $y - 1900) -
-            POSIX::mktime(00, 00, 00, 01, 00, 100)) / $secondstodays;
+    return int((POSIX::mktime(00, 00, 00, $d, $m - 1, $y - 1900) -
+            POSIX::mktime(00, 00, 00, 01, 00, 100)) / $secondstodays);
 }
 
 #Creates version resource file
Comment 12 Nerijus Baliūnas 2010-08-15 08:20:36 EDT
After applying the patch there is another error when compiling 
wine-mozilla/media/libtheora/lib/dec/x86_vc/mmxfrag.c:

In file included from /home/nerijus/rpmbuild/BUILD/wine-mozilla/media/libtheora/lib/dec/x86_vc/../../internal.h:25,
                 from /home/nerijus/rpmbuild/BUILD/wine-mozilla/media/libtheora/lib/dec/x86_vc/mmxfrag.c:17:
../../../dist/include/theora/theora.h:192: warning: comma at end of enumerator list
/home/nerijus/rpmbuild/BUILD/wine-mozilla/media/libtheora/lib/dec/x86_vc/mmxfrag.c: In function 'oc_frag_recon_intra_mmx':
/home/nerijus/rpmbuild/BUILD/wine-mozilla/media/libtheora/lib/dec/x86_vc/mmxfrag.c:36: error: '_asm' undeclared (first use in this function)
Comment 13 Jason Tibbitts 2010-11-17 08:22:14 EST
Please clear the whiteboard if providing a version which builds.
Comment 14 Erik van Pienbroek 2010-11-17 15:38:05 EST
The build can be fixed by applying this small patch:

--- js/src/jslock.cpp.orig      2010-11-17 19:34:59.449610055 +0100
+++ js/src/jslock.cpp   2010-11-17 20:07:18.980134638 +0100
@@ -66,7 +66,7 @@

 /* Implement NativeCompareAndSwap. */

-#if defined(_WIN32) && defined(_M_IX86)
+#if defined(_WIN32) && defined(_M_IX86) && !defined(__MINGW32__)
 #pragma warning( disable : 4035 )
 JS_BEGIN_EXTERN_C
 extern long __cdecl

A scratch build of this was done at http://koji.fedoraproject.org/koji/taskinfo?taskID=2607153
For this build I added this patch to the wine-mozilla-gcc4-compile-fix.patch file
Comment 15 Nerijus Baliūnas 2010-11-23 20:25:42 EST
This patch does not help on F14, I still get error as in Comment 12. But there is no more error as in Comment 10 w/o applying patch in Comment 11, as it was on F13.
Comment 16 Stephen Kitt 2010-11-24 09:55:02 EST
I don't know what's causing the problem in Comment 12, but I have figured out why the patch in Comment 11 was necessary previously: the daysFromBuildID function in config/version_win.pl extracts the build date from the build id, then calculates the number of days between the build date and Jan 1, 2000. This works fine just now because daylight savings time is not in effect; but at the time I posted Comment 11, it was, so the result of the calculation was one hour off and returned a non-integer value which caused the build to fail.

So the patch in Comment 11 is still necessary for builds to work all year round...
Comment 17 Andreas Bierfert 2011-02-06 05:36:44 EST
*** Bug 675422 has been marked as a duplicate of this bug. ***
Comment 18 Nerijus Baliūnas 2011-02-06 21:04:50 EST
Regarding error in Comment 12 - it seems it tries to compile in wine-mozilla/media/libtheora/lib/dec/x86_vc directory, which is for Visual C, while it should probably compile in wine-mozilla/media/libtheora/lib/dec/x86. Am I right?
Comment 19 Jacek Caban 2011-03-16 16:39:45 EDT
Hi all,

I'm maintainer of Wine Gecko. We've just released the new version:

http://www.winehq.org/pipermail/wine-devel/2011-March/089273.html

This is the first version that makes it possible to do proper packaging, like you've tried to do. We've improved the building process a lot, so your problem is probably already fixed. Please let me know if you have any problems, so we can work on them.
Comment 20 Andreas Bierfert 2011-06-21 14:16:26 EDT
All new fun build on f16 with cross enabled. Please test and leave feedback. Once all pieces of cross/mingw64 are in rawhide I hope someone will review:

http://fedora.lowlatency.de/review/mingw-wine-gecko.spec
http://fedora.lowlatency.de/review/mingw-wine-gecko-1.2.0-2.fc16.src.rpm


http://fedora.lowlatency.de/review/mingw32-wine-gecko-1.2.0-2.fc16.noarch.rpm
http://fedora.lowlatency.de/review/mingw64-wine-gecko-1.2.0-2.fc16.noarch.rpm
Comment 21 Erik van Pienbroek 2011-06-21 14:37:28 EDT
Very nice to see that you based this package on the new MinGW packaging guidelines (https://fedoraproject.org/wiki/Packaging:MinGW_Future) !

Some small comments:
Please start the .spec file with these set of lines:
%global __strip %{mingw_strip}
%global __objdump %{mingw_objdump}
%define __debug_install_post %{mingw_debug_install_post}

And add a line with %{?mingw_debug_package} right before the start of the first %package tag. This is needed to automatically extract debug information

I noticed you used the %{description} tag in some places. Last time I tried, this didn't give the expected results, so you may need to manually add a real description instead of this macro.
Comment 23 Kalev Lember 2011-08-02 05:08:23 EDT
The mingw-w64 toolchain isn't in Fedora and there is no clear indication how long it might take. What is sure is that it won't be in F16.

My suggestion at this point is to change mingw-wine-gecko spec file to work with the current MinGW toolchain in Fedora; once the mingw-w64 toolchain is imported it is easy to convert it over.

I would be happy to review this package if you make these changes.

Note that similar to the "future" guidelines, the "current" guidelines now suggest to use mingw- as source package prefix to ease the transition to the "future" guidelines.

Current guidelines: https://fedoraproject.org/wiki/Packaging/MinGW
Example spec file: https://fedoraproject.org/wiki/Packaging/MinGW#Example_Specfile
Comment 24 Jacek Caban 2011-08-05 03:17:13 EDT
(In reply to comment #23) 
> My suggestion at this point is to change mingw-wine-gecko spec file to work
> with the current MinGW toolchain in Fedora

This is not possible. MinGW is not sufficient for Wine Gecko compilation.
Comment 25 Erik van Pienbroek 2012-03-10 11:00:46 EST
Hey Andreas,

The mingw-w64 toolchain has just been added to Fedora 17 so this review should now be able to continue.

Here are some minor improvements you could make:
- The first 3 lines of the .spec file (%global/%define) can be replaced by %?mingw_package_header
- The BuildRoot tag and the line 'rm -rf %{buildroot}' can be removed now
- The -DMINGW_HAS_SECURE_API shouldn't be needed any more with mingw-w64. Could you test if this package can be built without that define set?
- We (the Fedora MinGW SIG) decided to get rid of the %global mingw_pkg_name in all mingw-* packages as it didn't have any added value. You also might want to remove it
- The new MinGW packaging guidelines don't use brackets in RPM macro calls any more, so you might want to change %{?mingw_debug_package} to %?mingw_debug_package
Comment 26 Andreas Bierfert 2012-03-11 17:44:23 EDT
Thanks for the info. I will try to upgrade to latest wine-gecko during the week and maybe we can get this in soon.
Comment 27 Andreas Bierfert 2012-03-20 17:46:16 EDT
updated srpm/spec:
http://fedora.lowlatency.de/review/mingw-wine-gecko.spec
http://fedora.lowlatency.de/review/mingw-wine-gecko-1.5-1.fc17.src.rpm

koji:
https://koji.fedoraproject.org/koji/taskinfo?taskID=3914963

changes:
- upgrade to latest wine gecko (1.5)
- cleanup spec including changes from comment 25

I have prepared an upgrade to wine 1.5.0 for f17 and rawhide. If wine-gecko gets a fast review I will hold wine 1.5.0 and release both at the same time. This would ensure that wine automatically pulls in the fedora mingw gecko on upgrading the prefix to 1.5.
Comment 28 Michael Cronenworth 2012-03-20 19:37:22 EDT
Hi Andreas, lets get wine-gecko in Fedora!

$ md5sum rpmbuild/SOURCES/wine-mozilla-1.5-src.tar.bz2
e1d9ba8914582d382b808332e4db0a54  wine-mozilla-1.5-src.tar.bz2
# wget $SOURCE0
$ md5sum $SOURCE0
e1d9ba8914582d382b808332e4db0a54  wine-mozilla-1.5-src.tar.bz2

Your spec looks great. Let's check rpmlint.

$ rpmlint mingw32-wine-gecko
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

$ rpmlint mingw64-wine-gecko
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

Looks great there, too. Everything looks in order to me.
Comment 29 Andreas Bierfert 2012-03-21 05:35:18 EDT
Thanks for picking it up so fast. I have made scratch builds for wine 1.5.0 so it is easier to do some testing.

rawhide:
https://koji.fedoraproject.org/koji/taskinfo?taskID=3916624

f17:
https://koji.fedoraproject.org/koji/taskinfo?taskID=3916632
Comment 30 Kalev Lember 2012-03-21 05:50:35 EDT
What packages are going to own the directories %{_datadir}/wine/gecko/ and %{_datadir}/wine/ ? If none of the core wine packages do, might want to have both mingw32-wine-gecko and mingw64-wine-gecko own these directories to make sure they get removed on uninstall.

By the way, if you want to get rid of some more rpm boilerplate code, these are no longer needed with latest mingw-filesystem:
%global mingw_build_win32 1
%global mingw_build_win64 1

... and the BuildRoot definition isn't needed either with the version of RPM we have in Fedora.


Otherwise the spec file looks great, really nice to finally have this in Fedora!
Comment 31 Andreas Bierfert 2012-03-21 05:55:32 EDT
The directories are owned by wine-common. I will add a requires for it and cleanup the spec after import. Thanks for the comment!
Comment 32 Andreas Bierfert 2012-03-21 05:56:07 EDT
New Package SCM Request
=======================
Package Name: mingw-wine-gecko
Short Description: Gecko library required for Wine
Owners: awjb
Branches: f17
Comment 33 Gwyn Ciesla 2012-03-21 08:40:12 EDT
Git done (by process-git-requests).
Comment 34 Andreas Bierfert 2012-03-21 14:16:51 EDT
Thanks for the review, help, and comments. And of course thanks to the mingw sig for the endurance.

Note You need to log in before you can comment on or make changes to this bug.