1390524 - extra_toolchain_flags applies to host and target compiles, when it should only apply to target

Status: RESOLVED FIXED

(Firefox Build System :: General, enhancement)

Description

[Nathan Froyd [:froydnj]]

I discovered this today when I attempted to modify extra_toolchain_flags on Android to pass the correct flags for clang:

@depends(android_platform, android_toolchain)
def extra_toolchain_flags(platform_dir, toolchain_dir):
    if not platform_dir:
        return []
    # -idirafter does not work correctly with llvm; the default #include search
    # paths include host paths outside of the NDK (!), so using -idirafter
    # inserts the android platform's /usr/include after the host's /usr/include,
    # which obviously doesn't work that well.  Using -isystem puts things in
    # the correct place.
    return ['-isystem',
            os.path.join(platform_dir, 'usr', 'include'),
            '-gcc-toolchain',
            toolchain_dir]

and then running configure says:

checking the host C compiler works... no
DEBUG: Creating `/tmp/conftest.qZJV7T.c` with content:
DEBUG: |
DEBUG: | int
DEBUG: | main(void)
DEBUG: | {
DEBUG: |
DEBUG: |   ;
DEBUG: |   return 0;
DEBUG: | }
DEBUG: Executing: `/usr/bin/gcc -std=gnu99 -isystem /home/froydnj/.mozbuild/android-ndk-r11b/platforms/android-9/arch-arm/usr/include -gcc-toolchain /home/froydnj/.mozbuild/android-ndk-r11b/toolchains/arm-linux-androideabi-4.9/prebuilt/linux-x86_64 -c /tmp/conftest.qZJV7T.c`

Notice that we're passing the aforementioned flags to the host compiler.  I think we get away with this in our current Android builds because we're passing -idirafter, which the host GCC understands, and which is harmless.  But -gcc-toolchain is obviously clang-specific, and attempting to pass it along to the host results in badness.

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: correctly pass extra_toolchain_flags only to the target

Inspecting logs suggests that this passing of flags only happens during
moz.configure, so it's not as bad as it could be, but it's still something we
should fix.

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/772250315e76
correctly pass extra_toolchain_flags only to the target; r=glandium

Comment 3

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for build bustage in test_compile_checks.py: compiler_class() takes exactly 2 arguments (1 given):

https://hg.mozilla.org/integration/mozilla-inbound/rev/2a790986a5716f9a070a601019de2a244f13e150

Failure log: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=abc85ce83caa024f5832f17a4955047a76520e95&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable

> TEST-UNEXPECTED-FAIL | /home/worker/workspace/build/src/python/mozbuild/mozbuild/test/configure/test_compile_checks.py | TestHeaderChecks.test_check_header, line 176: compiler_class() takes exactly 2 arguments (1 given)

Comment 4

[Nathan Froyd [:froydnj]]

Attachment: fix test bustage

Like an idiot, I forgot to run the configure tests, and of course there was
bustage.  We were abusing compiler_class as a decorator, which causes
problems.  This is the easiest way I can think of to fix it, but the fix was
complicated enough that I thought it was worth somebody taking a second look.
WDYT?

Comment 5

[Mike Hommey [:glandium]]

Comment on attachment 8898437 [details] [diff] [review]
fix test bustage

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

::: python/mozbuild/mozbuild/test/configure/test_compile_checks.py
@@ +60,5 @@
> +                if isfunction(obj):
> +                    return depends(when=True)(obj)
> +                return depends(when=True)(lambda: obj)
> +
> +            target = dependable(True)

that's a lot of code for something that could be written as:
  @depends(when=True):
  def target():
      return True

Comment 6

[Mike Hommey [:glandium]]

or even `target = depends(when=True)(lambda: True)`

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f3cc364d3dfd
correctly pass extra_toolchain_flags only to target compiles; r=glandium

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f3cc364d3dfd