Page MenuHomePhabricator

[zorg] Add AOSP builder
ClosedPublic

Authored by pzheng on Jan 23 2017, 2:29 PM.

Details

Summary

This builder builds AOSP with the latest Clang produced by the polly-arm-linux
builder. More specifically, only the target build uses the Clang from the
polly-arm-linux builder. The host build still uses AOSP's default Clang. This
builder is a nightly builder instead of one triggered by commits because of the
high computational resources needed for building AOSP.

Diff Detail

Event Timeline

pzheng created this revision.Jan 23 2017, 2:29 PM
pzheng updated this revision to Diff 85497.Jan 23 2017, 5:14 PM
grosser edited edge metadata.Jan 24 2017, 12:35 AM

Thanks for pushing forward to the AOSP buildbot. One thing I am not getting is how/where the AOSP source code is coming from and what is the software configuration that has to be available on the buildslave? Traditionally, our builders just check out all needed software from a git repo and only require cmake + C++ compiler. It seems this builder now needs a complex aosp checkout being available. Could you document the software configuration needed? If possibly, I would prefer to maximize the software that is automatically checked out, to make it easy to setup more buildslaves that can run this builder.

buildbot/osuosl/master/config/builders.py
680

Instead of giving a path to the installed clang, I would prefer if we build a new clang as part of the AOSP build factory. This would make sure that the SVN id that is reported in the build actually is the one that was used to build the clang/LLVM we run here.

kristof.beyls added inline comments.Jan 24 2017, 7:42 AM
buildbot/osuosl/master/config/builders.py
686

My understanding is that the 'aosp-O3-polly-before-vectorizer-unprofitable' patch is local to your builder, and needed to make it possible to use a non-AOSP-version of clang to build AOSP.
Would it be possible to also share this upstream somehow so that others could also set up AOSP builders, if wanted?

pzheng added a comment.EditedJan 24 2017, 3:27 PM

Thanks for reviewing the patch, Tobias. The AOSP source code repo is very large (over 50GB) and it takes hours to do a fresh checkout of the entire repo. Therefore, on the AOSP buildslave, I checkout the AOSP repo in advance and this is a one-time effort. After that, we can simply sync the AOSP repo (set sync=True) as necessary which only takes a few minutes. Detailed instructions for checking out AOSP is available at https://source.android.com/source/downloading.html

The software configuration for building AOSP is actually fairly simple if your build machine has a recent version of Ubuntu (>=15.04). All you need to do is to install openjdk-8-jdk. For older Ubuntu versions, the following link has detailed instructions. https://source.android.com/source/initializing.html

pzheng added inline comments.Jan 24 2017, 3:48 PM
buildbot/osuosl/master/config/builders.py
680

Sure, that makes sense. I will add another flag to control if we want to build a new clang as part of the AOSP build factory.

686

Yes, that is correct. The pre-generated patches are local to each buildslave and are needed to make it possible to use a non-AOSP-version of clang to build AOSP.

Sure, I will post the patch I use if you are interested.

Here is the patch I use to enable building AOSP with a non-AOSP-version Clang.

diff --git a/build/core/binary.mk b/build/core/binary.mk
index 4db3d51..0246ea5 100644

  • a/build/core/binary.mk

+++ b/build/core/binary.mk
@@ -371,6 +371,13 @@ ifeq ($(my_clang),false)

endif

endif

+my_target_clang := $(strip $(LOCAL_TARGET_CLANG))
+ifneq ($(strip $(TARGET_CLANG)),)
+ ifeq ($(my_target_clang),)
+ my_target_clang := true
+ endif
+endif
+

  1. clang is enabled by default for host builds
  2. enable it unless we've specifically disabled clang above ifdef LOCAL_IS_HOST_MODULE

