Closed Bug 440017 Opened 12 years ago Closed 12 years ago

include before using INSTALL_LIGHTNING so that var can be set


(Calendar :: Build Config, defect)

Not set


(Not tracked)



(Reporter: kairo, Assigned: kairo)




(1 file, 2 obsolete files)

bug 406441 egalized bug 349870 by requiring an INSTALL_LIGHTNING variable to be set to preinstall Lighting into Thunderbird (or SeaMonkey) at build time, but one cannot set this variable, e.g. through or right now, as has not been imported when that ifdef is being evaluated.
Attached patch patch: include (obsolete) — Splinter Review
Here's a patch to include right before that ifdef, so myconfig or app-config can set this var.
Assignee: nobody → kairo
Attachment #325572 - Flags: review?(mvl)
Comment on attachment 325572 [details] [diff] [review]
patch: include

Sorry, I don't have time for reviews.
Attachment #325572 - Flags: review?(mvl)
Attachment #325572 - Flags: review?(daniel.boelzle)
Attachment #325572 - Flags: review?(daniel.boelzle) → review?(ause)
Blocks: 442566
I propose a different fix.  I believe that the default introduced by bug 406441 is wrong: every developer working on lightning in-tree needs to figure out why their build isn't getting installed and then set it by hand, which is just an extra barrier to entry.  There are only ever a small, limited number of Tinderboxen, though.  So I propose to change from |ifdef INSTALL_LIGHTNING| to |ifndef DISABLE_LIGHTNING_INSTALL| and just set that variable in the .mozconfig as an env var in the various tinderboxen.
Attachment #329503 - Flags: review? → review?(ause)
i tend to prefer the second patch. as far as i see it, the future default will be to have lightning preinstalled. if that's right, it shouldn't require setting anything.
to have included early in a makefile may open up some nice oportunities, but may require moving/copying the block setting MOZILLA_DIR from to (it's used there). looks like someone already did similar for topsrcdir in the past.
Actually, I'm quite happy with dmose's approach. We even could add both patches if we want people to be able to turn it off via, but I'll leave the decision on what to take and not up to ause ;-)
Per IRC talk, I'm attaching this combined patch of mine and dmose's, so users (or build machines?) have the chance of disabling the Lightning install with The most significant change is still dmose's, so leaving his addition to the contibutors section.
Attachment #325572 - Attachment is obsolete: true
Attachment #329503 - Attachment is obsolete: true
Attachment #330247 - Flags: review?
Attachment #325572 - Flags: review?(ause)
Attachment #329503 - Flags: review?(ause)
Attachment #330247 - Flags: review? → review?(ause)
Attachment #330247 - Flags: review?(ause) → review+
Thanks, cross-committed to cvs trunk and 1.8 branch.
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → 0.9
Version: Trunk → unspecified
The checkins for this bug seem to be the only ones in the last days that might be the causer of Bug 446170.
(In reply to comment #9)
> The checkins for this bug seem to be the only ones in the last days that might
> be the causer of Bug 446170.

Thanks, see for my comments on what the real problem is and how to solve it there.
Blocks: 446170
You need to log in before you can comment on or make changes to this bug.