Add a TaskCluster job to build clang-tidy for OSX

RESOLVED FIXED in Firefox 53

Status

RESOLVED FIXED
2 years ago
6 months ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

unspecified
mozilla53
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

2 years ago
I have added support for cross-compiling LLVM for OS X on our Linux64 builders.  We can now run them through TaskCluster!

Initially I used toolchain-script similar to the Linux version of this script, but one issue that I have faced is that I needed to download the OS X 10.10 SDK from tooltool with visibility: internal but I couldn't get that to work because I couldn't find a way to download the /builds/relengapi.tok authentication file.  I have an unfinished patch for that and will file it separately.

Since I knew that mozharness jobs run through taskcluster can download internal tooltool resources (since our Mac cross-compilers download the internal OSX 10.7 SDK package) I switched to using mozharsness and the tooltool download issue is bypassed that way.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7c008085a785c385dcff7cd2e21d58257b67ee6b
(Assignee)

Updated

2 years ago
See Also: → bug 1328200
(Assignee)

Comment 1

2 years ago
Created attachment 8823157 [details] [diff] [review]
Part 1: Add support for cross-compiling clang for OS X on Linux
Attachment #8823157 - Flags: review?(michael)
(Assignee)

Comment 2

2 years ago
Created attachment 8823158 [details] [diff] [review]
Part 2: Add a TaskCluster job to build clang-tidy for OSX using mozharness
Attachment #8823158 - Flags: review?(dustin)
Comment on attachment 8823158 [details] [diff] [review]
Part 2: Add a TaskCluster job to build clang-tidy for OSX using mozharness

Review of attachment 8823158 [details] [diff] [review]:
-----------------------------------------------------------------

I'm really only capable of looking at the taskcluster/ci/** stuff, but that looks good to me.  I don't mind adding a new use of mozharness, but if it's easier to just use the relengapi proxy directly from a shell script, then I'm happy with that too.
Attachment #8823158 - Flags: review?(dustin) → review+
Comment on attachment 8823157 [details] [diff] [review]
Part 1: Add support for cross-compiling clang for OS X on Linux

Review of attachment 8823157 [details] [diff] [review]:
-----------------------------------------------------------------

::: build/build-clang/README
@@ +45,5 @@
>  * patches: Optional list of patches to apply per platform.  Supported platforms: macosx64, linux32, linux64.  The default is Release.
>  * build_type: The type of build to make.  Supported types: Release, Debug, RelWithDebInfo or MinSizeRel.
>  * build_libcxx: Whether to build with libcxx.  The default is false.
>  * import_clang_tidy: Whether to import Mozilla checks into clang-tidy before building.  The default is false.
> +* osx_cross_compile: Whether to invoke CMake for OS X cross compile builds.

I am not the biggest fan of the name "osx_cross_compile", because it seems very specific, but I'm fine with it.

::: build/build-clang/build-clang.py
@@ +229,5 @@
>      if is_windows():
>          cmake_args.insert(-1, "-DLLVM_EXPORT_SYMBOLS_FOR_PLUGINS=ON")
> +    if ranlib is not None:
> +        cmake_args += ["-DCMAKE_RANLIB=%s" % slashify_path(ranlib)]
> +    if osx_cross_compile:

Can we assert that we're on linux here? I doubt that this codepath would work well otherwise.

@@ +241,5 @@
> +                       "-DCMAKE_FIND_ROOT_PATH_MODE_LIBRARY=ONLY",
> +                       "-DCMAKE_FIND_ROOT_PATH_MODE_INCLUDE=ONLY",
> +                       "-DCMAKE_MACOSX_RPATH=@executable_path",
> +                       "-DCMAKE_OSX_ARCHITECTURES=x86_64",
> +                       "-DDARWIN_osx_ARCHS=x86_64",

This name looks like it might be a mistake (the lowercase "osx"), but I don't know enough off of the top of my head about clang's buildsystem to be able to say. A quick grep in my (admittedly outdated) clone of llvm+clang didn't find this variable mentioned anywhere.
Attachment #8823157 - Flags: review?(michael) → review+
(Assignee)

Comment 5

