Build pref-pane as universal binary on Mac OS X (issue 10383204)

16 views
Skip to first unread message

lambros...@chromium.org

unread,
May 15, 2012, 9:31:52 PM5/15/12
to jamie...@chromium.org, chromium...@chromium.org, jamiewal...@chromium.org, dcaiaf...@chromium.org, simonmor...@chromium.org, hclam...@chromium.org, wez+...@chromium.org, am...@chromium.org, sanj...@chromium.org, garyka...@chromium.org, lambroslam...@chromium.org, alexeyp...@chromium.org, sergey...@chromium.org
Reviewers: Jamie,

Description:
Build pref-pane as universal binary on Mac OS X


BUG=125116
TEST=manual


Please review this at http://codereview.chromium.org/10383204/

SVN Base: svn://svn.chromium.org/chrome/trunk/src

Affected files:
M remoting/host/me2me_preference_pane.mm
M remoting/remoting.gyp


rse...@chromium.org

unread,
May 16, 2012, 10:34:04 AM5/16/12
to lambros...@chromium.org, jamie...@chromium.org, chromium...@chromium.org, jamiewal...@chromium.org, dcaiaf...@chromium.org, simonmor...@chromium.org, hclam...@chromium.org, wez+...@chromium.org, am...@chromium.org, sanj...@chromium.org, garyka...@chromium.org, lambroslam...@chromium.org, alexeyp...@chromium.org, sergey...@chromium.org

http://codereview.chromium.org/10383204/diff/1/remoting/host/me2me_preference_pane.mm
File remoting/host/me2me_preference_pane.mm (right):