@@ -528,6 +535,22 @@ my_target_global_cflags := $($(LOCAL_2ND_ARCH_VAR_PREFIX)CLANG_$(my_prefix)GLOBA
my_target_global_conlyflags := $($(LOCAL_2ND_ARCH_VAR_PREFIX)CLANG_$(my_prefix)GLOBAL_CONLYFLAGS) $(my_c_std_conlyflags)
my_target_global_cppflags := $($(LOCAL_2ND_ARCH_VAR_PREFIX)CLANG_$(my_prefix)GLOBAL_CPPFLAGS) $(my_cpp_std_cppflags)
my_target_global_ldflags := $($(LOCAL_2ND_ARCH_VAR_PREFIX)CLANG_$(my_prefix)GLOBAL_LDFLAGS)
+ ifeq ($(my_target_clang),true)
+ ifeq ($(strip $(my_cc)),)
+ ifeq ($(strip $(TIMEOUT)),)
+ my_cc := $(TARGET_CLANG)/clang
+ else
+ my_cc := ulimit -t $(TIMEOUT); $(TARGET_CLANG)/clang
+ endif
+ endif
+ ifeq ($(strip $(my_cxx)),)
+ ifeq ($(strip $(TIMEOUT)),)
+ my_cxx := $(TARGET_CLANG)/clang++
+ else
+ my_cxx := ulimit -t $(TIMEOUT); $(TARGET_CLANG)/clang++
+ endif
+ endif
+ endif
else
my_target_global_cflags := $($(LOCAL_2ND_ARCH_VAR_PREFIX)$(my_prefix)GLOBAL_CFLAGS)
my_target_global_conlyflags := $($(LOCAL_2ND_ARCH_VAR_PREFIX)$(my_prefix)GLOBAL_CONLYFLAGS) $(my_c_std_conlyflags)
diff --git a/build/core/clear_vars.mk b/build/core/clear_vars.mk
index 6e61d15..8cccb52 100644

  • a/build/core/clear_vars.mk

+++ b/build/core/clear_vars.mk
@@ -23,6 +23,7 @@ LOCAL_CFLAGS:=
LOCAL_CHECKED_MODULE:=
LOCAL_C_INCLUDES:=
LOCAL_CLANG:=
+LOCAL_TARGET_CLANG:=
LOCAL_CLANG_ASFLAGS:=
LOCAL_CLANG_CFLAGS:=
LOCAL_CLANG_CONLYFLAGS:=
diff --git a/build/core/definitions.mk b/build/core/definitions.mk
index dd53e7e..9ede127 100644

  • a/build/core/definitions.mk

+++ b/build/core/definitions.mk
@@ -1220,7 +1220,6 @@ endef

  1. Commands for running gcc to compile a C++ file ############### - define transform-cpp-to-o-compiler-args $(c-includes) \ -c \

@@ -1234,7 +1233,8 @@ define transform-cpp-to-o-compiler-args

	$(PRIVATE_CPPFLAGS) \
	$(PRIVATE_DEBUG_CFLAGS) \
	$(PRIVATE_CFLAGS_NO_OVERRIDE) \
  • $(PRIVATE_CPPFLAGS_NO_OVERRIDE)

+ $(PRIVATE_CPPFLAGS_NO_OVERRIDE) \
+ $(if $(findstring clang,$(PRIVATE_CXX)),$(TARGET_FLAGS))
endef

define clang-tidy-cpp
@@ -1282,7 +1282,8 @@ $(call transform-c-or-s-to-o-compiler-args, \

$(PRIVATE_CFLAGS) \
$(PRIVATE_CONLYFLAGS) \
$(PRIVATE_DEBUG_CFLAGS) \
  • $(PRIVATE_CFLAGS_NO_OVERRIDE))

+ $(PRIVATE_CFLAGS_NO_OVERRIDE)) \
+ $(if $(findstring clang,$(PRIVATE_CC)),$(TARGET_FLAGS))
endef

define clang-tidy-c

pzheng updated this revision to Diff 85657.Jan 24 2017, 4:55 PM

