Closed Bug 1318370 Opened 7 years ago Closed 5 years ago

Set Windows compiler options appropriately when using sccache

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: ted, Assigned: ted)

References

Details

Attachments

(3 obsolete files)

We currently do a bunch of fiddling in mozconfig.cache to set compiler flags and unset the compiler wrapper that we use to generate depfiles on Windows:
https://dxr.mozilla.org/mozilla-central/rev/13f49da109ea460665ad27c8497cb1489548450c/build/mozconfig.cache#121

My patch in bug 1286934 will add some minimal bits to make `--with-ccache=sccache` work. We should move the rest of this into moz.configure as well so that when we start telling developers to use sccache they can just do `--with-ccache=sccache`.
Depends on: 1322703
Assignee: nobody → ted
Attachment #8830676 - Attachment is obsolete: true
Attachment #8830676 - Flags: review?(mh+mozilla)
Comment on attachment 8830677 [details]
bug 1318370 - move _DEPEND_CFLAGS+CL_INCLUDES_PREFIX to toolchain.configure, ignore {CC,CXX}_WRAPPER when using sccache

https://reviewboard.mozilla.org/r/107402/#review109794

::: build/moz.configure/toolchain.configure:974
(Diff revision 1)
> +@depends(c_compiler, using_sccache)
> +def depend_cflags(c_compiler, using_sccache):
> +    if c_compiler.type != 'msvc':
> +        return '-MD -MP -MF $(MDDEPDIR)/$(@F).pp'
> +    elif using_sccache:
> +        # sccache supports parsing -showIncludes

The comment in old-configure was actually better.

::: build/moz.configure/toolchain.configure:979
(Diff revision 1)
> +        # sccache supports parsing -showIncludes
> +        return '-deps$(MDDEPDIR)/$(@F).pp'
> +
> +set_config('_DEPEND_CFLAGS', depend_cflags)
> +
> +@depends_when(c_compiler, when=building_with_msvc)

You can actually do depends(foo, when=...) since bug 1296530. I got distracted and never filed the bug to remove depends_when. Will do so after this review...

::: build/moz.configure/toolchain.configure:998
(Diff revision 1)
> +# Make sure that the build system can handle non-ASCII characters
> +# in environment variables to prevent it from breaking silently on
> +# non-English systems.
> +set_config('NONASCII',
> +           depends_if(c_compiler)(lambda info: u'\241\241' if info.type == 'msvc' else None))

I think we can remove this thing. IIRC, it actually didn't catch anything the last time CL_INCLUDES_PREFIX was broken on non-English locales.

Plus, you're returning a unicode string here while msvc_showincludes_prefix is likely to return a str in the windows native encoding (mbcs), so this test is, in fact, not relevant in this form.

It would be better to have some Japanese windows developer check that this doesn't break for them, and in the long term, to have a taskcluster job that installs MSVC in japanese and tries to run mach configure.

::: build/mozconfig.cache
(Diff revision 1)
> -    case "$master" in
> -    *us[ew][12].mozilla.com*|*euc1.mozilla.com*)
> -        mk_add_options "export SCCACHE_NAMESERVER=169.254.169.253"
> -        ;;
> -    esac

Please move this in a separate patch that states why it is removed, which has not much to do with the commit message associated with this patch.
Attachment #8830677 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8830680 [details]
bug 1318370 - stop using -Z7 for MSVC compilation with sccache.

https://reviewboard.mozilla.org/r/107412/#review109798
Attachment #8830680 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8830677 [details]
bug 1318370 - move _DEPEND_CFLAGS+CL_INCLUDES_PREFIX to toolchain.configure, ignore {CC,CXX}_WRAPPER when using sccache

https://reviewboard.mozilla.org/r/107402/#review109794

> I think we can remove this thing. IIRC, it actually didn't catch anything the last time CL_INCLUDES_PREFIX was broken on non-English locales.
> 
> Plus, you're returning a unicode string here while msvc_showincludes_prefix is likely to return a str in the windows native encoding (mbcs), so this test is, in fact, not relevant in this form.
> 
> It would be better to have some Japanese windows developer check that this doesn't break for them, and in the long term, to have a taskcluster job that installs MSVC in japanese and tries to run mach configure.

I filed bug 1340632 about having CI with a Japanese MSVC install.
https://hg.mozilla.org/integration/mozilla-inbound/rev/0848b3201c40dd75711aeb24d3ab7ca72a351eef
bug 1318370 - stop using -Z7 for MSVC compilation with sccache. r=glandium

https://hg.mozilla.org/integration/mozilla-inbound/rev/a8032ae9cb2ad1c4574c6ac6f5c2778863cd71e0
bug 1318370 - move _DEPEND_CFLAGS+CL_INCLUDES_PREFIX to toolchain.configure, ignore {CC,CXX}_WRAPPER when using sccache r=glandium

https://hg.mozilla.org/integration/mozilla-inbound/rev/c0c0e10ed6131dda7d98315574b3b739081055b0
bug 1318370 - remove SCCACHE_NAMESERVER from mozconfig.cache, unused since we switched to sccache2. r=glandium
Pushed by philringnalda@gmail.com:
https://hg.mozilla.org/mozilla-central/rev/196f9ddbf7aa
followup, touch CLOBBER to try to unbreak mozilla-central Windows static-analysis builds, a=hope
The error in those builds:
03:48:04     INFO -  .deps/log.obj.pp:1: warning: overriding recipe for target 'log.obj'
03:48:04     INFO -  z:/task_1487730637/build/src/config/rules.mk:877: warning: ignoring old recipe for target 'log.obj'
03:48:04     INFO -  .deps/log.obj.pp:2: *** missing separator (did you mean TAB instead of 8 spaces?).  Stop.

This happens during the processing of the misc rule.

Now, some interesting facts, when comparing the log of a failed build with the log of a successful build on mozilla-central before the merge:
- the command line to compile log.obj is the exact same
- nothing in the log indicates outputting a .deps/log.obj.pp file
- both builds recurse in that directory during the misc tier, so if the file was created during the successful build, it should have failed the same way
- sccache is not used on either build, because we don't set a bucket for mozilla-central builds. That is however a difference with inbound, which does use sccache, which changes how things are compiled, and things work in that case.
- whatever is creating the .deps/log.obj.pp file, it's doing it wrong, cf. the error. What does it is presumably the mozbuild.action.cl, which is not used during sccache builds, and considering it's based on CL_INCLUDES_PREFIX, and CL_INCLUDES_PREFIX is being touched in this bug, my guess is that something is going wrong with that.
Confirmed on try:
- before the patch, CL_INCLUDES_PREFIX is "Note: including file: " in autoconf.mk.
- after the patch, it is "" in autoconf.mk.
Oh, that's because of:

@depends_when(c_compiler, when=building_with_msvc)

After the patch, CL_INCLUDES_PREFIX is not set when building with clang-cl, while it used to.
Ah, good catch!

Having build system behavior that varies based on whether sccache enabled makes it tricky to get right.
Oh, also, in light of bug 1341515, turns out host targets don't use COMPILE_PDB_FLAG.
Depends on: 1384557
Product: Core → Firefox Build System
Depends on: 1506848
Attachment #8830677 - Attachment is obsolete: true
Attachment #8830680 - Attachment is obsolete: true

Between bug 1506848 and bug 1384557 let's call this fixed. Followup work in bug 1509961.

Status: REOPENED → RESOLVED
Closed: 7 years ago5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.