Set Windows compiler options appropriately when using sccache

REOPENED
Assigned to

Status

REOPENED
2 years ago
8 days ago

People

(Reporter: ted, Assigned: ted)

Tracking

(Depends on: 2 bugs)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

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/﷒0﷓

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`.
Duplicate of this bug: 1266717
Depends on: 1322703
Assignee: nobody → ted
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Attachment #8830676 - Attachment is obsolete: true
Attachment #8830676 - Flags: review?(mh+mozilla)
Comment hidden (mozreview-request)

Comment 9

2 years ago
mozreview-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

::: 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 10

2 years ago
mozreview-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+
(Assignee)

Comment 11

2 years ago
mozreview-review-reply
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

Comment 15

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0848b3201c40
https://hg.mozilla.org/mozilla-central/rev/a8032ae9cb2a
https://hg.mozilla.org/mozilla-central/rev/c0c0e10ed613
https://hg.mozilla.org/mozilla-central/rev/72f6eace33a1
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54

Comment 16

2 years ago
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.
Backed out in https://hg.mozilla.org/mozilla-central/rev/a180b976c165 for https://queue.taskcluster.net/v1/task/M1jKEBsrQryF8YUGmT7iGQ/runs/0/artifacts/public/logs/live_backing.log
Status: RESOLVED → REOPENED
status-firefox54: fixed → ---
Resolution: FIXED → ---
Target Milestone: mozilla54 → ---
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

Updated

9 months ago
Product: Core → Firefox Build System
Depends on: 1506848
You need to log in before you can comment on or make changes to this bug.