Thanks for explaining how to setup the buildslave. Could you add the information (the link what to install, the need for java, the information which AOSP repo to checkout, ...) as comment to the AOSPBuilder? Also, if the AOSP checkout is just a one-time thing, this could still be part of the builder, as long as we make sure we do not clean it in each build, but just update it.

Best,
Tobias

zorg/buildbot/builders/AOSPBuilder.py
85

Why do you need to list these steps again? Can you not just use include the normal Polly bullder (that you recently extended to allow the installation of clang)?

pzheng added a comment.EditedJan 24 2017, 10:31 PM

Thanks for explaining how to setup the buildslave. Could you add the information (the link what to install, the need for java, the information which AOSP repo to checkout, ...) as comment to the AOSPBuilder?

Sure, I will add these information.

Also, if the AOSP checkout is just a one-time thing, this could still be part of the builder, as long as we make sure we do not clean it in each build, but just update it.

Sorry for the confusion. Let me try to clarify this a bit more. Roughly speaking, there are two steps to checkout AOSP.

  1. repo init

This step should only be done once (manually), and therefore should not be included in any subsequent builds. This is part of the one-time setup needed for the build slave. Thus, this step is ommited from the AOSP build factory. We can include this information in the comment though.

  1. repo sync

This step does the actual checkout and is already included in the AOSP build factory. The first time you run "repo sync" after "repo init", it takes hours to finish. This is also part of the one-time setup I mentioned earlier. Fortunately, subsequent "repo sync"s only take a few minutes since it only fetches the latest changes and updates the repo.

Best,
Tobias

Thanks for explaining how to setup the buildslave. Could you add the information (the link what to install, the need for java, the information which AOSP repo to checkout, ...) as comment to the AOSPBuilder? Also, if the AOSP checkout is just a one-time thing, this could still be part of the builder, as long as we make sure we do not clean it in each build, but just update it.

Best,
Tobias

pzheng added inline comments.Jan 24 2017, 10:39 PM
zorg/buildbot/builders/AOSPBuilder.py
85

These steps are included in getPollyBuildFactory, but can not be directly used here since getPollyBuildFactory returns a BuildFactory. We can certainly do some refactoring of the code in PollyBuilder.py to make some of the code reusable by both the Polly Builder and AOSP Builder if that makes more sense to you.

grosser added inline comments.Jan 24 2017, 10:56 PM
zorg/buildbot/builders/AOSPBuilder.py
85

Why can you not just add more steps to the BuildFactory returned by getPollyBuildFactory.

getPollyLNTFactory() does this. It uses a function addLNTTestsToFactory, but as we add only steps at the end, this might not even be needed. And yes, I think avoiding code duplication makes sense here.

srhines edited edge metadata.Jan 24 2017, 11:10 PM

Are you sure that you want to always be syncing AOSP? There is a good chance that the AOSP build could break for reasons beyond the control of the LLVM community. It might be better to pick a particular AOSP release to minimize those kinds of issues. One other potential problem is going to be the increasing presence of stricter warnings/errors from Clang. These can also cause issues with AOSP.

buildbot/osuosl/master/config/builders.py
686

I agree. You should be able to point to a different clang installation today (once I submit a patch tonight/tomorrow that makes the version number issue less problematic). https://android-review.googlesource.com/q/topic:clang-3625443+(status:open%20OR%20status:merged) is the topic that I am testing in AOSP gerrit.

zorg/buildbot/builders/AOSPBuilder.py
102

You can switch from TARGET_CLANG to LLVM_PREBUILTS_BASE along with LLVM_PREBUILTS_VERSION. You might also need to set/adjust LLVM_RELEASE_VERSION (currently 4.0, but obviously upstream will move past that).

pzheng added a comment.EditedJan 25 2017, 11:51 AM

