make distcc builds work for both HOST and TARGET compilation

RESOLVED FIXED in 2.0 S5 (4july)

Status

Firefox OS
General
P2
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: huseby, Assigned: huseby)

Tracking

({perf})

unspecified
2.0 S5 (4july)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [c=tools p=1 s= u=])

Attachments

(2 attachments)

(Assignee)

Description

3 years ago
Currently the TARGET portion of the b2g build uses relative paths to the cross-compiling toolchain.  This prevents distcc from working because it can only handle generic executable calls resolved via search path (e.g. "gcc -c foo.c") and full path executable calls (e.g. "/opt/gcc -c foo.c").  With relative paths, the remote distcc hosts can't find the correct executable to run.
(Assignee)

Updated

3 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 1

3 years ago
Created attachment 8373642 [details] [review]
patch to force make to use fully qualified paths to compiler executables when USE_DISTCC is defined
Attachment #8373642 - Flags: review?(mwu)
Attachment #8373642 - Flags: review?(fabrice)
Comment on attachment 8373642 [details] [review]
patch to force make to use fully qualified paths to compiler executables when USE_DISTCC is defined

I'll let mwu and vicamo review.
Attachment #8373642 - Flags: review?(fabrice)
(Assignee)

Updated

3 years ago
Attachment #8373642 - Flags: review?(vyang)

Comment 3

3 years ago
We should make a TARGET_TOOLCHAIN_ROOT variable and run realpath on that, and then use it to generate TARGET_TOOLS_PREFIX.
(In reply to Michael Wu [:mwu] from comment #3)
> We should make a TARGET_TOOLCHAIN_ROOT variable and run realpath on that,
> and then use it to generate TARGET_TOOLS_PREFIX.

But that doesn't work on host toolchain.  For HOST_darwin-x86 and HOST_linux-x86, HOST_{CC,CXX} are aliased to $(CC) and $(CXX) directly.  For HOST_windows-x86, "TOOLS_PREFIX" is used but it's still assigned to empty string by now.  Creating a TARGET_TOOLCHAIN_ROOT, and maybe HOST_TOOLCHAIN_ROOT as well, means you have to touch many many files just like what we had in that pull request.

So why not call $(realpath $($(combo_target)CC)) directly?  "build/core/combo/select.mk" is used to select both TARGET_ and HOST_ mk files.  One can easily get absolute path for those variables by:

  ifneq ($(USE_DISTCC),)
    $(combo_target)CC := $(realpath $($(combo_target)CC))
    $(combo_target)CXX := $(realpath $($(combo_target)CXX))
  endif

in four lines.  Just four lines and you get it for all platforms, both host and target.
Attachment #8373642 - Flags: review?(vyang) → feedback-
(Assignee)

Comment 5

3 years ago
Let me rework this patch to include your suggestions and see if that works.  Thanks for the feedback.
(Assignee)

Updated

3 years ago
Keywords: perf
Whiteboard: [c= p= s= u=]
(Assignee)

Updated

3 years ago
Whiteboard: [c= p= s= u=] → [c= p=1 s= u=]

Updated

3 years ago
Priority: -- → P2
Whiteboard: [c= p=1 s= u=] → [c=tool p=1 s= u=]

Updated

3 years ago
Whiteboard: [c=tool p=1 s= u=] → [c=tools p=1 s= u=]

Updated

3 years ago
Attachment #8373642 - Flags: review?(mwu)
(Assignee)

Comment 6

3 years ago
Comment on attachment 8373642 [details] [review]
patch to force make to use fully qualified paths to compiler executables when USE_DISTCC is defined

I updated the pull request to reflect the feedback.
Attachment #8373642 - Flags: review?(vyang)
Attachment #8373642 - Flags: review?(mwu)
(Assignee)

Updated

3 years ago
Attachment #8373642 - Attachment description: patch to force make to use fully qualified paths to TARGET executables → patch to force make to use fully qualified paths compiler executables when USE_DISTCC is defined
Attachment #8373642 - Flags: feedback-
(Assignee)

Updated

3 years ago
Attachment #8373642 - Attachment description: patch to force make to use fully qualified paths compiler executables when USE_DISTCC is defined → patch to force make to use fully qualified paths to compiler executables when USE_DISTCC is defined
(Assignee)

Comment 7

3 years ago
I updated the patch to just modify select.mk as suggested in the bugzilla bug. This is all that is needed in the platform_build to make distcc work correctly. The user still needs to have a .userconfig that has something like this in it:

export CC=gcc-4.6
export CXX=g++-4.6
export USE_DISTCC=${USE_DISTCC:-}

# enable distcc if specified
if [ "${USE_DISTCC}" == "1" ]; then
  export CC=`which ${CC}`
  export CXX=`which ${CXX}`
  export CCACHE_PREFIX="distcc"
fi

This will make sure that both CC and CXX have full static path's to the 4.6 compiler on their machine as well as set distcc to be the "prefix command" that ccache uses when calling either compiler. The prefix command settings is essential to making ccache and distcc work well together. 

My results are pretty dramatic. I have a rMBP with a quad-core i7 in it. A full rebuild from scratch on that machine alone takes 45 minutes. I also have a dual quad-core Xeon workstation that I used for distcc and with it in the pool, the full rebuild time drops to 18 mintues. When bisecting bugs in gecko, the combination of ccache and distcc drops the build times to well under 5 minutes. I've seen several builds of only ~1m45s.
Comment on attachment 8373642 [details] [review]
patch to force make to use fully qualified paths to compiler executables when USE_DISTCC is defined

Not a Gonk peer, so I place only f+.
Attachment #8373642 - Flags: review?(vyang) → feedback+

Updated

3 years ago
Attachment #8373642 - Flags: review?(mwu) → review+
(Assignee)

Comment 9

3 years ago
merged: http://bit.ly/MNMXPl

NOTE: this patch does NOT work with the new 4.3 and 4.4 branches of platform_build.  That means it cannot be pulled into the nexus-4 and nexus-4-kk builds as-is.  I was seeing build errors in libicuuc.
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
(Assignee)

Comment 10

3 years ago
Created attachment 8443149 [details] [review]
a patch to enable distcc for v1.3t tarako builds

the first patch for this bug never made it into v1.3t for tarako builds.  this new patch fixes that.
Attachment #8443149 - Flags: review?(mwu)
Attachment #8443149 - Flags: feedback?(vyang)
(Assignee)

Comment 11

3 years ago
re-opening to get the v1.3t patch reviewed and landed.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 8443149 [details] [review]
a patch to enable distcc for v1.3t tarako builds

I think the thing you really need is to ask for 1.3T approval.
Attachment #8443149 - Flags: feedback?(vyang)

Comment 13

3 years ago
Comment on attachment 8443149 [details] [review]
a patch to enable distcc for v1.3t tarako builds

I think it's too late to ask for things on 1.3t unless they're showstoppers.
Attachment #8443149 - Flags: review?(mwu)
(Assignee)

Comment 14

3 years ago
alrighty, I'll just keep it as a patch I apply locally.
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
Resolution: --- → FIXED

Updated

3 years ago
Target Milestone: --- → 2.0 S5 (4july)
You need to log in before you can comment on or make changes to this bug.