Closed Bug 646489 Opened 9 years ago Closed 9 years ago

Require yasm 1.0.1 instead of yasm 1.1.0 for libjpeg-turbo on Linux

Categories

(Firefox Build System :: General, defect)

All
Linux
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla5

People

(Reporter: justin.lebar+bug, Assigned: justin.lebar+bug)

References

Details

Attachments

(2 files)

yasm 1.1.0 is hard to come by on Linux distributions, since it's pretty new.  If 1.0.1 works just as well, we could require that version instead.

smaug verified that Firefox compiled and passed the jpeg reftests (/modules/libpr0n/test/reftest/jpeg) on Fedora 14 x86-64 using yasm 1.0.1.  But we still need to verify that it works on x86-32 using yasm 1.0.1.
Blocks: 573948
I verified this and commented on the original bug that decided to enforce 1.1 and was completely ignored.
Well now I am not sure it was on the bug, but was definitely on the IRC conversation about this issue.
I guess since you were so quick to dupe this, that means you are not interested int he patch I intended to apply to my bug.
I would r+ and land a patch to drop the version requirement down to 1.0.1.  I don't really want to just change the error to a warning though.
Yes, but somehow now that my bug has been duped, I feel OK now the person who duped it is on the hook for developing such a patch. I am still **** off that this landed in the first place since before it did I had pointed out to those pushing for it the version 1.0.1 worked just fine.
Bill, I'm sorry I missed your comment about yasm 1.0.1 working, and I'm sorry this has frustrated you.  I just searched through my IRC logs, and I don't see messages from you containing "1.0.1" or "yasm" in #developers, so you must have spoken to someone else.

If you can confirm for me that yasm 1.0.1 passes all the reftests in modules/libpr0n/test/reftest/jpeg on Linux x86-32?  If so, I'll develop the requisite patch.
(In reply to comment #7)
> Bill, I'm sorry I missed your comment about yasm 1.0.1 working, and I'm sorry
> this has frustrated you.  I just searched through my IRC logs, and I don't see
> messages from you containing "1.0.1" or "yasm" in #developers, so you must have
> spoken to someone else.

Actually, it is me should should be apologizing form my frustrations in my real job from spilling over here.

> 
> If you can confirm for me that yasm 1.0.1 passes all the reftests in
> modules/libpr0n/test/reftest/jpeg on Linux x86-32?  If so, I'll develop the
> requisite patch.

The yasm version I am running passes all the reftests in modules/libpr0n/test/reftest/jpeg, however it is not strictly speaking 1.0.1.  yasm --version returns the following:

yasm 1.0.1.2326
Compiled on May 23 2010.
Copyright (c) 2001-2010 Peter Johnson and other Yasm developers.
Run yasm --license for licensing overview and summary.

I suspect the .2326 part might be significant here.
Attached patch Patch v1Splinter Review
Attachment #524205 - Flags: review?(khuey)
Comment on attachment 524205 [details] [diff] [review]
Patch v1

r+ assuming we actually do need yasm 1.1 on non-Linux.
Attachment #524205 - Flags: review?(khuey) → review+
I'm pretty sure we actually do need yasm 1.1 on non-Linux, as bug 573948 comment 65 was made two months after the 1.0.1 release [1].

[1] https://github.com/yasm/yasm/commits/1.0.1
Whiteboard: [fixed-in-cedar]
http://hg.mozilla.org/mozilla-central/rev/028a07f2fae2
Assignee: nobody → justin.lebar+bug
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-cedar]
Target Milestone: --- → mozilla2.2
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Version: unspecified → Trunk
Comment on attachment 524205 [details] [diff] [review]
Patch v1

>+        if test "$_YASM_MAJOR_VERSION" -lt "1" -o \( "$_YASM_MAJOR_VERSION" -eq "1" -a "$_YASM_MINOR_VERSION" -eq "0" -a "$_YASM_RELEASE" -lt "1" \) ; then
>+            AC_MSG_ERROR([yasm 1.0.1 or greater is required to build with libjpeg-turbo's optimized JPEG decoding routines, but you appear to have version $_YASM_MAJOR_VERSION.$_YASM_MINOR_VERSION.$_YASM_MINOR_VERSION.  Upgrade to the newest version or configure with --disable-libjpeg-turbo to use the pure C JPEG decoder.  See https://developer.mozilla.org/en/YASM for more details.])

This should be $_YASM_MAJOR_VERSION.$_YASM_MINOR_VERSION.$_YASM_RELEASE .
Attached patch FollowupSplinter Review
Attachment #525402 - Flags: review?(khuey)
The followup patch was landed:

http://hg.mozilla.org/mozilla-central/rev/c89cfb2997dc

I mistakenly backed it out <http://hg.mozilla.org/mozilla-central/rev/4090fb78bbac> but immediately restored it <http://hg.mozilla.org/mozilla-central/rev/ecfed11869b2>.  Sorry!
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.