Are you sure that you want to always be syncing AOSP? There is a good chance that the AOSP build could break for reasons beyond the control of the LLVM community. It might be better to pick a particular AOSP release to minimize those kinds of issues. One other potential problem is going to be the increasing presence of stricter warnings/errors from Clang. These can also cause issues with AOSP.

Thanks for bringing this up. These are indeed valid concerns. Can you please suggest an AOSP release for use with this buildbot?

zorg/buildbot/builders/AOSPBuilder.py
85

Sure, I will make the change probably along with the one suggested by Stephen.

102

Thanks for your suggestion. Have a couple of questions.

  1. TARGET_CLANG only switches the Clang used for target build. Host build still uses the AOSP's default Clang. Can I have the same effect by using LLVM_PREBUILTS_BASE & LLVM_RELEASE_VERSION?
  2. Can you please give an example of how to use LLVM_PREBUILTS_BASE & LLVM_RELEASE_VERSION to switch the Clang for build AOSP? Let's assume the Clang we want to switch to is located at /local/aosp/llvm.inst/bin/clang(++).

Are you sure that you want to always be syncing AOSP? There is a good chance that the AOSP build could break for reasons beyond the control of the LLVM community. It might be better to pick a particular AOSP release to minimize those kinds of issues. One other potential problem is going to be the increasing presence of stricter warnings/errors from Clang. These can also cause issues with AOSP.

Thanks for bringing this up. These are indeed valid concerns. Can you please suggest an AOSP release for use with this buildbot?

http://source.android.com/source/build-numbers.html#source-code-tags-and-builds is the list of branches. So for angler, you could do the repo init with "-b android-7.1.1_r11". I only suggest this so that you don't have 2 moving targets (i.e. Clang/LLVM sources can break or be buggy, and AOSP sources can break or be buggy). There is definitely also value in having both be tested simultaneously, but I wouldn't want people to be spuriously notified when the failures are not necessarily related to the code they submitted.

zorg/buildbot/builders/AOSPBuilder.py
102
  1. I don't think it is wise to mix/match the Clang's used for the host and the target. Why not build the host components using the new Clang as well? We don't offer the ability to switch these independently in Android either, so this is just a weird configuration.
  1. The Android build system expects the BUILD_OS to be present as well, so you ideally should transform the installation path of your tools to include a "linux-x86" or "darwin-x86" component. In general, the paths are used like this:

$(LLVM_PREBUILTS_BASE)/$(BUILD_OS)-x86/$(LLVM_PREBUILTS_VERSION)/bin/clang(++)

So, let's say we update your path to look more like:
/local/aosp/linux-x86/llvm.inst/bin/clang(++)

LLVM_PREBUILTS_BASE=/local/aosp/
LLVM_PREBUILTS_VERSION=llvm.inst/
LLVM_RELEASE_VERSION=4.0 # This has to change when LLVM upstream changes their numbering too.

You can pass those env vars to the build command directly for AOSP, and it should use your new compiler for all host/device compilation.

pzheng added inline comments.Jan 25 2017, 4:53 PM
zorg/buildbot/builders/AOSPBuilder.py
102

I checked out android-7.1.1_r11 and tried running
"make LLVM_PREBUILTS_BASE=/local/aosp/ LLVM_PREBUILTS_VERSION=llvm.inst/ LLVM_RELEASE_VERSION=4.0",
but got the following error
"ninja: error: 'prebuilts/clang/host/linux-x86/llvm.inst/lib64/clang/3.8/lib/linux/libclang_rt.asan-aarch64-android.so', needed by 'out/target/product/angler/obj/SHARED_LIBRARIES/libclang_rt.asan-aarch64-android_intermediates/LINKED/libclang_rt.asan-aarch64-android.so', missing and no known rule to make it".

Is this because I used the variables incorrectly, or something else?

pzheng added inline comments.Jan 25 2017, 4:56 PM
zorg/buildbot/builders/AOSPBuilder.py
102

