Closed Bug 1390524 Opened 4 years ago Closed 4 years ago

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

Categories

(Firefox Build System :: General, enhancement)

enhancement
Not set
normal

Tracking

(firefox57 fixed)

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: froydnj, Assigned: froydnj)

References

Details

Attachments

(2 files)

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.
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.
Attachment #8897530 - Flags: review?(mh+mozilla)
Attachment #8897530 - Flags: review?(mh+mozilla) → review+
Assignee: nobody → nfroyd
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/772250315e76
correctly pass extra_toolchain_flags only to the target; r=glandium
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)
Flags: needinfo?(nfroyd)
Attached patch fix test bustageSplinter Review
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?
Attachment #8898437 - Flags: review?(mh+mozilla)
Flags: needinfo?(nfroyd)
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
Attachment #8898437 - Flags: review?(mh+mozilla) → review+
or even `target = depends(when=True)(lambda: True)`
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f3cc364d3dfd
correctly pass extra_toolchain_flags only to target compiles; r=glandium
https://hg.mozilla.org/mozilla-central/rev/f3cc364d3dfd
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.