Closed Bug 1328199 Opened 7 years ago Closed 7 years ago

Add a TaskCluster job to build clang-tidy for OSX

Categories

(Developer Infrastructure :: Source Code Analysis, defect)

defect
Not set
normal

Tracking

(firefox53 fixed)

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)

References

Details

Attachments

(2 files, 1 obsolete file)

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
See Also: → 1328200
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+
(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.
Depends on: 1328172
(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()
Blocks: 1328454
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)
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+
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
Depends on: 1338008
Product: Core → Firefox Build System
Product: Firefox Build System → Developer Infrastructure
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: