Closed Bug 469558 Opened 11 years ago Closed 11 years ago

--enable-system-lcms build option should be removed

Categories

(Core :: GFX: Color Management, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: bholley, Assigned: bholley)

References

Details

(Keywords: fixed1.9.1)

Attachments

(1 file, 2 obsolete files)

We can't build with system lcms anymore and we won't be able to any time soon.

filed in response to bug 469556.
Attached patch fix 0.1 (obsolete) — Splinter Review
Added a patch for this. It builds fine on my local machine - I'm running it through the tryserver now.

This is certainly not my area of expertise - I'm mostly just going off of mxr. I'll flag for review once the tryserver build comes out positive.
Assignee: nobody → bholley
Status: NEW → ASSIGNED
Comment on attachment 352925 [details] [diff] [review]
fix 0.1

tryserver all green. Flagging vlad for review.

Vlad, I'm not sure who the appropriate build system people are for the sr. Can you flag them for sr? if your r+ this?

I'll be gone for the next 11 days, and after that i'll have 1 day before being gone again until Jan 6th. If this needs to be pushed through soon someone else should take ownership.
Attachment #352925 - Flags: review?(vladimir)
Attachment #352925 - Flags: superreview?(ted.mielczarek)
Oh, was just thinking -- we rename the lcms symbols, right?  To avoid conflicts with system lcms in case something else pulls it in?
Comment on attachment 352925 [details] [diff] [review]
fix 0.1

-#if MOZ_NATIVE_LCMS==1
-#define WRAP_LCMS_HEADERS
-#endif
 #ifdef WRAP_LCMS_HEADERS
 icc34.h
 lcms.h
 #endif

Just kill those following lines. (And in js/src/config/system-headers as well.)

--- a/toolkit/toolkit-makefiles.sh

How about you move this block up underneath MAKEFILES_libmar and use the same style for it? Not a big deal, but might as well be consistent.

Looks fine otherwise.
Attachment #352925 - Flags: superreview?(ted.mielczarek) → superreview+
vlad - no, I don't think so. What kind of scenario would have it pulled in?
ted - I assume I should kill the lines immediately before as well? (they deal with setting WRAP_LCMS_HEADERS, which it seems like we don't care about).

Running the build through the tryserver again just to be safe. I'll land it once it goes green.
Comment on attachment 352925 [details] [diff] [review]
fix 0.1

Flagging beltzner for 191 approval.

Beltzner - you said we wanted this over email, so it should be a no-brainer.
Attachment #352925 - Flags: approval1.9.1?
Attachment #352925 - Flags: approval1.9.1? → approval1.9.1+
Keywords: checkin-needed
pushed to 8f347bf50a53. Will land in 191 after baking.
Whiteboard: [needs 191 landing]
Depends on: 473378
I guess this broke static-analysis tbox
Comment on attachment 352925 [details] [diff] [review]
fix 0.1

And this is not the patch which was pushed to m-c
This patch broke non-libxul clobber builds on linux. On x86-64 this manifests as a build bustage... on i686 it's just a text relocation. The problem is this hunk in config/system-headers:

--- a/config/system-headers	Mon Jan 12 21:47:31 2009 +0100
+++ b/config/system-headers	Mon Jan 12 16:20:45 2009 -0800
@@ -1014,18 +1014,6 @@ png.h
 #if MOZ_NATIVE_ZLIB==1
 zlib.h
 #endif
-#if MOZ_ENABLE_LIBXUL!=1
-#if BUILD_STATIC_LIBS!=1
-#define WRAP_LCMS_HEADERS
-#endif
-#endif
-#if MOZ_NATIVE_LCMS==1
-#define WRAP_LCMS_HEADERS
-#endif
-#ifdef WRAP_LCMS_HEADERS
-icc34.h
-lcms.h
-#endif
 #ifdef MOZ_ENABLE_STARTUP_NOTIFICATION
 libsn/sn.h
 libsn/sn-common.h

We should still be wrapping icc34.h and lcms.h in the no-libxul case: this hunk should only have removed the MOZ_NATIVE_LCMS case. I will commit a fix, but please make sure you include it when you port to the branch.
hrgh, given that the committed patch didn't match the reviewed patch, I'm going to back this out
Attached patch patch that was committed (obsolete) — Splinter Review
this is the patch that was committed and broke bsmedberg's build. It has the changes ted asked for in the sr+. It looks like those are the problematic ones. I have to head out but I'll re-submit a patch once I get back with the proper fixes.
Attachment #352925 - Attachment is obsolete: true
Attachment #356739 - Attachment is obsolete: true
Attachment #356739 - Attachment is patch: true
Attachment #356739 - Attachment mime type: application/octet-stream → text/plain
Attachment #356815 - Flags: superreview?(ted.mielczarek)
Attachment #356815 - Flags: review?(benjamin)
Comment on attachment 356815 [details] [diff] [review]
patch 0.2 - fixes bsmedberg's issue

I've added an updated patch that should fix things. Flagging bsmedberg and ted.

Bsmedberg - I don't have access to an x86_64 box. Is there some way I can test that this definitely fixes things?
Attachment #356815 - Flags: review?(benjamin) → review+
Attachment #356815 - Flags: superreview?(ted.mielczarek) → superreview+
pushed to mc in 71a49097a2fd.
Keywords: checkin-needed
pushed to mozilla-1.9.1 in 94974f18a47c. Resolving as fixed.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [needs 191 landing]
Please remember the fixed1.9.1 keyword.
Keywords: fixed1.9.1
yep - I had a midair collision with you when you added it. Sorry about that.
Blocks: 475111
You need to log in before you can comment on or make changes to this bug.