It seems that the variable LLVM_PREBUILTS_BASE I set did not take effect and "prebuilts/clang/host" is still used by AOSP.

srhines added inline comments.Jan 25 2017, 4:57 PM
zorg/buildbot/builders/AOSPBuilder.py
102

Can you try again with the master branch? I think that some variables were not able to be overridden in the past, and it is possible that the 7.1.1_r11 branch is too old (as compared to TOT aosp/master).

pzheng added inline comments.Jan 25 2017, 5:12 PM
zorg/buildbot/builders/AOSPBuilder.py
102

Just synced the master branch and tried the command again. Got a similar error
"ninja: error: 'prebuilts/clang/host/linux-x86/llvm.inst/test/arm/bin/asan_test', needed by 'out/target/product/angler/obj_arm/EXECUTABLES/asan-test_intermediates/asan-test', missing and no known rule to make it"

srhines added inline comments.Jan 25 2017, 5:30 PM
zorg/buildbot/builders/AOSPBuilder.py
102

Ugh, this will require some substantial number of changes to Android's build rules. Perhaps your way of hacking the build/ project is better for the short term, but honestly there are still changes needed to upstream LLVM so that it can properly build a multi-target compiler-rt. That is where these files are being pulled in from, since our toolchain build creates these multiple libraries, and then bundles those in prebuilts/clang/host/linux-x86/* (you can look at the Android.mk file there to see what I mean about what it is looking up).

One other alternative is to create a symlink in prebuilts/clang/host/linux-x86/ so that you can point to that with LLVM_PREBUILTS_VERSION. This, of course, would still require you to package the proper compiler-rt pieces in your new toolchain. I'm not sure if that is any easier to do.

A lot of this infrastructure for supporting better use of the upstream Clang toolchain is on my team's roadmap for this year, but unfortunately, it is mostly planned for much later in the year, rather than something we are working on in the short term. I can try to push some parts forward, but you most likely will still need some workarounds until that is complete.

pzheng added inline comments.Jan 25 2017, 5:46 PM
zorg/buildbot/builders/AOSPBuilder.py
102

I see. Looks like we still need my hack of the build system for now. Really looking forward to seeing a better infrastructure for supporting upstream Clang toolchain.

pzheng updated this revision to Diff 85848.Jan 25 2017, 5:55 PM

Added instructions for setting up build environment and downloading AOSP source.
Using PollyBuilder.getPollyBuildFactory to add steps for building LLVM.
Setting sync=False since we decided to use a AOSP release instead of the master. Also setting clean=False and patch=None for the same reason.

grosser accepted this revision.Jan 25 2017, 9:11 PM

Great. This looks good from my side.

This revision is now accepted and ready to land.Jan 25 2017, 9:11 PM

Are you sure that you want to always be syncing AOSP? There is a good chance that the AOSP build could break for reasons beyond the control of the LLVM community. It might be better to pick a particular AOSP release to minimize those kinds of issues. One other potential problem is going to be the increasing presence of stricter warnings/errors from Clang. These can also cause issues with AOSP.

Hi Steve,

Do you have a marker for "good" reviews? This could either be a tag or branch, but it could also be the result of some public buildbot being green.

If there isn't an easy way to do this, I agree that sticking to a known release is better, but with the caveat that the bot manager manually changes the revision every time there's a new stable.

It isn't very helpful to know we can build AOSP a year ago. :)

cheers,
--renato

Are you sure that you want to always be syncing AOSP? There is a good chance that the AOSP build could break for reasons beyond the control of the LLVM community. It might be better to pick a particular AOSP release to minimize those kinds of issues. One other potential problem is going to be the increasing presence of stricter warnings/errors from Clang. These can also cause issues with AOSP.

Hi Steve,

Do you have a marker for "good" reviews? This could either be a tag or branch, but it could also be the result of some public buildbot being green.

Nope. There are no public AOSP buildbots maintained by Google, nor do I know of any current plans to expose this (although I wish we would do it). Part of the problem is that it really comes down to knowing that a given manifest is acceptable, so it is more than just a single CL.

