Closed
Bug 383934
Opened 17 years ago
Closed 17 years ago
Re-organize targets to allow preference panes to use -bundle_loader
Categories
(Camino Graveyard :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Camino1.6
People
(Reporter: stuart.morgan+bugzilla, Assigned: mark)
References
Details
(Keywords: fixed1.8.1.12)
Attachments
(3 files, 1 obsolete file)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
stuart.morgan+bugzilla
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review |
Right now we have several places where we are either compiling duplicate code into preferences panes, or we are doing ugly workarounds (including copying and pasting methods into the pref pane class) to give the pref panes access to code that's compiled into Camino anyway. I'm running into that pretty hard trying to switch the preferences to the new Cocoa nsIPermissionManager wrapper.
I think we should re-organize the targets so that we have one that builds just the Camino executable, so that we can reference it with -bundle_loader in the preference panes, then another that bundles all the pref panes into a full Camino.app. It should make pref pane development easier and potentially make them a bit smaller.
Thoughts?
Blocks: 352451
Assignee | ||
Comment 1•17 years ago
|
||
Yes, this should keep things cleaner. I hope that this won't be complicated by having both Camino and CaminoStatic targets, though.
Reporter | ||
Updated•17 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee | ||
Comment 2•17 years ago
|
||
I need this for bug 354975, so I'm taking it and doing it. Stuart assures me he doesn't mind.
Assignee: stuart.morgan → mark
Reporter | ||
Comment 3•17 years ago
|
||
Sweet, then I don't have to hack my pref changes after all.
Assignee | ||
Comment 4•17 years ago
|
||
Use aggregate targets to build the application; set bundle loader for preference panes
The high-level overview here: I introduce new aggregate targets (ordered lists of other targets) to ensure that we can build the application and the preference panes without either having a direct dependency on the other as far as Xcode is concerned. The application's target no longer has a CopyFiles phase to copy preference panes into place. Instead, preference pane bundles are built directly in the right spot in the application (Camino.app/Contents/PreferencePanes). Preference panes specify that Camino.app/Contents/MacOS/Camino is their bundle loader, and therefore are now able to link against anything in the main application bundle. The aggregate targets build the preference panes after the main application in order for the application to be available for ld's use as a -bundle_laoder when the panes are linked.
More details: the existing application targets Camino and CaminoStatic have been renamed to CaminoApp and CaminoStaticApp. The new aggregate targets are named Camino and CaminoStatic. There's no change to the makefiles, because the aggregate targets (going by the names that the application targets used to have) are what we want to build: they'll build the world and produce properly-functioning complete Camino.apps in the same spot as always in the build directory.
An added benefit of this reorganization is that I was able to move PreferencePaneBase into the application, rather than having each preference pane compile its own copy. Incidentally, that fixes bug 185954. I wanted to rename PreferencePaneBase to CmPreferencePane, but I was already doing lots of other stuff that resulted in a huge patch, so I decided against it for now.
Here are the notes on the project file changes. This'll help us come up with a branch version of the patch:
- delete prefpane dependencies from Camino, CaminoStatic
- delete Copy Preference Pane phases from Camino, CaminoStatic
- rename Camino to CaminoApp, CaminoStatic to CaminoStaticApp
- sort prefpanes in target list as:
Navigation Appearance Privacy Security History Tabs Downloads WebFeatures
- add Camino target as Aggregate, include CaminoApp and prefpanes
- add CaminoStatic target as Aggregate, include CaminoStaticApp and prefpanes
- set default target to Camino
- fix PreferencePaneBase.h,mm to point to files in new location
(src/preferences)
- add PreferencePaneBase.h to CaminoApp,CaminoStaticApp's Copy Headers
- add PreferencePaneBase.mm to CaminoApp,CaminoStaticApp's Compile Sources
- remove PreferencePaneBase.mm from each preference pane
- ensure PreferencePaneBase.h is in each preference pane's Copy Headers
- remove PreferencePanes.framework from each preference pane
- remove all Gecko libs from each preference pane, except:
+ leave libxpcom_core.dylib in Privacy, History, Downloads, Web Features
- remove Carbon.framework from Appearance, Navigation, History, Tabs
- clear framework, header, and library search paths from all build
configurations for all preference panes, except:
+ framework search path
* Navigation: sparkle/build/releaswe
+ header search path
* Privacy: ../dist/include ../dist/include/cookie
../dist/include/necko ../dist/include/nspr
../dist/include/pref ../dist/include/string
../dist/include/xpcom
* Security: ../dist/include ../dist/include/nspr ../dist/include/pref
../dist/include/xpcom
* History: ../dist/include ../dist/include/docshell
../dist/include/history ../dist/include/necko
../dist/include/nkcache ../dist/include/nspr
../dist/include/xpcom
* Tabs: ../dist/include ../dist/include/dom ../dist/include/nspr
../dist/include/xpcom
* Downloads: ../dist/include ../dist/include/nspr ../dist/include/xpcom
$(SYSTEM_DEVELOPER_DIR)/Headers/FlatCarbon
* WebFeatures: ../dist/include ../dist/include/necko
../dist/include/nspr ../dist/include/string
../dist/include/xpcom
* all others: ../dist/include
+ library search path:
* Privacy, History, Downloads, WebFeatures: ../dist/lib
Attachment #292474 -
Flags: review?(stuart.morgan)
Reporter | ||
Comment 5•17 years ago
|
||
Comment on attachment 292474 [details] [diff] [review]
Use aggregate targets to build the application; set bundle loader for preference panes
r+, but lets leave the -fshort-wchar until I finish deGeckoizing prefs, just in case.
Attachment #292474 -
Flags: review?(stuart.morgan) → review+
Assignee | ||
Comment 6•17 years ago
|
||
Checked in on the trunk, leaving -fshort-wchar.
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 7•17 years ago
|
||
Reopened, this didn't play nice with Xcode 2.5's toolchain.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 8•17 years ago
|
||
For reference, here's a patch that, in conjunction with attachment 292474 [details] [diff] [review], bases the preference panes on CmPreferencePane.framework.
Assignee | ||
Comment 9•17 years ago
|
||
After poking this with a series of sticks of varying sizes, I found out that the problem is a bad interaction between -bundle_loader and -dead_strip in Xcode 2.5's ld. As a workaround, we won't link prefpanes with -dead_strip. This shouldn't be too big of a deal because there shouldn't really be any dead code in the prefpanes.
I'm also including a change that passes -executable_path to ld, because Xcode 2.5's toolchain wasn't reliably able to resolve dependencies of libxpcom_core.dylib.
The only changes between the earlier checkin and v2 are these lines added to mozilla/camino/config/PrefPane.xcconfig:
>+OTHER_LDFLAGS = >+-Wl,-executable_path,$(BUILD_DIR)/$(CONFIGURATION)/Camino.app/Contents/MacOS
>+
>+// -bundle_loader and -dead_strip interact poorly with one another in the Xcode
>+// 2.5 cctools, causing pref pane linking to fail with:
>+// ld: warning internal error: output_flush(offset = w, size = x) overlaps with flushed block(offset = y, size = z)
>+// This is not a problem in Xcode 3.0. This can be removed once Xcode 3.0 is
>+// the minimum build requirement.
>+DEAD_CODE_STRIPPING = NO
v2 also includes OTHER_CFLAGS = -fshort-wchar, as requested by smorgan in comment 5.
Attachment #292474 -
Attachment is obsolete: true
Attachment #292533 -
Flags: review?(stuart.morgan)
Assignee | ||
Comment 10•17 years ago
|
||
I tested this patch for a cycle on boxset (19:50 - 20:24), and it was green.
Reporter | ||
Comment 11•17 years ago
|
||
Comment on attachment 292533 [details] [diff] [review]
Xcode 2.5-compatible fix
> I tested this patch for a cycle on boxset (19:50 - 20:24), and it was green.
Can't argue with that!
Attachment #292533 -
Flags: review?(stuart.morgan) → review+
Assignee | ||
Comment 12•17 years ago
|
||
Checked in. Let's try this again!
Status: REOPENED → RESOLVED
Closed: 17 years ago → 17 years ago
Resolution: --- → FIXED
I'm nominating this for b1 just to make sure it stays on some sort of radar for branch landing while we let it bake. Since it's blocking so many other bugs, I'm hoping we'll remember it, but you never know how we are ;)
Flags: camino1.6b1?
Assignee | ||
Updated•17 years ago
|
Flags: camino1.6b1? → camino1.6b1+
Hardware: PC → All
Target Milestone: --- → Camino1.6
Assignee | ||
Comment 14•17 years ago
|
||
I forgot to mention, and it applies to both the trunk patch and this patch: the Downloads preference pane needs its header search path to be set to:
../dist/include ../dist/include/dom ../dist/include/nspr ../dist/include/xpcom $(SYSTEM_DEVELOPER_DIR)/Headers/FlatCarbon
In my notes above, I neglected to list the FlatCarbon directory.
The one other change I needed in addition to the trunk project file update procedure was that the WebFeatures preference pane needed the header search path to include ../dist/include/cookie on the 1.8 branch. This is because nsIPermission.h is in dist/include/cookie on the 1.8 branch, but has moved into dist/include/necko on the trunk.
Assignee | ||
Comment 15•17 years ago
|
||
Tested static with Xcode 3.0 and shared with Xcode 2.5. Checked in on MOZILLA_1_8_BRANCH pre-1.6b1.
Keywords: fixed1.8.1.12
You need to log in
before you can comment on or make changes to this bug.
Description
•