http://codereview.chromium.org/10383204/diff/1/remoting/host/me2me_preference_pane.mm#newcode142
remoting/host/me2me_preference_pane.mm:142: pid_t PIDForJob(const
std::string& job_label) {
http://code.google.com/codesearch#OAMlx_jo-ck/src/base/mac/launchd.h&exact_package=chromium&q=pidforjob&type=cs

http://codereview.chromium.org/10383204/

rse...@chromium.org

unread,
May 16, 2012, 10:37:43 AM5/16/12
to lambros...@chromium.org, jamie...@chromium.org, chromium...@chromium.org, jamiewal...@chromium.org, dcaiaf...@chromium.org, simonmor...@chromium.org, hclam...@chromium.org, wez+...@chromium.org, am...@chromium.org, sanj...@chromium.org, garyka...@chromium.org, lambroslam...@chromium.org, alexeyp...@chromium.org, sergey...@chromium.org

http://codereview.chromium.org/10383204/diff/1/remoting/host/me2me_preference_pane.mm
File remoting/host/me2me_preference_pane.mm (right):

http://codereview.chromium.org/10383204/diff/1/remoting/host/me2me_preference_pane.mm#newcode142
remoting/host/me2me_preference_pane.mm:142: pid_t PIDForJob(const
std::string& job_label) {
On 2012/05/16 14:34:04, rsesek wrote:

http://code.google.com/codesearch#OAMlx_jo-ck/src/base/mac/launchd.h&exact_package=chromium&q=pidforjob&type=cs

Sorry, just saw your comment. Got confused by seeing other #include
"base/..." files.

Though I thought we supported building base as 64-bit? I tried it a few
weeks ago and it worked by just setting the archs properly.

http://codereview.chromium.org/10383204/

lambros...@chromium.org

unread,
May 16, 2012, 1:43:44 PM5/16/12
to jamie...@chromium.org, chromium...@chromium.org, jamiewal...@chromium.org, dcaiaf...@chromium.org, simonmor...@chromium.org, hclam...@chromium.org, wez+...@chromium.org, am...@chromium.org, sanj...@chromium.org, garyka...@chromium.org, lambroslam...@chromium.org, alexeyp...@chromium.org, sergey...@chromium.org, rse...@chromium.org

http://codereview.chromium.org/10383204/diff/1/remoting/host/me2me_preference_pane.mm
File remoting/host/me2me_preference_pane.mm (right):

http://codereview.chromium.org/10383204/diff/1/remoting/host/me2me_preference_pane.mm#newcode142
remoting/host/me2me_preference_pane.mm:142: pid_t PIDForJob(const
std::string& job_label) {
On 2012/05/16 14:37:43, rsesek wrote:
> On 2012/05/16 14:34:04, rsesek wrote:
> >

http://code.google.com/codesearch#OAMlx_jo-ck/src/base/mac/launchd.h&exact_package=chromium&q=pidforjob&type=cs

> Sorry, just saw your comment. Got confused by seeing other #include
"base/..."
> files.

> Though I thought we supported building base as 64-bit? I tried it a
few weeks
> ago and it worked by just setting the archs properly.
mark@ told me it works, but it's currently untested and unsupported.
It's something we want to do, but we'd have to get support from Chrome -
it seems a large change to make to Chrome, just for a small pref-pane
application for Chromoting :)

I will followup with a post to chromium-dev. In the meantime, I welcome
any suggestions you have.

http://codereview.chromium.org/10383204/

tha...@chromium.org

unread,
May 16, 2012, 4:22:42 PM5/16/12
to lambros...@chromium.org, jamie...@chromium.org, chromium...@chromium.org, jamiewal...@chromium.org, dcaiaf...@chromium.org, simonmor...@chromium.org, hclam...@chromium.org, wez+...@chromium.org, am...@chromium.org, sanj...@chromium.org, garyka...@chromium.org, lambroslam...@chromium.org, alexeyp...@chromium.org, sergey...@chromium.org, rse...@chromium.org
As discussed over IRC, i'll look into adding basic ARCHS support to ninja
early
next week.


http://codereview.chromium.org/10383204/diff/1/remoting/remoting.gyp
File remoting/remoting.gyp (right):

http://codereview.chromium.org/10383204/diff/1/remoting/remoting.gyp#newcode415
remoting/remoting.gyp:415: 'ARCHS': ['i386', 'x86_64'],
This isn't supported in ninja yet, so it won't pass the trybots (and
will break them if you land manually)

http://codereview.chromium.org/10383204/diff/1/remoting/remoting.gyp#newcode416
remoting/remoting.gyp:416: 'GCC_ENABLE_OBJC_GC': 'supported',
ninja ignores this setting at the moment. i guess this needs to be set
for prefpanes? (i can add support for it)

http://codereview.chromium.org/10383204/

jamie...@chromium.org

unread,
May 16, 2012, 4:31:34 PM5/16/12
to lambros...@chromium.org, tha...@chromium.org, chromium...@chromium.org, jamiewal...@chromium.org, dcaiaf...@chromium.org, simonmor...@chromium.org, hclam...@chromium.org, wez+...@chromium.org, am...@chromium.org, sanj...@chromium.org, garyka...@chromium.org, lambroslam...@chromium.org, alexeyp...@chromium.org, sergey...@chromium.org, rse...@chromium.org
lgtm once Nico's comments are addressed. It might also be worth holding off
on
this until we've had some feedback from chromium-dev about making 64-bit
more
widely supported.


http://codereview.chromium.org/10383204/diff/1/remoting/host/me2me_preference_pane.mm
File remoting/host/me2me_preference_pane.mm (left):

http://codereview.chromium.org/10383204/diff/1/remoting/host/me2me_preference_pane.mm#oldcode203
remoting/host/me2me_preference_pane.mm:203: DCHECK(result);
In changing these DCHECKs, you're changing the Release-build behaviour.
I think what you have now is better, but be aware of the change.

http://codereview.chromium.org/10383204/

lambros...@chromium.org

unread,
May 17, 2012, 2:03:29 PM5/17/12
to jamie...@chromium.org, tha...@chromium.org, chromium...@chromium.org, jamiewal...@chromium.org, dcaiaf...@chromium.org, simonmor...@chromium.org, hclam...@chromium.org, wez+...@chromium.org, am...@chromium.org, sanj...@chromium.org, garyka...@chromium.org, lambroslam...@chromium.org, alexeyp...@chromium.org, sergey...@chromium.org, rse...@chromium.org

http://codereview.chromium.org/10383204/diff/1/remoting/remoting.gyp
File remoting/remoting.gyp (right):

http://codereview.chromium.org/10383204/diff/1/remoting/remoting.gyp#newcode415
remoting/remoting.gyp:415: 'ARCHS': ['i386', 'x86_64'],
On 2012/05/16 20:22:43, Nico wrote:
> This isn't supported in ninja yet, so it won't pass the trybots (and
will break
> them if you land manually)
OK, I'll hold off on landing the "ARCHS" change.

http://codereview.chromium.org/10383204/diff/1/remoting/remoting.gyp#newcode416
remoting/remoting.gyp:416: 'GCC_ENABLE_OBJC_GC': 'supported',
On 2012/05/16 20:22:43, Nico wrote:
> ninja ignores this setting at the moment. i guess this needs to be set
for
> prefpanes? (i can add support for it)
The 64-bit prefpane needs to support GC, otherwise it triggers a "GC
mismatch" error and the System Preferences applet restarts in 32-bit
mode.

http://codereview.chromium.org/10383204/
Reply all
Reply to author
Forward
0 new messages