If there isn't an easy way to do this, I agree that sticking to a known release is better, but with the caveat that the bot manager manually changes the revision every time there's a new stable.

Maintenance Releases (MRs) come out fairly often, so it's not like this is only a once per year thing.

It isn't very helpful to know we can build AOSP a year ago. :)

True, but there is a lot of AOSP that doesn't change (external/ projects rarely get modified that much). We have definitely seen instability from upstream Clang (ha, that's what I am up chasing right now), so there is value in just compiling a large codebase like AOSP with an active Clang.

cheers,
--renato

Maintenance Releases (MRs) come out fairly often, so it's not like this is only a once per year thing.

This should be more than enough. Is this a tar-ball or a tag?

We should try to automate some version check to only update when there's a new release, but this sounds like the thing I was referring to.

--renato

Maintenance Releases (MRs) come out fairly often, so it's not like this is only a once per year thing.

This should be more than enough. Is this a tar-ball or a tag?

It's a tag (http://source.android.com/source/build-numbers.html#source-code-tags-and-builds shows the "branch" name that you can use with "repo init -b".

We should try to automate some version check to only update when there's a new release, but this sounds like the thing I was referring to.

You can automate some of this, but you really do need at least one run to verify that the build is acceptable before you would want to expect good results for everyone else.

--renato

It's a tag (http://source.android.com/source/build-numbers.html#source-code-tags-and-builds shows the "branch" name that you can use with "repo init -b".

Right, this should be easy, then.

You can automate some of this, but you really do need at least one run to verify that the build is acceptable before you would want to expect good results for everyone else.

Good point.

pzheng added a comment.EditedJan 26 2017, 11:27 AM

I had a build of the AOSP release "android-7.1.1_r11" with the latest LLVM toolchain and noticed a lot of AOSP source code issues (compared to using the AOSP master branch) due to stricter compiler checks. Looks like many of the issues have already been fixed in AOSP mater but those fixes have not made it into the release yet. For now, I propose to use the AOSP master until a more "stable" release comes out.

Sounds good. Just choose a version that you verified builds correctly and we can update it to the next release, when it builds correctly as well.

Would it not be better to suppress those warnings via an additional set of flags instead for the stable release? aosp/master breaks at least once per week, and I think that this might be seen as quite disruptive to LLVM committers. If you don't repo sync, then perhaps it is also ok. One other alternative is to pull the manifest.xml for a working version that you like, and use that in your repo init (i.e. check that in as a build configuration for the bot, and use it in the initial configure). Then you can just move the manifest forward as you see fit.

https://android-review.googlesource.com/#/c/328784/ may fix the problem with the top-level path. I think that more is still needed there too, but I just wanted to make you aware (so that perhaps the build patch hack can go away).

I have already added "-Wno-error" in the flags used to build AOSP, but there are still many not suppressible. For example,

bootable/recovery/minui/resources.cpp:271:16: error: ordered comparison between pointer and zero ('int *' and 'int')

if (frames <= 0 || fps <= 0) {
    ~~~~~~ ^  ~

bootable/recovery/minui/resources.cpp:271:28: error: ordered comparison between pointer and zero ('int *' and 'int')

if (frames <= 0 || fps <= 0) {
                   ~~~ ^  ~

2 errors generated.

Can you please elaborate on how to use the manifest.xml file as you suggested?

repo init -m <stable-manifest.xml>

You can just take .repo/manifest.xml for something you validated and save that file off as <stable-manifest.xml>. Then you can use it as part of your init. Alternately, you can copy it into .repo/manifest.xml after doing your regular repo init (which will overwrite that copy).

Thanks for explaining the usage of the manifest file. We will use the current manifest file for the AOSP master and won't sync the repo until a better one is found.

pzheng closed this revision.Jan 26 2017, 1:50 PM