Bug 97592

Summary: [EFL] Add minimum version information for tool dependencies
Product: WebKit Reporter: Laszlo Gombos <laszlo.gombos>
Component: PlatformAssignee: Laszlo Gombos <laszlo.gombos>
Status: RESOLVED FIXED    
Severity: Normal CC: eric, gyuyoung.kim, laszlo.gombos, mxie, paroga, rakuco, rwlbuis, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 102551, 108061    
Bug Blocks:    
Attachments:
Description Flags
min version info
none
change python version from 2.5.0+ to 2.6.0+
none
change gperf min version to 3.0.3
none
fix comment for the perl version check
none
patch to commit
none
changed the gperf requirement to 3.0.1 kenneth: review+, webkit.review.bot: commit-queue-

Description Laszlo Gombos 2012-09-25 12:05:37 PDT
I find it important to capture the minimum version information for the tools (not just for libraries) that are required to build the EFL port (or more generally WebKit).
Comment 1 Laszlo Gombos 2012-09-25 12:08:50 PDT
Created attachment 165649 [details]
min version info
Comment 2 Raphael Kubo da Costa (:rakuco) 2012-09-25 12:26:29 PDT
Comment on attachment 165649 [details]
min version info

Are you sure these are the minimum versions we require? Python 2.5 itself is not supported by the WebKit tools, but it might work for building the port; the others I don't really know.
Comment 3 Laszlo Gombos 2012-09-25 14:01:08 PDT
Created attachment 165664 [details]
change python version from 2.5.0+ to 2.6.0+
Comment 4 Raphael Kubo da Costa (:rakuco) 2012-09-26 00:10:03 PDT
Comment on attachment 165664 [details]
change python version from 2.5.0+ to 2.6.0+

View in context: https://bugs.webkit.org/attachment.cgi?id=165664&action=review

Please note that not all find modules actually check for versions, as I mentioned in the inline comments.

It would also be a good idea to mention all those versions in the wiki, BTW.

> CMakeLists.txt:62
> +FIND_PACKAGE(Gperf 3.0.4 REQUIRED)

Debian-based distros still seem to be on 3.0.3 (plus FindGperf does not actually check for the actual gperf version at all). Are you sure you need 3.0.4?

> CMakeLists.txt:63
> +FIND_PACKAGE(Perl 5.10.0 REQUIRED)