2 years ago
(In reply to Dustin J. Mitchell [:dustin] from comment #3)
> Comment on attachment 8823158 [details] [diff] [review]
> Part 2: Add a TaskCluster job to build clang-tidy for OSX using mozharness
> 
> Review of attachment 8823158 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm really only capable of looking at the taskcluster/ci/** stuff, but that
> looks good to me.  I don't mind adding a new use of mozharness, but if it's
> easier to just use the relengapi proxy directly from a shell script, then
> I'm happy with that too.

Bug 1328172 was the only reason why I used MozHarness here.  If we can easily fix that, I'd be happy to avoid MozHarness in this job similar to its sibling jobs.
(Assignee)

Updated

2 years ago
Depends on: 1328172
(Assignee)

Comment 6

2 years ago
(In reply to Michael Layzell [:mystor] from comment #4)
> Comment on attachment 8823157 [details] [diff] [review]
> Part 1: Add support for cross-compiling clang for OS X on Linux
> 
> Review of attachment 8823157 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: build/build-clang/README
> @@ +45,5 @@
> >  * patches: Optional list of patches to apply per platform.  Supported platforms: macosx64, linux32, linux64.  The default is Release.
> >  * build_type: The type of build to make.  Supported types: Release, Debug, RelWithDebInfo or MinSizeRel.
> >  * build_libcxx: Whether to build with libcxx.  The default is false.
> >  * import_clang_tidy: Whether to import Mozilla checks into clang-tidy before building.  The default is false.
> > +* osx_cross_compile: Whether to invoke CMake for OS X cross compile builds.
> 
> I am not the biggest fan of the name "osx_cross_compile", because it seems
> very specific, but I'm fine with it.

Well, this variable is only intended to be used for OXS targeting cross compiles on Linux, so the specificity of the name is intentional.  Can you think of a better name?

> ::: build/build-clang/build-clang.py
> @@ +229,5 @@
> >      if is_windows():
> >          cmake_args.insert(-1, "-DLLVM_EXPORT_SYMBOLS_FOR_PLUGINS=ON")
> > +    if ranlib is not None:
> > +        cmake_args += ["-DCMAKE_RANLIB=%s" % slashify_path(ranlib)]
> > +    if osx_cross_compile:
> 
> Can we assert that we're on linux here? I doubt that this codepath would
> work well otherwise.

Definitely a good idea.

> @@ +241,5 @@
> > +                       "-DCMAKE_FIND_ROOT_PATH_MODE_LIBRARY=ONLY",
> > +                       "-DCMAKE_FIND_ROOT_PATH_MODE_INCLUDE=ONLY",
> > +                       "-DCMAKE_MACOSX_RPATH=@executable_path",
> > +                       "-DCMAKE_OSX_ARCHITECTURES=x86_64",
> > +                       "-DDARWIN_osx_ARCHS=x86_64",
> 
> This name looks like it might be a mistake (the lowercase "osx"), but I
> don't know enough off of the top of my head about clang's buildsystem to be
> able to say. A quick grep in my (admittedly outdated) clone of llvm+clang
> didn't find this variable mentioned anywhere.

It isn't a typo, it's referenced here: <https://llvm.org/svn/llvm-project/compiler-rt/trunk/cmake/builtin-config-ix.cmake>

See the default assignment which screws us over, since our cctools are too old for x86_64h:

  if(NOT DARWIN_osx_ARCHS)
    set(DARWIN_osx_ARCHS i386 x86_64 x86_64h)
  endif()
(Assignee)

Updated

2 years ago
Blocks: 1328454
(Assignee)

Comment 7

2 years ago
Created attachment 8823522 [details] [diff] [review]
Part 2: Add a TaskCluster job to build clang-tidy for OSX on Linux

With the fix to bug 1328172, this can now be done using toolchain-script like other similar jobs, which is much simpler.  I decided to just redo things here.

Green on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2699b38c721752976231d747c66652b2221cc015
Attachment #8823522 - Flags: review?(dustin)
(Assignee)

Updated

2 years ago
Attachment #8823158 - Attachment is obsolete: true
Comment on attachment 8823522 [details] [diff] [review]
Part 2: Add a TaskCluster job to build clang-tidy for OSX on Linux

Review of attachment 8823522 [details] [diff] [review]:
-----------------------------------------------------------------

::: taskcluster/scripts/misc/build-clang-tidy-macosx.sh
@@ +1,4 @@
> +#!/bin/bash
> +set -x -e -v
> +
> +# This script is for building clang for Linux.

..for Mac OS X :)
Attachment #8823522 - Flags: review?(dustin) → review+

Comment 9

2 years ago
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f6512e0df57
Part 1: Add support for cross-compiling clang for OS X on Linux; r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/0026f22490e2
Part 2: Add a TaskCluster job to build clang-tidy for OSX on Linux; r=dustin

Comment 11

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8f6512e0df57
https://hg.mozilla.org/mozilla-central/rev/0026f22490e2
https://hg.mozilla.org/mozilla-central/rev/229a81285dff
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Depends on: 1338008

Updated

6 months ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.