Please note that only CMake 2.8.8+ actually checks for Perl versions. 5.10.0 should be old enough anyway.
Comment 5 Laszlo Gombos 2012-09-26 21:50:27 PDT
(In reply to comment #4)
> It would also be a good idea to mention all those versions in the wiki, BTW.

Done (before posted the patch - http://trac.webkit.org/wiki/EFLWebKit?action=diff&version=74&old_version=73 .

> > CMakeLists.txt:62
> > +FIND_PACKAGE(Gperf 3.0.4 REQUIRED)

> (plus FindGperf does not actually check for the actual gperf version at all).

I considered making it a comment in the same file, but than the expected version information is not logged in the build output (which I think is useful). Would you prefer it as a comment instead ? 

> Are you sure you need 3.0.4?

Chromium team seems to believe that 3.0.4 is a requirement - http://code.google.com/p/chromium/wiki/LinuxBuildInstructionsPrerequisites .
Comment 6 Raphael Kubo da Costa (:rakuco) 2012-09-27 01:46:05 PDT
(In reply to comment #5)
> > > CMakeLists.txt:62
> > > +FIND_PACKAGE(Gperf 3.0.4 REQUIRED)
> 
> > (plus FindGperf does not actually check for the actual gperf version at all).
> 
> I considered making it a comment in the same file, but than the expected version information is not logged in the build output (which I think is useful). Would you prefer it as a comment instead ? 

Perhaps a TODO comment explaining the version check currently does nothing would be helpful.

> > Are you sure you need 3.0.4?
> 
> Chromium team seems to believe that 3.0.4 is a requirement - http://code.google.com/p/chromium/wiki/LinuxBuildInstructionsPrerequisites .

Hmm, I've been using 3.0.3 on this Debian installation and everything seems to work fine. Since Ubuntu still ships 3.0.3 as well, I guess requiring that version as a minimum should not cause any problems.
Comment 7 Laszlo Gombos 2012-09-27 08:20:43 PDT
Created attachment 166009 [details]
change gperf min version to 3.0.3
Comment 8 Laszlo Gombos 2012-09-27 08:22:40 PDT
(In reply to comment #6)
> (In reply to comment #5)

> Hmm, I've been using 3.0.3 on this Debian installation and everything seems to work fine. Since Ubuntu still ships 3.0.3 as well, I guess requiring that version as a minimum should not cause any problems.

I looked trough the gperf changelog and found no evidence that gperf 3.0.4 would be required, so I agree with you.

Updated the patch.
Comment 9 Raphael Kubo da Costa (:rakuco) 2012-09-27 14:22:02 PDT
Comment on attachment 166009 [details]
change gperf min version to 3.0.3

View in context: https://bugs.webkit.org/attachment.cgi?id=166009&action=review

Looks fine, thanks!

> CMakeLists.txt:66
> +# TODO Enforce version requirement for perl

Minor nit: FindPerl.cmake already detects versions, but only from version 2.8.8 on.
Comment 10 Laszlo Gombos 2012-10-02 09:11:50 PDT
Created attachment 166695 [details]
fix comment for the perl version check
Comment 11 Raphael Kubo da Costa (:rakuco) 2012-10-02 09:19:39 PDT
Comment on attachment 166695 [details]
fix comment for the perl version check

Informal r+ from my side.
Comment 12 WebKit Review Bot 2012-10-04 16:53:06 PDT
Comment on attachment 166695 [details]
fix comment for the perl version check

Rejecting attachment 166695 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

ERROR: /mnt/git/webkit-commit-queue/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/14177248
Comment 13 Laszlo Gombos 2012-10-08 09:44:00 PDT
Created attachment 167548 [details]
patch to commit
Comment 14 WebKit Review Bot 2012-10-08 11:10:13 PDT
Comment on attachment 167548 [details]
patch to commit

Clearing flags on attachment: 167548

Committed r130656: <http://trac.webkit.org/changeset/130656>
Comment 15 WebKit Review Bot 2012-10-08 11:10:18 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 Patrick R. Gansterer 2012-10-14 08:00:25 PDT
Reverted r130656 for reason:

Broke CMake build on Windows

Committed r131266: <http://trac.webkit.org/changeset/131266>
Comment 17 Patrick R. Gansterer 2012-10-14 08:10:29 PDT
Can you explain why we require exactly the given version? Maybe newer version work faster or so and have additional (unnecessary) features, but are there any real problems with older versions?
Copy-Paste-ing the requirements from Chromiums doesn't seam good to me, if you do not check the details.
Main problem is that http://gnuwin32.sourceforge.net (and MinGW) does not provide actual versions. So an easy update isn't possible.

BTW: Please at least CC other port maintainers which use CMake too, when changing general CMake stuff.
Comment 18 Laszlo Gombos 2012-10-18 14:03:13 PDT
(In reply to comment #17)

> Main problem is that http://gnuwin32.sourceforge.net (and MinGW) does not provide actual versions. So an easy update isn't possible.

Patrick, sorry for the build break. Can you help me and tell us which dependency is not available in the specified version for Windows? I will look into understanding the differences in versions for those dependencies.
Comment 19 Patrick R. Gansterer 2012-10-19 00:26:20 PDT
See http://gnuwin32.sourceforge.net/packages.html.
Comment 20 Laszlo Gombos 2012-10-22 16:15:17 PDT
(In reply to comment #19)
> See http://gnuwin32.sourceforge.net/packages.html.

Patrick, would you consider using a different distribution for Windows binaries ?

The Qt port on Windows had a similar problem and they decided to use http://sourceforge.net/projects/winflexbison/ instead - see http://lists.webkit.org/pipermail/webkit-qt/2012-September/003107.html. The mail also includes justification for using a new version than it is available from gnuwin32.

This would allow us to focus on a smaller set of possibly dependencies for WebKit ports. Also note that these are "only" build requirements, so it does not restrict the usage of the built binaries in any way.
Comment 21 Patrick R. Gansterer 2012-10-22 22:38:51 PDT
(In reply to comment #20)
> (In reply to comment #19)
> > See http://gnuwin32.sourceforge.net/packages.html.
> 
> Patrick, would you consider using a different distribution for Windows binaries ?

Of course. I only need a solution for getting the binaries without maintaining them myself. If there are any working win32 binaries out there, I see no problem in switching to them

> The Qt port on Windows had a similar problem and they decided to use http://sourceforge.net/projects/winflexbison/ instead - see http://lists.webkit.org/pipermail/webkit-qt/2012-September/003107.html. The mail also includes justification for using a new version than it is available from gnuwin32.

I only checked the Wiki pages of the Qt port on WebKit, where still the gnu32 binaries are mentioned.


I'll try to update my local build tree to winflexbison and keep you updated.
Comment 22 Laszlo Gombos 2012-11-06 08:02:52 PST
> I'll try to update my local build tree to winflexbison and keep you updated.

Patrick, have you had a chance to try update these windows dependencies ?
Comment 23 Patrick R. Gansterer 2012-11-16 12:43:28 PST
(In reply to comment #22)
> > I'll try to update my local build tree to winflexbison and keep you updated.
> 
> Patrick, have you had a chance to try update these windows dependencies ?

At first big fat sorry for the very long delay. I've uploaded a patch at bug 102551, which adds support for the alternative version. Then bison is at version 2.5 and flex at 2.5.35. Gperf is then still at 3.0.1.
Comment 24 Laszlo Gombos 2012-11-23 11:57:14 PST
Created attachment 175832 [details]
changed the gperf requirement to 3.0.1
Comment 25 Kenneth Rohde Christiansen 2012-11-23 13:08:38 PST
Comment on attachment 175832 [details]
changed the gperf requirement to 3.0.1

Btw, if not necessarily required, but if newer versions provides better performance, bug fixes etc, we should bump the version as well!
Comment 26 Laszlo Gombos 2012-11-23 18:04:36 PST
(In reply to comment #25)
> (From update of attachment 175832 [details])
> Btw, if not necessarily required, but if newer versions provides better performance, bug fixes etc, we should bump the version as well!

I agree now and going forward. In this particular case (using a recent snapshot) I double-checked and gperf 3.0.1 and gperf 3.0.3 both seems to generate the same output (even though gperf 3.0.1 was released in 2003.
Comment 27 Eric Seidel (no email) 2013-01-04 00:43:37 PST
Comment on attachment 166695 [details]
fix comment for the perl version check

Cleared Kenneth Rohde Christiansen's review+ from obsolete attachment 166695 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment 28 Eric Seidel (no email) 2013-01-04 00:54:18 PST
Attachment 175832 [details] was posted by a committer and has review+, assigning to Laszlo Gombos for commit.
Comment 29 WebKit Review Bot 2013-01-29 12:35:33 PST
Comment on attachment 175832 [details]
changed the gperf requirement to 3.0.1

Rejecting attachment 175832 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=gce-cq-04', 'apply-attachment', '--no-update', '--non-interactive', 175832, '--port=chromium-xvfb']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue

Last 500 characters of output:
ly', '--force', '--reviewer', 'Kenneth Rohde Christiansen']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue

Parsed 2 diffs from patch file(s).
patching file ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file CMakeLists.txt
Hunk #1 FAILED at 57.
1 out of 1 hunk FAILED -- saving rejects to file CMakeLists.txt.rej

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', '--force', '--reviewer', 'Kenneth Rohde Christiansen']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue

Full output: http://queues.webkit.org/results/16198270
Comment 30 Patrick R. Gansterer 2013-01-29 13:20:55 PST
Committed r141157: <http://trac.webkit.org/changeset/141157>